diff --git a/gio/gdbusconnection.c b/gio/gdbusconnection.c index ccca4e98c..65ece54b2 100644 --- a/gio/gdbusconnection.c +++ b/gio/gdbusconnection.c @@ -409,7 +409,7 @@ struct _GDBusConnection GDBusConnectionFlags flags; /* Map used for managing method replies, protected by @lock */ - GHashTable *map_method_serial_to_task; /* guint32 -> GTask* */ + GHashTable *map_method_serial_to_task; /* guint32 -> owned GTask* */ /* Maps used for managing signal subscription, protected by @lock */ GHashTable *map_rule_to_signal_data; /* match rule (gchar*) -> SignalData */ @@ -1061,7 +1061,7 @@ g_dbus_connection_init (GDBusConnection *connection) g_mutex_init (&connection->lock); g_mutex_init (&connection->init_lock); - connection->map_method_serial_to_task = g_hash_table_new (g_direct_hash, g_direct_equal); + connection->map_method_serial_to_task = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref); connection->map_rule_to_signal_data = g_hash_table_new (g_str_hash, g_str_equal); @@ -1752,7 +1752,7 @@ typedef struct gulong cancellable_handler_id; GSource *cancelled_idle_source; /* (owned) (nullable) */ - GSource *timeout_source; + GSource *timeout_source; /* (owned) (nullable) */ gboolean delivered; } SendMessageData; @@ -1761,6 +1761,7 @@ typedef struct static void send_message_data_free (SendMessageData *data) { + /* These should already have been cleared by send_message_with_reply_cleanup(). */ g_assert (data->timeout_source == NULL); g_assert (data->cancellable_handler_id == 0); @@ -1769,7 +1770,7 @@ send_message_data_free (SendMessageData *data) /* ---------------------------------------------------------------------------------------------------- */ -/* can be called from any thread with lock held; @task is (transfer full) */ +/* can be called from any thread with lock held; @task is (transfer none) */ static void send_message_with_reply_cleanup (GTask *task, gboolean remove) { @@ -1785,7 +1786,7 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove) if (data->timeout_source != NULL) { g_source_destroy (data->timeout_source); - data->timeout_source = NULL; + g_clear_pointer (&data->timeout_source, g_source_unref); } if (data->cancellable_handler_id > 0) { @@ -1804,13 +1805,11 @@ send_message_with_reply_cleanup (GTask *task, gboolean remove) GUINT_TO_POINTER (data->serial)); g_warn_if_fail (removed); } - - g_object_unref (task); } /* ---------------------------------------------------------------------------------------------------- */ -/* Called from GDBus worker thread with lock held; @task is (transfer full). */ +/* Called from GDBus worker thread with lock held; @task is (transfer none). */ static void send_message_data_deliver_reply_unlocked (GTask *task, GDBusMessage *reply) @@ -1828,7 +1827,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 none) */ static void send_message_data_deliver_error (GTask *task, GQuark domain, @@ -1845,7 +1844,10 @@ send_message_data_deliver_error (GTask *task, return; } + /* Hold a ref on @task as send_message_with_reply_cleanup() will remove it + * from the task map and could end up dropping the last reference */ g_object_ref (task); + send_message_with_reply_cleanup (task, TRUE); CONNECTION_UNLOCK (connection); @@ -1855,7 +1857,7 @@ 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) { @@ -1887,7 +1889,7 @@ 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) { @@ -1895,7 +1897,7 @@ send_message_with_reply_timeout_cb (gpointer user_data) send_message_data_deliver_error (task, G_IO_ERROR, G_IO_ERROR_TIMED_OUT, _("Timeout was reached")); - return FALSE; + return G_SOURCE_REMOVE; } /* ---------------------------------------------------------------------------------------------------- */ @@ -1956,7 +1958,6 @@ g_dbus_connection_send_message_with_reply_unlocked (GDBusConnection *connect g_source_set_static_name (data->timeout_source, "[gio] send_message_with_reply_unlocked"); g_task_attach_source (task, data->timeout_source, (GSourceFunc) send_message_with_reply_timeout_cb); - g_source_unref (data->timeout_source); } g_hash_table_insert (connection->map_method_serial_to_task, @@ -2397,7 +2398,8 @@ on_worker_message_about_to_be_sent (GDBusWorker *worker, return message; } -/* called with connection lock held, in GDBusWorker thread */ +/* called with connection lock held, in GDBusWorker thread + * @key, @value and @user_data are (transfer none) */ static gboolean cancel_method_on_close (gpointer key, gpointer value, gpointer user_data) { @@ -3749,7 +3751,7 @@ g_dbus_connection_signal_unsubscribe (GDBusConnection *connection, typedef struct { SignalSubscriber *subscriber; /* (owned) */ - GDBusMessage *message; + GDBusMessage *message; /* (owned) */ GDBusConnection *connection; const gchar *sender; /* (nullable) for peer-to-peer connections */ const gchar *path; @@ -3813,7 +3815,7 @@ emit_signal_instance_in_idle_cb (gpointer data) static void signal_instance_free (SignalInstance *signal_instance) { - g_object_unref (signal_instance->message); + g_clear_object (&signal_instance->message); g_object_unref (signal_instance->connection); signal_subscriber_unref (signal_instance->subscriber); g_free (signal_instance); @@ -4225,7 +4227,7 @@ has_object_been_unregistered (GDBusConnection *connection, typedef struct { GDBusConnection *connection; - GDBusMessage *message; + GDBusMessage *message; /* (owned) */ gpointer user_data; const gchar *property_name; const GDBusInterfaceVTable *vtable; @@ -4239,7 +4241,7 @@ static void property_data_free (PropertyData *data) { g_object_unref (data->connection); - g_object_unref (data->message); + g_clear_object (&data->message); g_free (data); } @@ -4581,7 +4583,7 @@ handle_getset_property (GDBusConnection *connection, typedef struct { GDBusConnection *connection; - GDBusMessage *message; + GDBusMessage *message; /* (owned) */ gpointer user_data; const GDBusInterfaceVTable *vtable; GDBusInterfaceInfo *interface_info; @@ -4590,10 +4592,10 @@ typedef struct } PropertyGetAllData; static void -property_get_all_data_free (PropertyData *data) +property_get_all_data_free (PropertyGetAllData *data) { g_object_unref (data->connection); - g_object_unref (data->message); + g_clear_object (&data->message); g_free (data); } @@ -6821,7 +6823,7 @@ typedef struct static void subtree_deferred_data_free (SubtreeDeferredData *data) { - g_object_unref (data->message); + g_clear_object (&data->message); exported_subtree_unref (data->es); g_free (data); } diff --git a/gio/gdbusprivate.c b/gio/gdbusprivate.c index 762afcee4..2c9238c63 100644 --- a/gio/gdbusprivate.c +++ b/gio/gdbusprivate.c @@ -889,21 +889,25 @@ _g_dbus_worker_do_initial_read (gpointer data) struct _MessageToWriteData { GDBusWorker *worker; - GDBusMessage *message; + GDBusMessage *message; /* (owned) */ gchar *blob; gsize blob_size; gsize total_written; - GTask *task; + GTask *task; /* (owned) and (nullable) before writing starts and after g_task_return_*() is called */ }; static void message_to_write_data_free (MessageToWriteData *data) { _g_dbus_worker_unref (data->worker); - if (data->message) - g_object_unref (data->message); + g_clear_object (&data->message); g_free (data->blob); + + /* The task must either not have been created, or have been created, returned + * and finalised by now. */ + g_assert (data->task == NULL); + g_slice_free (MessageToWriteData, data); } @@ -915,21 +919,22 @@ static void write_message_continue_writing (MessageToWriteData *data); * * write-lock is not held on entry * output_pending is PENDING_WRITE on entry + * @user_data is (transfer full) */ static void write_message_async_cb (GObject *source_object, GAsyncResult *res, gpointer user_data) { - MessageToWriteData *data = user_data; - GTask *task; + MessageToWriteData *data = g_steal_pointer (&user_data); gssize bytes_written; GError *error; - /* Note: we can't access data->task after calling g_task_return_* () because the - * callback can free @data and we're not completing in idle. So use a copy of the pointer. - */ - task = data->task; + /* The ownership of @data is a bit odd in this function: it’s (transfer full) + * when the function is called, but the code paths which call g_task_return_*() + * on @data->task will indirectly cause it to be freed, because @data is + * always guaranteed to be the user_data in the #GTask. So that’s why it looks + * like @data is not always freed on every code path in this function. */ error = NULL; bytes_written = g_output_stream_write_finish (G_OUTPUT_STREAM (source_object), @@ -937,8 +942,9 @@ write_message_async_cb (GObject *source_object, &error); if (bytes_written == -1) { + GTask *task = g_steal_pointer (&data->task); g_task_return_error (task, error); - g_object_unref (task); + g_clear_object (&task); goto out; } g_assert (bytes_written > 0); /* zero is never returned */ @@ -949,12 +955,13 @@ write_message_async_cb (GObject *source_object, g_assert (data->total_written <= data->blob_size); if (data->total_written == data->blob_size) { + GTask *task = g_steal_pointer (&data->task); g_task_return_boolean (task, TRUE); - g_object_unref (task); + g_clear_object (&task); goto out; } - write_message_continue_writing (data); + write_message_continue_writing (g_steal_pointer (&data)); out: ; @@ -971,9 +978,9 @@ on_socket_ready (GSocket *socket, GIOCondition condition, gpointer user_data) { - MessageToWriteData *data = user_data; - write_message_continue_writing (data); - return FALSE; /* remove source */ + MessageToWriteData *data = g_steal_pointer (&user_data); + write_message_continue_writing (g_steal_pointer (&data)); + return G_SOURCE_REMOVE; } #endif @@ -981,22 +988,21 @@ on_socket_ready (GSocket *socket, * * write-lock is not held on entry * output_pending is PENDING_WRITE on entry + * @data is (transfer full) */ static void write_message_continue_writing (MessageToWriteData *data) { GOutputStream *ostream; #ifdef G_OS_UNIX - GTask *task; GUnixFDList *fd_list; #endif -#ifdef G_OS_UNIX - /* Note: we can't access data->task after calling g_task_return_* () because the - * callback can free @data and we're not completing in idle. So use a copy of the pointer. - */ - task = data->task; -#endif + /* The ownership of @data is a bit odd in this function: it’s (transfer full) + * when the function is called, but the code paths which call g_task_return_*() + * on @data->task will indirectly cause it to be freed, because @data is + * always guaranteed to be the user_data in the #GTask. So that’s why it looks + * like @data is not always freed on every code path in this function. */ ostream = g_io_stream_get_output_stream (data->worker->stream); #ifdef G_OS_UNIX @@ -1025,11 +1031,12 @@ write_message_continue_writing (MessageToWriteData *data) { if (!(data->worker->capabilities & G_DBUS_CAPABILITY_FLAGS_UNIX_FD_PASSING)) { + GTask *task = g_steal_pointer (&data->task); g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, "Tried sending a file descriptor but remote peer does not support this capability"); - g_object_unref (task); + g_clear_object (&task); goto out; } control_message = g_unix_fd_message_new_with_fd_list (fd_list); @@ -1059,16 +1066,20 @@ write_message_continue_writing (MessageToWriteData *data) data->worker->cancellable); g_source_set_callback (source, (GSourceFunc) on_socket_ready, - data, + g_steal_pointer (&data), NULL); /* GDestroyNotify */ g_source_attach (source, g_main_context_get_thread_default ()); g_source_unref (source); g_error_free (error); goto out; } - g_task_return_error (task, error); - g_object_unref (task); - goto out; + else + { + GTask *task = g_steal_pointer (&data->task); + g_task_return_error (task, error); + g_clear_object (&task); + goto out; + } } g_assert (bytes_written > 0); /* zero is never returned */ @@ -1078,12 +1089,13 @@ write_message_continue_writing (MessageToWriteData *data) g_assert (data->total_written <= data->blob_size); if (data->total_written == data->blob_size) { + GTask *task = g_steal_pointer (&data->task); g_task_return_boolean (task, TRUE); - g_object_unref (task); + g_clear_object (&task); goto out; } - write_message_continue_writing (data); + write_message_continue_writing (g_steal_pointer (&data)); } #endif else @@ -1094,12 +1106,13 @@ write_message_continue_writing (MessageToWriteData *data) /* We were trying to write byte 0 of the message, which needs * the fd list to be attached to it, but this connection doesn't * support doing that. */ + GTask *task = g_steal_pointer (&data->task); g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, "Tried sending a file descriptor on unsupported stream of type %s", g_type_name (G_TYPE_FROM_INSTANCE (ostream))); - g_object_unref (task); + g_clear_object (&task); goto out; } #endif @@ -1110,7 +1123,7 @@ write_message_continue_writing (MessageToWriteData *data) G_PRIORITY_DEFAULT, data->worker->cancellable, write_message_async_cb, - data); + data); /* steal @data */ } #ifdef G_OS_UNIX out: @@ -1133,7 +1146,7 @@ write_message_async (GDBusWorker *worker, g_task_set_source_tag (data->task, write_message_async); g_task_set_name (data->task, "[gio] D-Bus write message"); data->total_written = 0; - write_message_continue_writing (data); + write_message_continue_writing (g_steal_pointer (&data)); } /* called in private thread shared by all GDBusConnection instances (with write-lock held) */ @@ -1322,6 +1335,7 @@ prepare_flush_unlocked (GDBusWorker *worker) * * write-lock is not held on entry * output_pending is PENDING_WRITE on entry + * @user_data is (transfer full) */ static void write_message_cb (GObject *source_object, @@ -1540,7 +1554,7 @@ continue_writing (GDBusWorker *worker) write_message_async (worker, data, write_message_cb, - data); + data); /* takes ownership of @data as user_data */ } }