GNOME Bugzilla – Bug 786456
g_subprocess_wait[_check]_async() breaks when cancelled after subprocess has exited
Last modified: 2017-08-23 10:38:48 UTC
g_subprocess_wait[_check]_async() sometimes attempts to dispatch its callback twice if due to a synchronous g_subprocess_wait(), the subprocess is already known to be dead, and its GCancellable is subsequently cancelled (whether before the callback is called, or from within the callback). The results vary: * If cancelled before the callback is called, we see "GLib-GIO-CRITICAL **: g_task_return_boolean: assertion 'task->result_set == FALSE' failed", and the callback is called exactly once * If cancelled within the callback, the callback is called again, in a re-entrant fashion Either way, bad news. Patch adding failing tests for both scenarios to follow.
Created attachment 357873 [details] [review] gio: failing cases for subprocess cancellable bug
Failing test output: $ gtester -p /gsubprocess/exit1 -k --verbose ./gio/tests/gsubprocess TEST: /home/wjt/src/gnome/glib/gio/tests/.libs/lt-gsubprocess... (pid=9816) /gsubprocess/exit1: OK /gsubprocess/exit1/cancel: (/home/wjt/src/gnome/glib/gio/tests/.libs/lt-gsubprocess:9816): GLib-GIO-CRITICAL **: g_task_return_boolean: assertion 'task->result_set == FALSE' failed FAIL GTester: last random seed: R02Se46301658697ca4f59e1601a5ef142a0 (pid=9849) /gsubprocess/exit1/cancel_in_cb: ** GLib-GIO:ERROR:gsubprocess.c:262:test_exit1_cancel_in_cb_wait_check_cb: 'data->cb_called' should be FALSE FAIL GTester: last random seed: R02Sebfb5d4999b88868055af0315a901206 (pid=9870) FAIL: /home/wjt/src/gnome/glib/gio/tests/.libs/lt-gsubprocess
Review of attachment 357873 [details] [review]: These tests look good to me. a_c-n, although don’t actually commit them until we’ve got a fix otherwise our test failure stats will look bad. :-)
Created attachment 358221 [details] [review] gio: Fix double-callback on cancellation with GSubprocess See bug #786456 for a detailed analysis of the situation which can cause this (in summary, if a g_subprocess_wait_async() call is cancelled on a GSubprocess which is already known to be dead). The problem was that the GCancellable callback handler was unconditionally returning a result for the GTask for g_subprocess_wait_async(), even if that GTask had already returned a result and the callback was being invoked after the GTask had been removed from the pending_waits list. Fix that by checking whether the GTask is still in the pending_waits list before returning a result for it. Thanks to Will Thompson for some very useful unit tests which reproduce this (which will be pushed in the following commit). Signed-off-by: Philip Withnall <withnall@endlessm.com>
Created attachment 358222 [details] [review] tests: Fix some leaks and double-frees in the GSubprocess tests Signed-off-by: Philip Withnall <withnall@endlessm.com>
Here’s a fix. When these patches are pushed, it would be best to push them in the order: fix, unit tests, leak fixes; so that the commit message from the fix refers to the unit tests correctly.
Review of attachment 358222 [details] [review]: :+1:
Review of attachment 358221 [details] [review]:
Attachment 357873 [details] pushed as 65a95a5 - gio: failing cases for subprocess cancellable bug Attachment 358221 [details] pushed as 8f86d31 - gio: Fix double-callback on cancellation with GSubprocess Attachment 358222 [details] pushed as 73eee8d - tests: Fix some leaks and double-frees in the GSubprocess tests