GNOME Bugzilla – Bug 637832
Use correct type sizes when assigning to pointer out args in closures
Last modified: 2013-03-18 08:32:23 UTC
The pointer type is lost when _pygi_closure_convert_ffi_arguments() simply generalizes the pointer to a void*. Because of this, a simple assignment of the entire GIArgument union could overwrite adjacent memory. For example, on my 64 bit machine, when I implement get_preferred_width_for_height[1] which expects two int32 out args, one out arg clobbers the other with it's padding because GIArgument is 8 bytes long. So if I return a tuple, like (24, 24), One of the arguments will return natively as 0. Hope that makes sense. 1. http://library.gnome.org/devel/gtk/2.91/GtkWidget.html#gtk-widget-get-preferred-width-for-height
Created attachment 176898 [details] [review] Respect different type lengths when assigning out-argument pointers.
Thanks! Attachment 176898 [details] pushed as 8d5a785 - Respect different type lengths when assigning out-argument pointers.
Re-opening. I'm still observing the callers stack being smashed due to the default case in the switch statement. default: *((GIArgument *) retval) = arg; break; Since GIArgument is 8 bytes, marshaling unknown argument types of a smaller size will smash the callers stack frame. Found when attempting to implement the Gtk.Sortable.get_sort_column_id vfunc which expects a 4 byte flags as a return argument. We should basically add a few more types and remove the default case to fix this. Any default guessing here will be incorrect at some pointer or another.
Could this crasher be related? I'm occasionally seeing that, I haven't figured out a consistent way to reproduce it within my app, even less writing a testcase:
+ Trace 231512
Frame 7 is waiting for notify::model and the callbacks accesses a tree sortable which may trash the stack.
Created attachment 236114 [details] [review] Correct the size use for pointer out args enum/flags use a different outsize and causes stack corruption.
Created attachment 236115 [details] [review] Correct the size use for pointer out args enum/flags use a different outsize and causes stack corruption.
Review of attachment 236115 [details] [review]: Thanks Johan. I've listed a potential problem but I am uncertain as this is somewhat new territory for me. I was also thinking of doing some re-factoring of _pygi_argument_from_object to take a GIArgument pointer instead of returning one. This way we could completely get rid of _pygi_closure_assign_pyobj_to_out_argument and use the logic already available for correct assignment. ::: gi/pygi-closure.c @@ +91,1 @@ *((GIArgument *) retval) = arg; Does this suffer the same stack corruption problem on 32 bit machines? I think it is casting the lvalue into a 64 bit type (GIArgument). @@ +120,3 @@ + case GI_INFO_TYPE_TYPE: + case GI_INFO_TYPE_UNRESOLVED: + case GI_INFO_TYPE_STRUCT: Same here.
Possibly, large parts of the patch is untested on x64 as well. I'm only really testing basic types and enums.
For reference Gjs return marshaling is here: http://git.gnome.org/browse/gjs/tree/gi/function.c#n99 and out arg marshaling is done with: http://git.gnome.org/browse/gjs/tree/gi/arg.c#n1113 which is very similar to pygobjects pygi-argument.c:_pygi_argument_from_object http://git.gnome.org/browse/pygobject/tree/gi/pygi-argument.c#n874 I hope at some point we can convert _pygi_argument_from_object to take an out arg which is just the ffi argument cast into a GIArgument*. In the short term Johans fix with some tweaks should work.
Created attachment 239094 [details] [review] gimarshalingtests: Add tests for enum returned from vfunc Add vfuncs and methods for an object returning an enum or using one as an output argument.
Created attachment 239095 [details] [review] Fix stask smasher when marshaling enums as a vfunc return value Add special case for marshaling GI_TYPE_TAG_INTERFACE with enum or flag types. Default interfaces to marshal as a pointer. Fix leaking of GIBaseInfo when marshaling interface as out arg. Someone should run 'make check' on a 32 bit machine as well...
Comment on attachment 239095 [details] [review] Fix stask smasher when marshaling enums as a vfunc return value + case GI_INFO_TYPE_ENUM: + case GI_INFO_TYPE_FLAGS: + *(ffi_sarg *) retval = arg.v_long; GEnumValue is defined as gint, GFlagsValue as guint, so I don't think we should use v_long here. It again may cause wrong values on 32 bit or big endian machines. Same thing in the second hunk in _pygi_closure_assign_pyobj_to_out_argument(). default: - *((GIArgument *) retval) = arg; + *(ffi_arg *) retval = (ffi_arg) arg.v_uint64; I guess we should really raise an assertion there instead of blindly assuming that we can just copy the bits. This looks cleaner, but OOI, does that actually change something in practice, given that GIArgument is just a union, and arg.v_uint64 is the biggest entry anyway? Rest looks good to me, thanks!
(In reply to comment #12) > (From update of attachment 239095 [details] [review]) > + case GI_INFO_TYPE_ENUM: > + case GI_INFO_TYPE_FLAGS: > + *(ffi_sarg *) retval = arg.v_long; > > GEnumValue is defined as gint, GFlagsValue as guint, so I don't think we should > use v_long here. It again may cause wrong values on 32 bit or big endian > machines. Good to know, I will separate the assignments as such (GI_INFO_TYPE_FLAGS should also be using ffi_arg as the lvalue in this case). > > default: > - *((GIArgument *) retval) = arg; > + *(ffi_arg *) retval = (ffi_arg) arg.v_uint64; > > I guess we should really raise an assertion there instead of blindly assuming > that we can just copy the bits. This looks cleaner, but OOI, does that actually > change something in practice, given that GIArgument is just a union, and > arg.v_uint64 is the biggest entry anyway? I didn't want to assert here due to potential regression. Some things might work fine on 64 bit architectures with the old way but fail on 32 bit here. We also don't have enough test cases yet for testing everything with this and I don't have enough time to be totally thorough ATM. This will change things because it is casting the lvalue to an ffi_arg instead of a GIArgument, GIArgument is always 64 bits whereas ffi_arg is an ulong, so this would fix things for 32 bit architectures (this was actually taken from Gjs but that also doesn't mean it's correct though).
(In reply to comment #13) > Good to know, I will separate the assignments as such (GI_INFO_TYPE_FLAGS > should also be using ffi_arg as the lvalue in this case). Right, thanks. > I didn't want to assert here due to potential regression. Indeed. I actually meant to write "but let's not do this for 3.7.x to avoid regressions", but somehow forgot this, sorry. > This will change things because it is casting the lvalue to an ffi_arg instead > of a GIArgument, GIArgument is always 64 bits whereas ffi_arg is an ulong, so > this would fix things for 32 bit architectures (this was actually taken from > Gjs but that also doesn't mean it's correct though). Indeed, thanks!
Created attachment 239098 [details] [review] Fix stack smasher when marshaling enums as a vfunc return value Add special case for marshaling GI_TYPE_TAG_INTERFACE with enum or flag types. Default interfaces to marshal as a pointer. Add explicit cases for GType and Unichar out/return marshaling. Fix leaking of GIBaseInfo when marshaling interface as out arg.
Comment on attachment 239098 [details] [review] Fix stack smasher when marshaling enums as a vfunc return value + g_base_info_unref(interface); Tiny nitpick, please add a space before the (. Not that code style consistency wouldn't already be hopeless in pygobject :-) Looks great, thank you!
Attachment 239098 [details] pushed as a0844a8 - Fix stack smasher when marshaling enums as a vfunc return value