mistakes with g_value_set_boxed()

In today’s PSA, mistakes with g_value_set_boxed(). A mistake that’s been made several times by yours truly, and only realised today thanks to Xavier.

At some point in the GLib 2.22 cycle, types such as GArray, GPtrArray, GByteArray and GHashTable ((You can discover what g_boxed_copy() and g_boxed_free() will do for a type by looking for its G_DEFINE_BOXED_TYPE in GLib, which is probably in gobject/gboxed.c.)) gained ref() and unref() methods, which allowed things like g_boxed_copy() and g_boxed_free() to be a stack faster for those types. This was mid-2008.

Danni, who didn’t get the memo, had been occasionally writing bits of code like this:

GArray *array = g_array_new (...);

g_value_set_boxed (value, array);
g_array_free (array, TRUE);

Which, if you read the code, will keep the wrapper alive, so your code won’t crash, but will release all the data the array contains. Similarly for g_ptr_array_free() etc. It nicely zeros out the length of the array, so when you come to read it in your GValue, it looks empty.

The correct thing to do (of course) is unref your data, which will then free the memory and the wrapper when the refcount reaches zero.

GArray *array = g_array_new (...);

g_value_set_boxed (value, array);
g_array_unref (array);

Update: As pointed out in this simple example, you can replace this with the following pattern:

GArray *array = g_array_new (...);

g_value_take_boxed (value, array);

Which passes your ownership to the GValue. Where this falls down is for more complex values built for dbus-glib, where the ownership isn’t clear-cut, and so you need to free the components separately afterwards.

Author: Danielle

Danielle is an Australian software engineer, computer scientist and feminist. She doesn't really work on GNOME any more (sadly). Opinions and writing are solely her own and so not represent her employer, the GNOME Foundation, or anyone else but herself. View all posts by Danielle

4 thoughts on “mistakes with g_value_set_boxed()”

Thanks for blogging about this. I’ve fixed telepathy-glib using that magic command:

for f in `find -name “*.c”`; do echo $f; sed $f -re ‘s/g_array_free $([^ ,]+), TRUE$/g_array_unref $\1$/’ > $f.tmp; mv $f.tmp $f; done

repeat with g_ptr_array, g_byte_array, g_hash_table, etc…

@xclaesse: I figure this approach is fairly safe. The worst that could happen is you could leak the data because you leaked a ref somewhere. However, seeing as you would have previously leaked the wrapper anyway, the resulting bug should be much easier to detect.

the pattern:

  pointer foo = g_<boxed_type>_new ();
  g_value_set_boxed (value, foo);
  g_<boxed_type>_free (pointer);

should have not been used in the first place. if you want to assign a boxed type to a GValue and transfer ownership you should use g_value_take_boxed() instead.

In this simple case, you’re right, that works, but sometimes you can’t do this because you don’t own everything that will be taken. This is very common when you’re building complex structures for dbus-glib.

Author: Danielle

4 thoughts on “mistakes with g_value_set_boxed()”

Leave a Reply