GNOME Bugzilla – Bug 711802
Fix memory leaks in libgio tests
Last modified: 2018-05-24 15:59:38 UTC
In order to make libgio tests be valgrindable, I'll post patches which fix memory leaks. In general it's good practice to free stuff in tests, so that we can run through those code paths, and make sure we received appropriate references/memory.
Created attachment 259479 [details] [review] buffered-output-stream: Fix some access after free The memory pointed to by a GMemoryOutputStream can be realloc'd
Created attachment 259480 [details] [review] data-output-stream: Fix some access after free The memory pointed to by a GMemoryOutputStream can be realloc'd
Created attachment 259481 [details] [review] contexts: Fix memory leak in test
Created attachment 259484 [details] [review] g-icon: Fix memory leak in test
Created attachment 259486 [details] [review] gsubprocess: Fix error detection logic in tests Various tests were depending on local_error being set by a callback when it could never have been the case. Simplify async error detection logic in those cases, and fix leak of GError.
Created attachment 259487 [details] [review] gsubprocess: Fix leaks in tests
Created attachment 259497 [details] [review] gdbus-auth: Fix leaks in tests
Created attachment 259498 [details] [review] gdbus-connection: Fix use after free in test
Created attachment 259499 [details] [review] gdbus-connection-slow: Fix leak in tests
Created attachment 259500 [details] [review] gdbus-connection: Fix leaks in tests
Created attachment 259502 [details] [review] gdbus-export: Fix leaks in tests
Created attachment 259503 [details] [review] gdbus-introspection: Fix leaks in tests
Created attachment 259506 [details] [review] gdbus-connection-loss: Fix leak in test
Review of attachment 259479 [details] [review]: sure
Review of attachment 259480 [details] [review]: sure
Review of attachment 259481 [details] [review]: ok
Review of attachment 259484 [details] [review]: sure
Review of attachment 259486 [details] [review]: humm, ok
Review of attachment 259487 [details] [review]: absolutely
Review of attachment 259497 [details] [review]: ::: gio/tests/gdbus-auth.c @@ +199,3 @@ g_thread_join (client_thread); + while (g_main_context_iteration (NULL, FALSE)); The empty loop body looks a little deceptive here. Maybe do { } while (...); would make it more obvious ?
Review of attachment 259498 [details] [review]: ugh, yes
Review of attachment 259499 [details] [review]: ok
Review of attachment 259500 [details] [review]: sure
Review of attachment 259502 [details] [review]: Not sure I see why you need to strdup the dyna_data members - what does that fix ?
Review of attachment 259503 [details] [review]: ok
Review of attachment 259506 [details] [review]: ok
Review of attachment 259502 [details] [review]: ::: gio/tests/gdbus-export.c @@ +1203,2 @@ /* now register a dynamic subtree, spawning objects as they are called */ + dyna_data = g_ptr_array_new_with_free_func (g_free); The array now contains heap allocated strings, freed with g_free(). Previously only one of the items were allocated on the heap. Now they all are.
Thanks for the review. First pass pushing patches that don't need modification. Attachment 259479 [details] pushed as 7217124 - buffered-output-stream: Fix some access after free Attachment 259480 [details] pushed as 438f711 - data-output-stream: Fix some access after free Attachment 259481 [details] pushed as faafd4c - contexts: Fix memory leak in test Attachment 259484 [details] pushed as 78ad171 - g-icon: Fix memory leak in test Attachment 259486 [details] pushed as fe3c878 - gsubprocess: Fix error detection logic in tests Attachment 259487 [details] pushed as 95526b5 - gsubprocess: Fix leaks in tests Attachment 259498 [details] pushed as 0d51ff7 - gdbus-connection: Fix use after free in test Attachment 259499 [details] pushed as f80e269 - gdbus-connection-slow: Fix leak in tests Attachment 259500 [details] pushed as 91c8fb8 - gdbus-connection: Fix leaks in tests
(In reply to comment #20) > Review of attachment 259497 [details] [review]: > > ::: gio/tests/gdbus-auth.c > @@ +199,3 @@ > g_thread_join (client_thread); > > + while (g_main_context_iteration (NULL, FALSE)); > > The empty loop body looks a little deceptive here. Maybe > > do { > } while (...); > > would make it more obvious ? Hmmm, this is fairly common (esp. when dealing testing gdbus or stuff that uses it). It would be a shame if every instance turned into three lines. Do you think a new function like g_main_context_drain() is in order?
Attachment 259503 [details] pushed as 14b27ea - gdbus-introspection: Fix leaks in tests Attachment 259506 [details] pushed as 670379b - gdbus-connection-loss: Fix leak in test
Review of attachment 259502 [details] [review]: ah, I missed that point. Would be good to put that explanation in the commit message
Comment on attachment 259506 [details] [review] gdbus-connection-loss: Fix leak in test Was causing distcheck to fail for Ryan, reverted.
Attachment 259497 [details] pushed as db6a297 - gdbus-auth: Fix leaks in tests Attachment 259502 [details] pushed as baed90a - gdbus-export: Fix leaks in tests
(In reply to Stef Walter from comment #32) > Comment on attachment 259506 [details] [review] [review] > gdbus-connection-loss: Fix leak in test > > Was causing distcheck to fail for Ryan, reverted. I’ve applied this patch locally and run the test in a loop, and eventually reproduced the failure, which looks like bug #725503: (gdb) t a a bt
+ Trace 238095
Thread 1 (Thread 0x7fe0d9272c40 (LWP 9733))
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/786.