luc14n0/glib
1
0
Fork 0
mirror of https://gitlab.gnome.org/GNOME/glib.git synced 2024-12-25 23:16:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge branch '1264-gdbus-double-unref' into 'main'

Browse Source 
gdbusconnection: Fix double unref on timeout/cancel sending a message

Closes #1264

See merge request GNOME/glib!3291
This commit is contained in:
Philip Withnall 2023-04-14 14:57:32 +00:00
commit 56bc6bcad2
2 changed files with 72 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

46
gio/gdbusconnection.c
View File

@ -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);
}

78
gio/gdbusprivate.c
View File

@ -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,17 +1066,21 @@ 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;
}
else
{
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 */
write_message_print_transport_debug (bytes_written, data);
@ -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 */
}
}