After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 635809 - ParamSpec value_validate() on non ref counted boxed
ParamSpec value_validate() on non ref counted boxed
Status: RESOLVED FIXED
Product: gnome-perl
Classification: Bindings
Component: Glib
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk2-perl-bugs
gtk2-perl-bugs
Depends on:
Blocks:
 
 
Reported: 2010-11-25 20:55 UTC by Kevin Ryde
Modified: 2010-12-12 14:13 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (1.65 KB, patch)
2010-11-25 20:55 UTC, Kevin Ryde
none Details | Review
test case (1.21 KB, patch)
2010-11-25 20:59 UTC, Kevin Ryde
committed Details | Review
failing program (404 bytes, text/x-perl)
2010-11-25 21:07 UTC, Kevin Ryde
  Details
patch (2.19 KB, patch)
2010-11-28 00:24 UTC, Kevin Ryde
committed Details | Review

Description Kevin Ryde 2010-11-25 20:55:45 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().
Comment 1 Kevin Ryde 2010-11-25 20:59:06 UTC
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.
Comment 2 Kevin Ryde 2010-11-25 21:07:16 UTC
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).
Comment 3 Torsten Schoenfeld 2010-11-27 13:53:13 UTC
(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.
Comment 4 Kevin Ryde 2010-11-28 00:24:43 UTC
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.
Comment 5 Torsten Schoenfeld 2010-12-12 14:13:40 UTC
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!