GNOME Bugzilla – Bug 666113
various leaks in GLib, GIO are visible in the regression tests
Last modified: 2012-01-16 18:32:55 UTC
Running the GLib regression tests under valgrind (for which I'll submit a patch soon) reveals various leaks: * g_hmac_copy gives the new GHMac an uninitialized refcount, which in practice means the GHMac is leaked * g_hmac_get_string leaks a buffer the size of the hash * g_key_file_get_string_list leaks a GSList of the encoded strings if one of them can't be decoded * clearing a GArray, GByteArray, GPtrArray while refs still exist results in leaking the actual struct (but not its contents) * GKeyFile leaks group comments if the group is removed * g_variant_byteswap leaks a copy of the serialized data Additionally: * g_data_set_internal() has a harmless use-after-free * g_strcompress segfaults when its argument is NULL Patches to follow.
Created attachment 203362 [details] [review] g_hmac_copy: initialize the refcount In practice, the uninitialized refcount will typically mean that the copy is never freed, and leaks.
Created attachment 203363 [details] [review] g_hmac_get_string: don't allocate and leak an unused buffer Also document why we're not actually using the buffer for anything.
Created attachment 203364 [details] [review] g_key_file_get_string_list: don't leak the pieces on error
Created attachment 203365 [details] [review] Don't leak GArray etc. if cleared while a ref exists While running the GLib tests under valgrind I discovered that this sequence: p = g_ptr_array_new (); g_ptr_array_ref (p); g_ptr_array_free (p, TRUE); /* now p is empty but still exists */ g_ptr_array_unref (p); /* now p should have been freed */ leaks the actual GPtrArray struct (although not its contents, if any). GArray (and GByteArray, which is internally the same thing) has the same bug.
Created attachment 203366 [details] [review] GKeyFile: free group comments when the group is removed These were leaked. Valgrind was sad.
Created attachment 203367 [details] [review] g_strcompress: check that source is non-NULL rather than just crashing Calling this function with a NULL argument is considered to be invalid, but one of the regression tests does it anyway (to watch it crash), which seems a good indication that it's expected to be somewhat common. Let's check it rather than segfaulting.
Created attachment 203368 [details] [review] g_variant_byteswap: don't leak serialised.data
Created attachment 203369 [details] [review] g_data_set_internal: avoid use-after-free if datalist is in dataset Removing the last thing in a dataset frees the dataset, and if the datalist was in a dataset, we can't safely unlock it after the dataset has been freed. Unlock it sooner.
Created attachment 203370 [details] [review] GDBusActionGroup: don't leak param_str --- I was going to do the GIO patches separately, but there are only a couple and they're relatively simple...
Created attachment 203371 [details] [review] g_dbus_action_group_changed: don't leak iterator and its contents
All of these except the two GDBusActionGroup patches could probably backport to 2.30 or even 2.28, but I haven't checked that yet.
Review of attachment 203362 [details] [review]: obviously correct.
Review of attachment 203363 [details] [review]: looks good to me
Review of attachment 203364 [details] [review]: ::: glib/gkeyfile.c @@ +1974,3 @@ + g_free (p->data); + + g_slist_free (pieces); you can use g_slist_free_full() here.
Review of attachment 203366 [details] [review]: looks good to me
Review of attachment 203367 [details] [review]: looks good to me
Review of attachment 203371 [details] [review]: looks good to me
Review of attachment 203367 [details] [review]: Good point
Review of attachment 203369 [details] [review]: Good catch. Was this showing up in any tests ? I guess it just didn't crash ?
Review of attachment 203370 [details] [review]: Looks right
Review of attachment 203365 [details] [review]: ::: glib/garray.c @@ +307,3 @@ /* if others are holding a reference, preserve the wrapper but do free/return the data */ + if (!g_atomic_int_dec_and_test (&array->ref_count)) + flags |= PRESERVE_WRAPPER; I guess this is the crucial change here. Might be easier to see by splitting it into 2 patches, one for introducing the flags without behavior change, and then the actual fix.
Review of attachment 203368 [details] [review]: Looks right.
All patches applied except Attachment #203364 [details] and Attachment #203365 [details]. (In reply to comment #20) > Review of attachment 203369 [details] [review] [the GData one] > > Good catch. Was this showing up in any tests ? I guess it just didn't crash ? Yes, it's wrong-but-harmless in practice: it's a write to freed memory, so even memory debuggers that scrub freed memory wouldn't spot it (but Valgrind does, by remembering that the freed memory shouldn't be accessed). The only way I can see that it could crash a real application is if another thread made a memory allocation in a very small window (between the free and the lock) and was given the just-freed block. (In reply to comment #14) > Review of attachment 203364 [details] [review] ... > you can use g_slist_free_full() here. Sure, will do. (In reply to comment #22) > Review of attachment 203365 [details] [review] > > Might be easier to see by splitting it into 2 patches, one for introducing the > flags without behavior change, and then the actual fix. I'll try that and see how it looks; if it doesn't improve clarity I might just commit what I originally wrote, though.
Created attachment 203494 [details] [review] [v2] g_key_file_get_string_list: don't leak the pieces on error --- Now with 100% more g_slist_free_full.
Created attachment 203495 [details] [review] GArray, GPtrArray: factor out the actual freeing Depending how the array is freed, we may want to free the underlying array (the "segment"), the struct wrapper or both. --- You're right, splitting this one up is nicer.
Created attachment 203496 [details] [review] g_array_free, g_ptr_array_free: decrement refcount if not the last ref foo_free is conceptually "worth" one unref; not decrementing the refcount here means the GArray or GPtrArray wrapper (but not its contents) would leak in the following call sequence: p = g_ptr_array_new (); g_ptr_array_ref (p); g_ptr_array_free (p, TRUE); g_ptr_array_unref (p); --- Relies on Attachment #203495 [details].
Review of attachment 203494 [details] [review]: LGTM
Review of attachment 203495 [details] [review]: LGTM
Review of attachment 203496 [details] [review]: LGTM
(In reply to comment #30) > LGTM All pushed, thanks. Fixed in git for 2.31.5. I'll leave this open while I work out which ones are applicable to 2.30.
(In reply to comment #0) > * g_hmac_copy gives the new GHMac an uninitialized refcount, which in > practice means the GHMac is leaked Also in 2.30; new API since 2.28 > * g_hmac_get_string leaks a buffer the size of the hash Also in 2.30; new API since 2.28 > * g_key_file_get_string_list leaks a GSList of the encoded strings if > one of them can't be decoded Also in 2.30 and 2.28 > * clearing a GArray, GByteArray, GPtrArray while refs still exist results > in leaking the actual struct (but not its contents) Also in 2.30 and 2.28 > * GKeyFile leaks group comments if the group is removed Also in 2.30 and 2.28 > * g_variant_byteswap leaks a copy of the serialized data Not in 2.30 or 2.28, regressed in 2.31.x > * g_data_set_internal() has a harmless use-after-free Also in 2.30 but not 2.28, regressed somewhere between the two > * g_strcompress segfaults when its argument is NULL Also in 2.30 and 2.28 (In reply to comment #9) > GDBusActionGroup: don't leak param_str Not in 2.30 or 2.28, new API (In reply to comment #10) > g_dbus_action_group_changed: don't leak iterator and its contents Not in 2.30 or 2.28, new API I've pushed the applicable fixes to the 2.28 and 2.30 branches. Fixed in git for 2.28.9 and 2.30.3, if either of those ever happens. (Yes, I know further releases from these branches are unlikely, particularly 2.28 - but I have to maintain a branch based on 2.28 anyway, so I might as well make the patches easy to find for others in my situation.)