From d553d92d6e9f53cbe5a34166fcb919ba652c6a8e Mon Sep 17 00:00:00 2001 From: Patrick Griffis Date: Tue, 29 Jan 2019 10:07:06 -0500 Subject: [PATCH] gsocketclient: Fix criticals This ensures the parent GTask is kept alive as long as an enumeration is running and trying to connect. Closes #1646 Closes #1649 Upstream-Status: Backport CVE: CVE-2019-9633 patch 2 Affects: < 2.59.2 Signed-off-by: Armin Kuster --- gio/gsocketclient.c | 74 +++++++++++++++++++++++++++++------------- gio/tests/gsocketclient-slow.c | 55 ++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 23 deletions(-) Index: glib-2.58.0/gio/gsocketclient.c =================================================================== --- glib-2.58.0.orig/gio/gsocketclient.c +++ glib-2.58.0/gio/gsocketclient.c @@ -1327,7 +1327,7 @@ g_socket_client_connect_to_uri (GSocketC typedef struct { - GTask *task; + GTask *task; /* unowned */ GSocketClient *client; GSocketConnectable *connectable; @@ -1345,6 +1345,7 @@ static void connection_attempt_unref (gp static void g_socket_client_async_connect_data_free (GSocketClientAsyncConnectData *data) { + data->task = NULL; g_clear_object (&data->connectable); g_clear_object (&data->enumerator); g_clear_object (&data->proxy_addr); @@ -1444,13 +1445,19 @@ set_last_error (GSocketClientAsyncConnec } static void -enumerator_next_async (GSocketClientAsyncConnectData *data) +enumerator_next_async (GSocketClientAsyncConnectData *data, + gboolean add_task_ref) { /* We need to cleanup the state */ g_clear_object (&data->socket); g_clear_object (&data->proxy_addr); g_clear_object (&data->connection); + /* Each enumeration takes a ref. This arg just avoids repeated unrefs when + an enumeration starts another enumeration */ + if (add_task_ref) + g_object_ref (data->task); + g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVING, data->connectable, NULL); g_socket_address_enumerator_next_async (data->enumerator, g_task_get_cancellable (data->task), @@ -1478,7 +1485,7 @@ g_socket_client_tls_handshake_callback ( else { g_object_unref (object); - enumerator_next_async (data); + enumerator_next_async (data, FALSE); } } @@ -1509,7 +1516,7 @@ g_socket_client_tls_handshake (GSocketCl } else { - enumerator_next_async (data); + enumerator_next_async (data, FALSE); } } @@ -1530,13 +1537,24 @@ g_socket_client_proxy_connect_callback ( } else { - enumerator_next_async (data); + enumerator_next_async (data, FALSE); return; } g_socket_client_tls_handshake (data); } +static gboolean +task_completed_or_cancelled (GTask *task) +{ + if (g_task_get_completed (task)) + return TRUE; + else if (g_task_return_error_if_cancelled (task)) + return TRUE; + else + return FALSE; +} + static void g_socket_client_connected_callback (GObject *source, GAsyncResult *result, @@ -1549,8 +1567,7 @@ g_socket_client_connected_callback (GObj GProxy *proxy; const gchar *protocol; - /* data is NULL once the task is completed */ - if (data && g_task_return_error_if_cancelled (data->task)) + if (g_cancellable_is_cancelled (attempt->cancellable) || task_completed_or_cancelled (data->task)) { g_object_unref (data->task); connection_attempt_unref (attempt); @@ -1570,17 +1587,15 @@ g_socket_client_connected_callback (GObj { clarify_connect_error (error, data->connectable, attempt->address); set_last_error (data, error); + connection_attempt_remove (attempt); + enumerator_next_async (data, FALSE); } else - g_clear_error (&error); - - if (data) { - connection_attempt_remove (attempt); - enumerator_next_async (data); + g_clear_error (&error); + g_object_unref (data->task); + connection_attempt_unref (attempt); } - else - connection_attempt_unref (attempt); return; } @@ -1592,7 +1607,6 @@ g_socket_client_connected_callback (GObj { ConnectionAttempt *attempt_entry = l->data; g_cancellable_cancel (attempt_entry->cancellable); - attempt_entry->data = NULL; connection_attempt_unref (attempt_entry); } g_slist_free (data->connection_attempts); @@ -1625,7 +1639,7 @@ g_socket_client_connected_callback (GObj G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED, _("Proxying over a non-TCP connection is not supported.")); - enumerator_next_async (data); + enumerator_next_async (data, FALSE); } else if (g_hash_table_contains (data->client->priv->app_proxies, protocol)) { @@ -1652,7 +1666,7 @@ g_socket_client_connected_callback (GObj _("Proxy protocol “%s” is not supported."), protocol); - enumerator_next_async (data); + enumerator_next_async (data, FALSE); } } @@ -1661,7 +1675,7 @@ on_connection_attempt_timeout (gpointer { ConnectionAttempt *attempt = data; - enumerator_next_async (attempt->data); + enumerator_next_async (attempt->data, TRUE); g_clear_pointer (&attempt->timeout_source, g_source_unref); return G_SOURCE_REMOVE; @@ -1687,7 +1701,7 @@ g_socket_client_enumerator_callback (GOb ConnectionAttempt *attempt; GError *error = NULL; - if (g_task_return_error_if_cancelled (data->task)) + if (task_completed_or_cancelled (data->task)) { g_object_unref (data->task); return; @@ -1698,7 +1712,10 @@ g_socket_client_enumerator_callback (GOb if (address == NULL) { if (data->connection_attempts) - return; + { + g_object_unref (data->task); + return; + } g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, NULL); if (!error) @@ -1732,7 +1749,7 @@ g_socket_client_enumerator_callback (GOb if (socket == NULL) { g_object_unref (address); - enumerator_next_async (data); + enumerator_next_async (data, FALSE); return; } @@ -1804,11 +1821,24 @@ g_socket_client_connect_async (GSocketCl else data->enumerator = g_socket_connectable_enumerate (connectable); + /* The flow and ownership here isn't quite obvious: + - The task starts an async attempt to connect. + - Each attempt holds a single ref on task. + - Each attempt may create new attempts by timing out (not a failure) so + there are multiple attempts happening in parallel. + - Upon failure an attempt will start a new attempt that steals its ref + until there are no more attempts left and it drops its ref. + - Upon success it will cancel all other attempts and continue on + to the rest of the connection (tls, proxies, etc) which do not + happen in parallel and at the very end drop its ref. + - Upon cancellation an attempt drops its ref. + */ + data->task = g_task_new (client, cancellable, callback, user_data); g_task_set_source_tag (data->task, g_socket_client_connect_async); g_task_set_task_data (data->task, data, (GDestroyNotify)g_socket_client_async_connect_data_free); - enumerator_next_async (data); + enumerator_next_async (data, FALSE); } /**