GNOME Bugzilla – Bug 674800
gclosure: generic marshaller leaks return value
Last modified: 2012-08-05 12:41:25 UTC
Created attachment 212788 [details] [review] gclosure: generic marshaller should not copy and leak return value Conversion from ffi type to GValue always uses g_value_set_... which in some cases (e.g. g_value_set_string) leaks the real returned value (from the callback) only to return a copy. Attached patch replaces some _set_ by _take_ (only not sure about variant case, but looks like the _set_ should be ok).
*** Bug 679829 has been marked as a duplicate of this bug. ***
do you have a testcase that demonstrates this leak when run under valgrind ?
Created attachment 219233 [details] [review] tests: make refcount signals test slightly valgrind cleaner
Created attachment 219235 [details] [review] tests: add check for leaking signal return value Not sure what scope you have in mind with a 'test case', but this patch modifies one of the glib tests to include a signal with a string return value, which will shown as leaked by valgrind (among other cases as can be seen from the original patch). The previous patch establishes a 'slightly cleaner valgrind baseline'.
Review of attachment 212788 [details] [review]: Interesting; I guess signals returning allocated memory are sufficiently rare in the GTK+/GNOME stack that we didn't notice this before. I certainly don't see any in GIO. ::: gobject/gclosure.c @@ +1274,3 @@ break; case G_TYPE_VARIANT: g_value_set_variant (gvalue, *(gpointer*)value); Don't we need to change this to g_value_take_variant() too ?
Review of attachment 219233 [details] [review]: Looks correct.
Review of attachment 219235 [details] [review]: Note that tests/ directory is stuff that predates gtestutils.h. So it's preferable to add new gobject tests to gobject/tests/, if there were an appropriate test there already.
(In reply to comment #5) > Review of attachment 212788 [details] [review]: > > Interesting; I guess signals returning allocated memory are sufficiently rare > in the GTK+/GNOME stack that we didn't notice this before. I certainly don't > see any in GIO. > > ::: gobject/gclosure.c > @@ +1274,3 @@ > break; > case G_TYPE_VARIANT: > g_value_set_variant (gvalue, *(gpointer*)value); > > Don't we need to change this to g_value_take_variant() too ? Was not sure about the 'floating' here (with variant), but contrary to the original comment, it probably needs changing to g_value_take_variant().
Created attachment 219469 [details] [review] gclosure: generic marshaller should not copy and leak return value Patch as before, but now also with g_value_take_variant()
Review of attachment 219469 [details] [review]: OK.
Mark, do you have developer access to git.gnome.org ? It would be good to get this in as soon as possible.
Only have gnome bugzilla account, but no gnome developer account.
Took the liberty to push this to master and 2.32 branch then: commit 10fc00b38515bc322d5e497d61a50165b04c3c8d Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Wed Apr 25 14:21:06 2012 +0200 gclosure: do not copy and leak when generically marshalling return value https://bugzilla.gnome.org/show_bug.cgi?id=674800 commit 24b9f61ee4b8b7bc755b84398207112c18b4121f Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Jul 19 16:38:59 2012 +0200 tests: add check for leaking signal return value https://bugzilla.gnome.org/show_bug.cgi?id=674800 commit 13a1154b4cf664e486df9fa6a76d279b8090c49e Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Jul 19 16:38:06 2012 +0200 tests: make refcount signals test slightly valgrind cleaner make check still passes on both branches and leak fix was confirmed via valgrind on the new test.