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`

^{1} 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

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:

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.