1From 4900ea5215e329fbfe893be7975cf05ff153ef81 Mon Sep 17 00:00:00 2001 2From: Philip Withnall <pwithnall@endlessos.org> 3Date: Wed, 22 Feb 2023 02:40:35 +0000 4Subject: [PATCH 1/9] gdbusconnection: Fix double unref on timeout/cancel 5 sending a message 6 7This appears to fix an intermittent failure seen when sending a D-Bus 8message with either of a cancellable or a timeout set. 9 10In particular, I can reliably reproduce it with: 11``` 12meson test gdbus-test-codegen-min-required-2-64 --repeat 10000 13``` 14 15It can be caught easily with asan when reproduced. Tracking down the 16location of the refcount mismatch was a little tricky, but was 17simplified by replacing a load of `g_object_ref (message)` calls with 18`g_dbus_message_copy (message, NULL)` to switch `GDBusMessage` handling 19to using copy semantics. This allowed asan to home in on where the 20refcount mismatch was happening. 21 22The problem was that `send_message_data_deliver_error()` takes ownership 23of the `GTask` passed to it, but the 24`send_message_with_replace_cancelled_idle_cb()` and 25`send_message_with_reply_timeout_cb()` functions which were calling it, 26were not passing in a strong reference as they should have. 27 28Another approach to fixing this would have been to change the transfer 29semantics of `send_message_data_deliver_error()` so it was `(transfer 30none)` on its `GTask`. That would probably have resulted in cleaner 31code, but would have been a lot harder to verify/review the fix, and 32easier to inadvertently introduce new bugs. 33 34The fact that the bug was only triggered by the cancellation and timeout 35callbacks explains why it was intermittent: these code paths are 36typically never hit, but the timeout path may sometimes be hit on a very 37slow test run. 38 39Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 40 41Fixes: #1264 42 43Conflict:NA 44Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/4900ea5215e329fbfe893be7975cf05ff153ef81 45 46--- 47 gio/gdbusconnection.c | 12 +++++++----- 48 1 file changed, 7 insertions(+), 5 deletions(-) 49 50diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c 51index d938f71b99..06c8a6141f 100644 52--- a/gio/gdbusconnection.c 53+++ b/gio/gdbusconnection.c 54@@ -1822,7 +1822,7 @@ send_message_data_deliver_reply_unlocked (GTask *task, 55 ; 56 } 57 58-/* Called from a user thread, lock is not held */ 59+/* Called from a user thread, lock is not held; @task is (transfer full) */ 60 static void 61 send_message_data_deliver_error (GTask *task, 62 GQuark domain, 63@@ -1849,13 +1849,14 @@ send_message_data_deliver_error (GTask *task, 64 65 /* ---------------------------------------------------------------------------------------------------- */ 66 67-/* Called from a user thread, lock is not held; @task is (transfer full) */ 68+/* Called from a user thread, lock is not held; @task is (transfer none) */ 69 static gboolean 70 send_message_with_reply_cancelled_idle_cb (gpointer user_data) 71 { 72 GTask *task = user_data; 73 74- send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_CANCELLED, 75+ g_object_ref (task); 76+ send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_CANCELLED, 77 _("Operation was cancelled")); 78 return FALSE; 79 } 80@@ -1879,13 +1880,14 @@ send_message_with_reply_cancelled_cb (GCancellable *cancellable, 81 82 /* ---------------------------------------------------------------------------------------------------- */ 83 84-/* Called from a user thread, lock is not held; @task is (transfer full) */ 85+/* Called from a user thread, lock is not held; @task is (transfer none) */ 86 static gboolean 87 send_message_with_reply_timeout_cb (gpointer user_data) 88 { 89 GTask *task = user_data; 90 91- send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT, 92+ g_object_ref (task); 93+ send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_TIMED_OUT, 94 _("Timeout was reached")); 95 return FALSE; 96 } 97-- 98GitLab 99 100 101From 127c899a2e727d10eb88b8fae196add11a6c053f Mon Sep 17 00:00:00 2001 102From: Philip Withnall <pwithnall@endlessos.org> 103Date: Wed, 22 Feb 2023 02:45:15 +0000 104Subject: [PATCH 2/9] gdbusconnection: Fix the type of a free function 105MIME-Version: 1.0 106Content-Type: text/plain; charset=UTF-8 107Content-Transfer-Encoding: 8bit 108 109This didn’t actually cause any observable bugs, since the structures of 110`PropertyData` and `PropertyGetAllData` were equivalent for the members 111which the free function touches. 112 113Definitely should be fixed though. 114 115Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 116 117Conflict:NA 118Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/127c899a2e727d10eb88b8fae196add11a6c053f 119 120--- 121 gio/gdbusconnection.c | 2 +- 122 1 file changed, 1 insertion(+), 1 deletion(-) 123 124diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c 125index 06c8a6141f..6a0d67a8ee 100644 126--- a/gio/gdbusconnection.c 127+++ b/gio/gdbusconnection.c 128@@ -4584,7 +4584,7 @@ typedef struct 129 } PropertyGetAllData; 130 131 static void 132-property_get_all_data_free (PropertyData *data) 133+property_get_all_data_free (PropertyGetAllData *data) 134 { 135 g_object_unref (data->connection); 136 g_object_unref (data->message); 137-- 138GitLab 139 140 141From 90af20d9505a11d02e81a4f8fa09ee15faba96b8 Mon Sep 17 00:00:00 2001 142From: Philip Withnall <pwithnall@endlessos.org> 143Date: Wed, 22 Feb 2023 02:46:55 +0000 144Subject: [PATCH 3/9] gdbusconnection: Improve docs of message ownership in 145 closures 146 147This introduces no functional changes, but makes it a little clearer how 148the ownership of these `GDBusMessage` instances works. The free function 149is changed to `g_clear_object()` to avoid the possibility of somehow 150using the messages after freeing them. 151 152Basically just some drive-by docs improvements while trying to debug 153issue #1264. 154 155Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 156 157Helps: #1264 158 159Conflict:NA 160Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/90af20d9505a11d02e81a4f8fa09ee15faba96b8 161 162--- 163 gio/gdbusconnection.c | 14 +++++++------- 164 1 file changed, 7 insertions(+), 7 deletions(-) 165 166diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c 167index 6a0d67a8ee..0cbfc66c13 100644 168--- a/gio/gdbusconnection.c 169+++ b/gio/gdbusconnection.c 170@@ -3743,7 +3743,7 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection, 171 typedef struct 172 { 173 SignalSubscriber *subscriber; /* (owned) */ 174- GDBusMessage *message; 175+ GDBusMessage *message; /* (owned) */ 176 GDBusConnection *connection; 177 const gchar *sender; /* (nullable) for peer-to-peer connections */ 178 const gchar *path; 179@@ -3807,7 +3807,7 @@ emit_signal_instance_in_idle_cb (gpointer data) 180 static void 181 signal_instance_free (SignalInstance *signal_instance) 182 { 183- g_object_unref (signal_instance->message); 184+ g_clear_object (&signal_instance->message); 185 g_object_unref (signal_instance->connection); 186 signal_subscriber_unref (signal_instance->subscriber); 187 g_free (signal_instance); 188@@ -4219,7 +4219,7 @@ has_object_been_unregistered (GDBusConnection *connection, 189 typedef struct 190 { 191 GDBusConnection *connection; 192- GDBusMessage *message; 193+ GDBusMessage *message; /* (owned) */ 194 gpointer user_data; 195 const gchar *property_name; 196 const GDBusInterfaceVTable *vtable; 197@@ -4233,7 +4233,7 @@ static void 198 property_data_free (PropertyData *data) 199 { 200 g_object_unref (data->connection); 201- g_object_unref (data->message); 202+ g_clear_object (&data->message); 203 g_free (data); 204 } 205 206@@ -4575,7 +4575,7 @@ handle_getset_property (GDBusConnection *connection, 207 typedef struct 208 { 209 GDBusConnection *connection; 210- GDBusMessage *message; 211+ GDBusMessage *message; /* (owned) */ 212 gpointer user_data; 213 const GDBusInterfaceVTable *vtable; 214 GDBusInterfaceInfo *interface_info; 215@@ -4587,7 +4587,7 @@ static void 216 property_get_all_data_free (PropertyGetAllData *data) 217 { 218 g_object_unref (data->connection); 219- g_object_unref (data->message); 220+ g_clear_object (&data->message); 221 g_free (data); 222 } 223 224@@ -6815,7 +6815,7 @@ typedef struct 225 static void 226 subtree_deferred_data_free (SubtreeDeferredData *data) 227 { 228- g_object_unref (data->message); 229+ g_clear_object (&data->message); 230 exported_subtree_unref (data->es); 231 g_free (data); 232 } 233-- 234GitLab 235 236 237From ed7044b5f383cf8b77df751578e184d4ad7a134f Mon Sep 17 00:00:00 2001 238From: Philip Withnall <pwithnall@endlessos.org> 239Date: Wed, 22 Feb 2023 02:49:29 +0000 240Subject: [PATCH 4/9] gdbusprivate: Improve docs on message ownership in 241 MessageToWriteData 242MIME-Version: 1.0 243Content-Type: text/plain; charset=UTF-8 244Content-Transfer-Encoding: 8bit 245 246This doesn’t introduce any functional changes, but should make the code 247a little clearer. 248 249Drive-by improvements while trying to debug #1264. 250 251Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 252 253Helps: #1264 254 255Conflict:NA 256Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/ed7044b5f383cf8b77df751578e184d4ad7a134f 257 258--- 259 gio/gdbusprivate.c | 5 ++--- 260 1 file changed, 2 insertions(+), 3 deletions(-) 261 262diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c 263index 762afcee46..bd776a4214 100644 264--- a/gio/gdbusprivate.c 265+++ b/gio/gdbusprivate.c 266@@ -889,7 +889,7 @@ _g_dbus_worker_do_initial_read (gpointer data) 267 struct _MessageToWriteData 268 { 269 GDBusWorker *worker; 270- GDBusMessage *message; 271+ GDBusMessage *message; /* (owned) */ 272 gchar *blob; 273 gsize blob_size; 274 275@@ -901,8 +901,7 @@ static void 276 message_to_write_data_free (MessageToWriteData *data) 277 { 278 _g_dbus_worker_unref (data->worker); 279- if (data->message) 280- g_object_unref (data->message); 281+ g_clear_object (&data->message); 282 g_free (data->blob); 283 g_slice_free (MessageToWriteData, data); 284 } 285-- 286GitLab 287 288 289From 861741ef4bff1a3ee15175e189136563b74fe790 Mon Sep 17 00:00:00 2001 290From: Philip Withnall <pwithnall@endlessos.org> 291Date: Wed, 22 Feb 2023 02:50:47 +0000 292Subject: [PATCH 5/9] gdbusprivate: Ensure data->task is cleared when it 293 returns 294MIME-Version: 1.0 295Content-Type: text/plain; charset=UTF-8 296Content-Transfer-Encoding: 8bit 297 298The existing comment in the code was correct that `data` is freed when 299the task callback is called, because `data` is also pointed to by the 300`user_data` for the task, and that’s freed at the end of the callback. 301 302So the existing code was correct to take a copy of `data->task` before 303calling `g_task_return_*()`. 304 305After calling `g_task_return_*()`, the existing code unreffed the task 306(which is correct), but then didn’t clear the `data->task` pointer, 307leaving `data->task` dangling. That could cause a use-after-free or a 308double-unref. 309 310Avoid that risk by explicitly clearing `data->task` before calling 311`g_task_return_*()`. 312 313After some testing, it turns out this doesn’t actually fix any bugs, but 314it’s still a good robustness improvement. 315 316Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 317 318Helps: #1264 319 320Conflict:NA 321Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/861741ef4bff1a3ee15175e189136563b74fe790 322 323--- 324 gio/gdbusprivate.c | 54 ++++++++++++++++++++++++++++------------------ 325 1 file changed, 33 insertions(+), 21 deletions(-) 326 327diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c 328index bd776a4214..0b4806f524 100644 329--- a/gio/gdbusprivate.c 330+++ b/gio/gdbusprivate.c 331@@ -894,7 +894,7 @@ struct _MessageToWriteData 332 gsize blob_size; 333 334 gsize total_written; 335- GTask *task; 336+ GTask *task; /* (owned) and (nullable) before writing starts and after g_task_return_*() is called */ 337 }; 338 339 static void 340@@ -903,6 +903,11 @@ message_to_write_data_free (MessageToWriteData *data) 341 _g_dbus_worker_unref (data->worker); 342 g_clear_object (&data->message); 343 g_free (data->blob); 344+ 345+ /* The task must either not have been created, or have been created, returned 346+ * and finalised by now. */ 347+ g_assert (data->task == NULL); 348+ 349 g_slice_free (MessageToWriteData, data); 350 } 351 352@@ -921,14 +926,14 @@ write_message_async_cb (GObject *source_object, 353 gpointer user_data) 354 { 355 MessageToWriteData *data = user_data; 356- GTask *task; 357 gssize bytes_written; 358 GError *error; 359 360- /* Note: we can't access data->task after calling g_task_return_* () because the 361- * callback can free @data and we're not completing in idle. So use a copy of the pointer. 362- */ 363- task = data->task; 364+ /* The ownership of @data is a bit odd in this function: it’s (transfer full) 365+ * when the function is called, but the code paths which call g_task_return_*() 366+ * on @data->task will indirectly cause it to be freed, because @data is 367+ * always guaranteed to be the user_data in the #GTask. So that’s why it looks 368+ * like @data is not always freed on every code path in this function. */ 369 370 error = NULL; 371 bytes_written = g_output_stream_write_finish (G_OUTPUT_STREAM (source_object), 372@@ -936,8 +941,9 @@ write_message_async_cb (GObject *source_object, 373 &error); 374 if (bytes_written == -1) 375 { 376+ GTask *task = g_steal_pointer (&data->task); 377 g_task_return_error (task, error); 378- g_object_unref (task); 379+ g_clear_object (&task); 380 goto out; 381 } 382 g_assert (bytes_written > 0); /* zero is never returned */ 383@@ -948,8 +954,9 @@ write_message_async_cb (GObject *source_object, 384 g_assert (data->total_written <= data->blob_size); 385 if (data->total_written == data->blob_size) 386 { 387+ GTask *task = g_steal_pointer (&data->task); 388 g_task_return_boolean (task, TRUE); 389- g_object_unref (task); 390+ g_clear_object (&task); 391 goto out; 392 } 393 394@@ -986,16 +993,14 @@ write_message_continue_writing (MessageToWriteData *data) 395 { 396 GOutputStream *ostream; 397 #ifdef G_OS_UNIX 398- GTask *task; 399 GUnixFDList *fd_list; 400 #endif 401 402-#ifdef G_OS_UNIX 403- /* Note: we can't access data->task after calling g_task_return_* () because the 404- * callback can free @data and we're not completing in idle. So use a copy of the pointer. 405- */ 406- task = data->task; 407-#endif 408+ /* The ownership of @data is a bit odd in this function: it’s (transfer full) 409+ * when the function is called, but the code paths which call g_task_return_*() 410+ * on @data->task will indirectly cause it to be freed, because @data is 411+ * always guaranteed to be the user_data in the #GTask. So that’s why it looks 412+ * like @data is not always freed on every code path in this function. */ 413 414 ostream = g_io_stream_get_output_stream (data->worker->stream); 415 #ifdef G_OS_UNIX 416@@ -1024,11 +1029,12 @@ write_message_continue_writing (MessageToWriteData *data) 417 { 418 if (!(data->worker->capabilities & G_DBUS_CAPABILITY_FLAGS_UNIX_FD_PASSING)) 419 { 420+ GTask *task = g_steal_pointer (&data->task); 421 g_task_return_new_error (task, 422 G_IO_ERROR, 423 G_IO_ERROR_FAILED, 424 "Tried sending a file descriptor but remote peer does not support this capability"); 425- g_object_unref (task); 426+ g_clear_object (&task); 427 goto out; 428 } 429 control_message = g_unix_fd_message_new_with_fd_list (fd_list); 430@@ -1065,9 +1071,13 @@ write_message_continue_writing (MessageToWriteData *data) 431 g_error_free (error); 432 goto out; 433 } 434- g_task_return_error (task, error); 435- g_object_unref (task); 436- goto out; 437+ else 438+ { 439+ GTask *task = g_steal_pointer (&data->task); 440+ g_task_return_error (task, error); 441+ g_clear_object (&task); 442+ goto out; 443+ } 444 } 445 g_assert (bytes_written > 0); /* zero is never returned */ 446 447@@ -1077,8 +1087,9 @@ write_message_continue_writing (MessageToWriteData *data) 448 g_assert (data->total_written <= data->blob_size); 449 if (data->total_written == data->blob_size) 450 { 451+ GTask *task = g_steal_pointer (&data->task); 452 g_task_return_boolean (task, TRUE); 453- g_object_unref (task); 454+ g_clear_object (&task); 455 goto out; 456 } 457 458@@ -1093,12 +1104,13 @@ write_message_continue_writing (MessageToWriteData *data) 459 /* We were trying to write byte 0 of the message, which needs 460 * the fd list to be attached to it, but this connection doesn't 461 * support doing that. */ 462+ GTask *task = g_steal_pointer (&data->task); 463 g_task_return_new_error (task, 464 G_IO_ERROR, 465 G_IO_ERROR_FAILED, 466 "Tried sending a file descriptor on unsupported stream of type %s", 467 g_type_name (G_TYPE_FROM_INSTANCE (ostream))); 468- g_object_unref (task); 469+ g_clear_object (&task); 470 goto out; 471 } 472 #endif 473-- 474GitLab 475 476 477From d7c813cf5b6148c18184e4f1af23d234e73aafb8 Mon Sep 17 00:00:00 2001 478From: Philip Withnall <pwithnall@endlessos.org> 479Date: Wed, 22 Feb 2023 02:56:56 +0000 480Subject: [PATCH 6/9] gdbusprivate: Improve ownership docs for 481 write_message_async() 482MIME-Version: 1.0 483Content-Type: text/plain; charset=UTF-8 484Content-Transfer-Encoding: 8bit 485 486The ownership transfers in this code are a bit complex, so adding some 487extra documentation and `g_steal_pointer()` calls should hopefully help 488clarify things. 489 490This doesn’t introduce any functional changes, just code documentation. 491 492Another drive-by improvement in the quest for #1264. 493 494Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 495 496Helps: #1264 497 498Conflict:NA 499Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/d7c813cf5b6148c18184e4f1af23d234e73aafb8 500 501--- 502 gio/gdbusprivate.c | 21 ++++++++++++--------- 503 1 file changed, 12 insertions(+), 9 deletions(-) 504 505diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c 506index 0b4806f524..5aa141a60e 100644 507--- a/gio/gdbusprivate.c 508+++ b/gio/gdbusprivate.c 509@@ -919,13 +919,14 @@ static void write_message_continue_writing (MessageToWriteData *data); 510 * 511 * write-lock is not held on entry 512 * output_pending is PENDING_WRITE on entry 513+ * @user_data is (transfer full) 514 */ 515 static void 516 write_message_async_cb (GObject *source_object, 517 GAsyncResult *res, 518 gpointer user_data) 519 { 520- MessageToWriteData *data = user_data; 521+ MessageToWriteData *data = g_steal_pointer (&user_data); 522 gssize bytes_written; 523 GError *error; 524 525@@ -960,7 +961,7 @@ write_message_async_cb (GObject *source_object, 526 goto out; 527 } 528 529- write_message_continue_writing (data); 530+ write_message_continue_writing (g_steal_pointer (&data)); 531 532 out: 533 ; 534@@ -977,8 +978,8 @@ on_socket_ready (GSocket *socket, 535 GIOCondition condition, 536 gpointer user_data) 537 { 538- MessageToWriteData *data = user_data; 539- write_message_continue_writing (data); 540+ MessageToWriteData *data = g_steal_pointer (&user_data); 541+ write_message_continue_writing (g_steal_pointer (&data)); 542 return FALSE; /* remove source */ 543 } 544 #endif 545@@ -987,6 +988,7 @@ on_socket_ready (GSocket *socket, 546 * 547 * write-lock is not held on entry 548 * output_pending is PENDING_WRITE on entry 549+ * @data is (transfer full) 550 */ 551 static void 552 write_message_continue_writing (MessageToWriteData *data) 553@@ -1064,7 +1066,7 @@ write_message_continue_writing (MessageToWriteData *data) 554 data->worker->cancellable); 555 g_source_set_callback (source, 556 (GSourceFunc) on_socket_ready, 557- data, 558+ g_steal_pointer (&data), 559 NULL); /* GDestroyNotify */ 560 g_source_attach (source, g_main_context_get_thread_default ()); 561 g_source_unref (source); 562@@ -1093,7 +1095,7 @@ write_message_continue_writing (MessageToWriteData *data) 563 goto out; 564 } 565 566- write_message_continue_writing (data); 567+ write_message_continue_writing (g_steal_pointer (&data)); 568 } 569 #endif 570 else 571@@ -1121,7 +1123,7 @@ write_message_continue_writing (MessageToWriteData *data) 572 G_PRIORITY_DEFAULT, 573 data->worker->cancellable, 574 write_message_async_cb, 575- data); 576+ data); /* steal @data */ 577 } 578 #ifdef G_OS_UNIX 579 out: 580@@ -1144,7 +1146,7 @@ write_message_async (GDBusWorker *worker, 581 g_task_set_source_tag (data->task, write_message_async); 582 g_task_set_name (data->task, "[gio] D-Bus write message"); 583 data->total_written = 0; 584- write_message_continue_writing (data); 585+ write_message_continue_writing (g_steal_pointer (&data)); 586 } 587 588 /* called in private thread shared by all GDBusConnection instances (with write-lock held) */ 589@@ -1333,6 +1335,7 @@ prepare_flush_unlocked (GDBusWorker *worker) 590 * 591 * write-lock is not held on entry 592 * output_pending is PENDING_WRITE on entry 593+ * @user_data is (transfer full) 594 */ 595 static void 596 write_message_cb (GObject *source_object, 597@@ -1551,7 +1554,7 @@ continue_writing (GDBusWorker *worker) 598 write_message_async (worker, 599 data, 600 write_message_cb, 601- data); 602+ data); /* takes ownership of @data as user_data */ 603 } 604 } 605 606-- 607GitLab 608 609 610From 08a4387678346caaa42b69e5e6e5995d48cd61c4 Mon Sep 17 00:00:00 2001 611From: Philip Withnall <pwithnall@endlessos.org> 612Date: Wed, 22 Feb 2023 02:58:05 +0000 613Subject: [PATCH 7/9] gdbusprivate: Use G_SOURCE_REMOVE in a source callback 614 615This is equivalent to the current behaviour, but a little clearer in its 616meaning. 617 618Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 619 620Helps: #1264 621 622Conflict:NA 623Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/08a4387678346caaa42b69e5e6e5995d48cd61c4 624 625--- 626 gio/gdbusprivate.c | 2 +- 627 1 file changed, 1 insertion(+), 1 deletion(-) 628 629diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c 630index 5aa141a60e..2c9238c638 100644 631--- a/gio/gdbusprivate.c 632+++ b/gio/gdbusprivate.c 633@@ -980,7 +980,7 @@ on_socket_ready (GSocket *socket, 634 { 635 MessageToWriteData *data = g_steal_pointer (&user_data); 636 write_message_continue_writing (g_steal_pointer (&data)); 637- return FALSE; /* remove source */ 638+ return G_SOURCE_REMOVE; 639 } 640 #endif 641 642-- 643GitLab 644 645 646From b84ec21f9c4c57990309e691206582c589f59770 Mon Sep 17 00:00:00 2001 647From: Philip Withnall <pwithnall@endlessos.org> 648Date: Wed, 22 Feb 2023 12:19:16 +0000 649Subject: [PATCH 8/9] gdbusconnection: Rearrange refcount handling of 650 map_method_serial_to_task 651MIME-Version: 1.0 652Content-Type: text/plain; charset=UTF-8 653Content-Transfer-Encoding: 8bit 654 655It already implicitly held a strong ref on its `GTask` values, but 656didn’t have a free function set so that they would be automatically 657unreffed on removal from the map. 658 659This meant that the functions handling removals from the map, 660`on_worker_closed()` (via `cancel_method_on_close()`) and 661`send_message_with_reply_cleanup()` had to call unref once more than 662they would otherwise. 663 664In `send_message_with_reply_cleanup()`, this behaviour depended on 665whether it was called with `remove == TRUE`. If not, it was `(transfer 666none)` not `(transfer full)`. This led to bugs in its callers. 667 668For example, this led to a direct leak in `cancel_method_on_close()`, as 669it needed to remove tasks from `map_method_serial_to_task`, but called 670`send_message_with_reply_cleanup(remove = FALSE)` and erroneously didn’t 671call unref an additional time. 672 673Try and simplify it all by setting a `GDestroyNotify` on 674`map_method_serial_to_task`’s values, and making the refcount handling 675of `send_message_with_reply_cleanup()` not be conditional on its 676arguments. 677 678Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 679 680Helps: #1264 681 682Conflict:NA 683Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/b84ec21f9c4c57990309e691206582c589f59770 684 685--- 686 gio/gdbusconnection.c | 24 ++++++++++++------------ 687 1 file changed, 12 insertions(+), 12 deletions(-) 688 689diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c 690index 0cbfc66c13..f4bc21bb37 100644 691--- a/gio/gdbusconnection.c 692+++ b/gio/gdbusconnection.c 693@@ -409,7 +409,7 @@ struct _GDBusConnection 694 GDBusConnectionFlags flags; 695 696 /* Map used for managing method replies, protected by @lock */ 697- GHashTable *map_method_serial_to_task; /* guint32 -> GTask* */ 698+ GHashTable *map_method_serial_to_task; /* guint32 -> owned GTask* */ 699 700 /* Maps used for managing signal subscription, protected by @lock */ 701 GHashTable *map_rule_to_signal_data; /* match rule (gchar*) -> SignalData */ 702@@ -1061,7 +1061,7 @@ g_dbus_connection_init (GDBusConnection *connection) 703 g_mutex_init (&connection->lock); 704 g_mutex_init (&connection->init_lock); 705 706- connection->map_method_serial_to_task = g_hash_table_new (g_direct_hash, g_direct_equal); 707+ connection->map_method_serial_to_task = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref); 708 709 connection->map_rule_to_signal_data = g_hash_table_new (g_str_hash, 710 g_str_equal); 711@@ -1768,7 +1768,7 @@ send_message_data_free (SendMessageData *data) 712 713 /* ---------------------------------------------------------------------------------------------------- */ 714 715-/* can be called from any thread with lock held; @task is (transfer full) */ 716+/* can be called from any thread with lock held; @task is (transfer none) */ 717 static void 718 send_message_with_reply_cleanup (GTask *task, gboolean remove) 719 { 720@@ -1798,13 +1798,11 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove) 721 GUINT_TO_POINTER (data->serial)); 722 g_warn_if_fail (removed); 723 } 724- 725- g_object_unref (task); 726 } 727 728 /* ---------------------------------------------------------------------------------------------------- */ 729 730-/* Called from GDBus worker thread with lock held; @task is (transfer full). */ 731+/* Called from GDBus worker thread with lock held; @task is (transfer none). */ 732 static void 733 send_message_data_deliver_reply_unlocked (GTask *task, 734 GDBusMessage *reply) 735@@ -1822,7 +1820,7 @@ send_message_data_deliver_reply_unlocked (GTask *task, 736 ; 737 } 738 739-/* Called from a user thread, lock is not held; @task is (transfer full) */ 740+/* Called from a user thread, lock is not held; @task is (transfer none) */ 741 static void 742 send_message_data_deliver_error (GTask *task, 743 GQuark domain, 744@@ -1839,7 +1837,10 @@ send_message_data_deliver_error (GTask *task, 745 return; 746 } 747 748+ /* Hold a ref on @task as send_message_with_reply_cleanup() will remove it 749+ * from the task map and could end up dropping the last reference */ 750 g_object_ref (task); 751+ 752 send_message_with_reply_cleanup (task, TRUE); 753 CONNECTION_UNLOCK (connection); 754 755@@ -1855,8 +1856,7 @@ send_message_with_reply_cancelled_idle_cb (gpointer user_data) 756 { 757 GTask *task = user_data; 758 759- g_object_ref (task); 760- send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_CANCELLED, 761+ send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_CANCELLED, 762 _("Operation was cancelled")); 763 return FALSE; 764 } 765@@ -1886,8 +1886,7 @@ send_message_with_reply_timeout_cb (gpointer user_data) 766 { 767 GTask *task = user_data; 768 769- g_object_ref (task); 770- send_message_data_deliver_error (g_steal_pointer (&task), G_IO_ERROR, G_IO_ERROR_TIMED_OUT, 771+ send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT, 772 _("Timeout was reached")); 773 return FALSE; 774 } 775@@ -2391,7 +2390,8 @@ on_worker_message_about_to_be_sent (GDBusWorker *worker, 776 return message; 777 } 778 779-/* called with connection lock held, in GDBusWorker thread */ 780+/* called with connection lock held, in GDBusWorker thread 781+ * @key, @value and @user_data are (transfer none) */ 782 static gboolean 783 cancel_method_on_close (gpointer key, gpointer value, gpointer user_data) 784 { 785-- 786GitLab 787 788 789From 0a84c182e28f50be2263e42e0bc21074dd523701 Mon Sep 17 00:00:00 2001 790From: Philip Withnall <pwithnall@endlessos.org> 791Date: Wed, 22 Feb 2023 14:55:40 +0000 792Subject: [PATCH 9/9] gdbusconnection: Improve refcount handling of timeout 793 source 794MIME-Version: 1.0 795Content-Type: text/plain; charset=UTF-8 796Content-Transfer-Encoding: 8bit 797 798The ref on the timeout source owned by `SendMessageData` was being 799dropped just after attaching the source to the main context, leaving it 800unowned in that struct. That meant the only ref on the source was held 801by the `GMainContext` it was attached to. 802 803This ref was dropped when returning `G_SOURCE_REMOVE` from 804`send_message_with_reply_timeout_cb()`. Before that happens, 805`send_message_data_deliver_error()` is called, which normally calls 806`send_message_with_reply_cleanup()` and destroys the source. 807 808However, if `send_message_data_deliver_error()` is called when the 809message has already been delivered, calling 810`send_message_with_reply_cleanup()` will be skipped. This leaves the 811source pointer in `SendMessageData` dangling, which will cause problems 812when `g_source_destroy()` is subsequently called on it. 813 814I’m not sure if it’s possible in practice for this situation to occur, 815but the code certainly does nothing to prevent it, and it’s easy enough 816to avoid by keeping a strong ref on the source in `SendMessageData`. 817 818Signed-off-by: Philip Withnall <pwithnall@endlessos.org> 819 820Helps: #1264 821 822Conflict:NA 823Reference:https://gitlab.gnome.org/GNOME/glib/-/commit/0a84c182e28f50be2263e42e0bc21074dd523701 824 825--- 826 gio/gdbusconnection.c | 8 ++++---- 827 1 file changed, 4 insertions(+), 4 deletions(-) 828 829diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c 830index f4bc21bb37..bed1ff2841 100644 831--- a/gio/gdbusconnection.c 832+++ b/gio/gdbusconnection.c 833@@ -1751,7 +1751,7 @@ typedef struct 834 835 gulong cancellable_handler_id; 836 837- GSource *timeout_source; 838+ GSource *timeout_source; /* (owned) (nullable) */ 839 840 gboolean delivered; 841 } SendMessageData; 842@@ -1760,6 +1760,7 @@ typedef struct 843 static void 844 send_message_data_free (SendMessageData *data) 845 { 846+ /* These should already have been cleared by send_message_with_reply_cleanup(). */ 847 g_assert (data->timeout_source == NULL); 848 g_assert (data->cancellable_handler_id == 0); 849 850@@ -1784,7 +1785,7 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove) 851 if (data->timeout_source != NULL) 852 { 853 g_source_destroy (data->timeout_source); 854- data->timeout_source = NULL; 855+ g_clear_pointer (&data->timeout_source, g_source_unref); 856 } 857 if (data->cancellable_handler_id > 0) 858 { 859@@ -1888,7 +1889,7 @@ send_message_with_reply_timeout_cb (gpointer user_data) 860 861 send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT, 862 _("Timeout was reached")); 863- return FALSE; 864+ return G_SOURCE_REMOVE; 865 } 866 867 /* ---------------------------------------------------------------------------------------------------- */ 868@@ -1949,7 +1950,6 @@ g_dbus_connection_send_message_with_reply_unlocked (GDBusConnection *connect 869 data->timeout_source = g_timeout_source_new (timeout_msec); 870 g_task_attach_source (task, data->timeout_source, 871 (GSourceFunc) send_message_with_reply_timeout_cb); 872- g_source_unref (data->timeout_source); 873 } 874 875 g_hash_table_insert (connection->map_method_serial_to_task, 876-- 877GitLab