GNOME Bugzilla – Bug 635809
ParamSpec value_validate() on non ref counted boxed
Last modified: 2010-12-12 14:13:47 UTC
Created attachment 175274 [details] [review] patch I think the code I made for value_validate() is no good on non reference counted boxed objects like GdkRectangle. In the 1.220 gperl_value_from_sv() copies to the local GValue, gperl_sv_from_value() makes a pointer into there, ie. doesn't copy), but then the memory is gone by g_value_unset of that local GValue. In the cvs head it's slightly better. gperl_value_from_sv() makes only a pointer to the incoming SV's memory, not a copy, gperl_sv_from_value() too makes a pointer not a copy, so you end up with an alias of the incoming SV. Which is ok so long as you don't naively discard that original SV. I'm looking at the change below to give back the input SV if value_validate() says it's ok already, and to copy out the GValue if it says it was changed. This would be for both 1.220 and the cvs head. The only thing I'm slightly unsure is whether setting the GValue to point into the original SV is right. Is g_param_value_validate() expected to set in a new block of memory if it makes a change, or will it modify the content in-place? I don't suppose anyone has every actually created a paramspec subtype taking a GdkRectangle or similar and mangled in value_validate().
Created attachment 175275 [details] [review] test case Bit of code for GdkRegion.t exercising value_validate(). Don't think there's any non ref counted boxed in base Glib to try this on.
Created attachment 175276 [details] failing program This program in 1.220 gives a bad return like newrect values: 0 2 3 4 or in the cvs head newrect values: 166513440 2 3 4 In the head it's ok if you don't "undef $rect" since that discards the memory aliased by $newrect. Dunno if that should be merely some pod instead of some code, but returning the original SV helps avoid that problem (by effectively assigning $newrect=$rect instead of mucking about with new boxed pointers).
(In reply to comment #0) > I'm looking at the change below to give back the input SV if value_validate() > says it's ok already, and to copy out the GValue if it says it was changed. > This would be for both 1.220 and the cvs head. Sounds correct to me. > The only thing I'm slightly unsure is whether setting the GValue to point into > the original SV is right. Is g_param_value_validate() expected to set in a new > block of memory if it makes a change, or will it modify the content in-place? > I don't suppose anyone has every actually created a paramspec subtype taking a > GdkRectangle or similar and mangled in value_validate(). It seems like a value_validate vfunc is allowed to do both, modify in-place and install a modified chunk of memory. param_string_validate (in gparamspecs.c), for example, can apparently do both depending on whether G_VALUE_NOCOPY_CONTENTS is set (i.e. whether g_value_set_static_string was used). In git master, we use g_value_set_static_boxed so that the G_VALUE_NOCOPY_CONTENTS flag will be set. So I think any well-behaved paramspec implementation will then have to refrain from modifying the memory in-place. The stable-1-22 branch is safe because it uses g_value_set_boxed which copies the boxed object. Does that sound correct to you? If so, I'll go ahead and commit your patches.
Created attachment 175394 [details] [review] patch Sounds close. I tweaked the comments per attached. Feel free to write something more. The only thing I further wondered was whether a GValue to SV like this might look at the NOCOPY flag, or whatever the public form of that is, and copy if the memory belongs to the GValue or just use the pointer if it belongs to someone else. That might apply generally in a few places, or it might be a bit too subtle, I'm not sure.
I don't see a public form of G_VALUE_NOCOPY_CONTENTS, so I think this is the best we can do. Patch and test case committed. Thanks!