diff options
author | Philip Withnall <pwithnall@endlessos.org> | 2023-02-22 02:40:35 +0000 |
---|---|---|
committer | Philip Withnall <pwithnall@endlessos.org> | 2023-02-23 12:11:24 +0000 |
commit | 4900ea5215e329fbfe893be7975cf05ff153ef81 (patch) | |
tree | fa249344689c66d7f151bffc6b5c76e2da405bf3 /gio | |
parent | 88fd3f0a762f28f3296a953aee17cf1d440997c1 (diff) | |
download | glib-4900ea5215e329fbfe893be7975cf05ff153ef81.tar.gz |
gdbusconnection: Fix double unref on timeout/cancel sending a message
This appears to fix an intermittent failure seen when sending a D-Bus
message with either of a cancellable or a timeout set.
In particular, I can reliably reproduce it with:
```
meson test gdbus-test-codegen-min-required-2-64 --repeat 10000
```
It can be caught easily with asan when reproduced. Tracking down the
location of the refcount mismatch was a little tricky, but was
simplified by replacing a load of `g_object_ref (message)` calls with
`g_dbus_message_copy (message, NULL)` to switch `GDBusMessage` handling
to using copy semantics. This allowed asan to home in on where the
refcount mismatch was happening.
The problem was that `send_message_data_deliver_error()` takes ownership
of the `GTask` passed to it, but the
`send_message_with_replace_cancelled_idle_cb()` and
`send_message_with_reply_timeout_cb()` functions which were calling it,
were not passing in a strong reference as they should have.
Another approach to fixing this would have been to change the transfer
semantics of `send_message_data_deliver_error()` so it was `(transfer
none)` on its `GTask`. That would probably have resulted in cleaner
code, but would have been a lot harder to verify/review the fix, and
easier to inadvertently introduce new bugs.
The fact that the bug was only triggered by the cancellation and timeout
callbacks explains why it was intermittent: these code paths are
typically never hit, but the timeout path may sometimes be hit on a very
slow test run.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #1264
Diffstat (limited to 'gio')
-rw-r--r-- | gio/gdbusconnection.c | 12 |
1 files changed, 7 insertions, 5 deletions
diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index d938f71b9..06c8a6141 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -1822,7 +1822,7 @@ send_message_data_deliver_reply_unlocked (GTask *task, ; } -/* Called from a user thread, lock is not held */ +/* Called from a user thread, lock is not held; @task is (transfer full) */ static void send_message_data_deliver_error (GTask *task, GQuark domain, @@ -1849,13 +1849,14 @@ send_message_data_deliver_error (GTask *task, /* ---------------------------------------------------------------------------------------------------- */ -/* Called from a user thread, lock is not held; @task is (transfer full) */ +/* Called from a user thread, lock is not held; @task is (transfer none) */ static gboolean send_message_with_reply_cancelled_idle_cb (gpointer user_data) { GTask *task = user_data; - send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_CANCELLED, + g_object_ref (task); + send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_CANCELLED, _("Operation was cancelled")); return FALSE; } @@ -1879,13 +1880,14 @@ send_message_with_reply_cancelled_cb (GCancellable *cancellable, /* ---------------------------------------------------------------------------------------------------- */ -/* Called from a user thread, lock is not held; @task is (transfer full) */ +/* Called from a user thread, lock is not held; @task is (transfer none) */ static gboolean send_message_with_reply_timeout_cb (gpointer user_data) { GTask *task = user_data; - send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT, + g_object_ref (task); + send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_TIMED_OUT, _("Timeout was reached")); return FALSE; } |