[glib/wip/tingping/socket-client-data-races-fix] gsocketclient: Remove shared state from GSocketClientAsyncConnectData



commit 768eeb19eda0ae6d591683e65650381653fd3dfa
Author: Patrick Griffis <tingping tingping se>
Date:   Thu Jan 23 19:58:41 2020 -0800

    gsocketclient: Remove shared state from GSocketClientAsyncConnectData
    
    It is possible for multiple ConnectionAttempts to set data in the
    shared GSocketClientAsyncConnectData which can do terrible things
    like bypass a set proxy.
    
    This change keeps as much state as possible inside the
    ConnectionAttempt and keeps it alive until the very end of
    connecting.
    
    Note there are no new tests because it is quite tricky to simulate
    all of the conditions to hit these cases. That will have to be
    investigated in far more depth later.

 gio/gsocketclient.c | 138 ++++++++++++++++++++++++++++------------------------
 1 file changed, 75 insertions(+), 63 deletions(-)
---
diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c
index 6adeee299..4170e60cc 100644
--- a/gio/gsocketclient.c
+++ b/gio/gsocketclient.c
@@ -1337,9 +1337,6 @@ typedef struct
 
   GSocketConnectable *connectable;
   GSocketAddressEnumerator *enumerator;
-  GProxyAddress *proxy_addr;
-  GSocket *socket;
-  GIOStream *connection;
 
   GSList *connection_attempts;
   GError *last_error;
@@ -1355,9 +1352,6 @@ 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);
-  g_clear_object (&data->socket);
-  g_clear_object (&data->connection);
   g_slist_free_full (data->connection_attempts, connection_attempt_unref);
 
   g_clear_error (&data->last_error);
@@ -1370,6 +1364,7 @@ typedef struct
   GSocketAddress *address;
   GSocket *socket;
   GIOStream *connection;
+  GProxyAddress *proxy_addr;
   GSocketClientAsyncConnectData *data; /* unowned */
   GSource *timeout_source;
   GCancellable *cancellable;
@@ -1401,6 +1396,7 @@ connection_attempt_unref (gpointer pointer)
       g_clear_object (&attempt->socket);
       g_clear_object (&attempt->connection);
       g_clear_object (&attempt->cancellable);
+      g_clear_object (&attempt->proxy_addr);
       if (attempt->timeout_source)
         {
           g_source_destroy (attempt->timeout_source);
@@ -1418,17 +1414,18 @@ connection_attempt_remove (ConnectionAttempt *attempt)
 }
 
 static void
-g_socket_client_async_connect_complete (GSocketClientAsyncConnectData *data)
+g_socket_client_async_connect_complete (ConnectionAttempt *attempt)
 {
-  g_assert (data->connection);
+  GSocketClientAsyncConnectData *data = attempt->data;
+  g_assert (attempt->connection);
 
-  if (!G_IS_SOCKET_CONNECTION (data->connection))
+  if (!G_IS_SOCKET_CONNECTION (attempt->connection))
     {
       GSocketConnection *wrapper_connection;
 
-      wrapper_connection = g_tcp_wrapper_connection_new (data->connection, data->socket);
-      g_object_unref (data->connection);
-      data->connection = (GIOStream *)wrapper_connection;
+      wrapper_connection = g_tcp_wrapper_connection_new (attempt->connection, attempt->socket);
+      g_object_unref (attempt->connection);
+      attempt->connection = (GIOStream *)wrapper_connection;
     }
 
   if (!data->completed)
@@ -1442,13 +1439,14 @@ g_socket_client_async_connect_complete (GSocketClientAsyncConnectData *data)
         }
       else
         {
-          g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, 
data->connection);
-          g_task_return_pointer (data->task, g_steal_pointer (&data->connection), g_object_unref);
+          g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_COMPLETE, data->connectable, 
attempt->connection);
+          g_task_return_pointer (data->task, g_steal_pointer (&attempt->connection), g_object_unref);
         }
 
       data->completed = TRUE;
     }
 
+  connection_attempt_unref (attempt);
   g_object_unref (data->task);
 }
 
@@ -1470,11 +1468,6 @@ static void
 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)
@@ -1492,37 +1485,40 @@ g_socket_client_tls_handshake_callback (GObject      *object,
                                        GAsyncResult *result,
                                        gpointer      user_data)
 {
-  GSocketClientAsyncConnectData *data = user_data;
+  ConnectionAttempt *attempt = user_data;
+  GSocketClientAsyncConnectData *data = attempt->data;
 
   if (g_tls_connection_handshake_finish (G_TLS_CONNECTION (object),
                                         result,
                                         &data->last_error))
     {
-      g_object_unref (data->connection);
-      data->connection = G_IO_STREAM (object);
+      g_object_unref (attempt->connection);
+      attempt->connection = G_IO_STREAM (object);
 
-      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_TLS_HANDSHAKED, data->connectable, 
data->connection);
-      g_socket_client_async_connect_complete (data);
+      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_TLS_HANDSHAKED, data->connectable, 
attempt->connection);
+      g_socket_client_async_connect_complete (attempt);
     }
   else
     {
       g_object_unref (object);
+      connection_attempt_unref (attempt);
       enumerator_next_async (data, FALSE);
     }
 }
 
 static void
-g_socket_client_tls_handshake (GSocketClientAsyncConnectData *data)
+g_socket_client_tls_handshake (ConnectionAttempt *attempt)
 {
+  GSocketClientAsyncConnectData *data = attempt->data;
   GIOStream *tlsconn;
 
   if (!data->client->priv->tls)
     {
-      g_socket_client_async_connect_complete (data);
+      g_socket_client_async_connect_complete (attempt);
       return;
     }
 
-  tlsconn = g_tls_client_connection_new (data->connection,
+  tlsconn = g_tls_client_connection_new (attempt->connection,
                                         data->connectable,
                                         &data->last_error);
   if (tlsconn)
@@ -1534,10 +1530,11 @@ g_socket_client_tls_handshake (GSocketClientAsyncConnectData *data)
                                        G_PRIORITY_DEFAULT,
                                        g_task_get_cancellable (data->task),
                                        g_socket_client_tls_handshake_callback,
-                                       data);
+                                       attempt);
     }
   else
     {
+      connection_attempt_unref (attempt);
       enumerator_next_async (data, FALSE);
     }
 }
@@ -1547,23 +1544,25 @@ g_socket_client_proxy_connect_callback (GObject      *object,
                                        GAsyncResult *result,
                                        gpointer      user_data)
 {
-  GSocketClientAsyncConnectData *data = user_data;
+  ConnectionAttempt *attempt = user_data;
+  GSocketClientAsyncConnectData *data = attempt->data;
 
-  g_object_unref (data->connection);
-  data->connection = g_proxy_connect_finish (G_PROXY (object),
+  g_object_unref (attempt->connection);
+  attempt->connection = g_proxy_connect_finish (G_PROXY (object),
                                             result,
                                             &data->last_error);
-  if (data->connection)
+  if (attempt->connection)
     {
-      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATED, data->connectable, 
data->connection);
+      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATED, data->connectable, 
attempt->connection);
     }
   else
     {
+      connection_attempt_unref (attempt);
       enumerator_next_async (data, FALSE);
       return;
     }
 
-  g_socket_client_tls_handshake (data);
+  g_socket_client_tls_handshake (attempt);
 }
 
 static gboolean
@@ -1586,6 +1585,21 @@ task_completed_or_cancelled (GSocketClientAsyncConnectData *data)
     return FALSE;
 }
 
+static void
+cancel_all_attempts (GSocketClientAsyncConnectData *data)
+{
+  GSList *l;
+
+  for (l = data->connection_attempts; l; l = g_slist_next (l))
+    {
+      ConnectionAttempt *attempt_entry = l->data;
+      g_cancellable_cancel (attempt_entry->cancellable);
+      connection_attempt_unref (attempt_entry);
+    }
+  g_slist_free (data->connection_attempts);
+  data->connection_attempts = NULL;
+}
+
 static void
 g_socket_client_connected_callback (GObject      *source,
                                    GAsyncResult *result,
@@ -1593,7 +1607,6 @@ g_socket_client_connected_callback (GObject      *source,
 {
   ConnectionAttempt *attempt = user_data;
   GSocketClientAsyncConnectData *data = attempt->data;
-  GSList *l;
   GError *error = NULL;
   GProxy *proxy;
   const gchar *protocol;
@@ -1632,37 +1645,30 @@ g_socket_client_connected_callback (GObject      *source,
       return;
     }
 
-  data->socket = g_steal_pointer (&attempt->socket);
-  data->connection = g_steal_pointer (&attempt->connection);
+  /* Remove this successful (so far) attempt from list.
+     We may cancel all other attempts unless this one turns out fatal.
+     QUESTION: The fatal conditions currently are just unsupported connection or unsupported proxy, does 
this make sense?
+  */
+  connection_attempt_remove (attempt);
 
-  for (l = data->connection_attempts; l; l = g_slist_next (l))
-    {
-      ConnectionAttempt *attempt_entry = l->data;
-      g_cancellable_cancel (attempt_entry->cancellable);
-      connection_attempt_unref (attempt_entry);
-    }
-  g_slist_free (data->connection_attempts);
-  data->connection_attempts = NULL;
-  connection_attempt_unref (attempt);
-
-  g_socket_connection_set_cached_remote_address ((GSocketConnection*)data->connection, NULL);
-  g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_CONNECTED, data->connectable, data->connection);
+  g_socket_connection_set_cached_remote_address ((GSocketConnection*)attempt->connection, NULL);
+  g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_CONNECTED, data->connectable, 
attempt->connection);
 
   /* wrong, but backward compatible */
-  g_socket_set_blocking (data->socket, TRUE);
+  g_socket_set_blocking (attempt->socket, TRUE);
 
-  if (!data->proxy_addr)
+  if (!attempt->proxy_addr)
     {
-      g_socket_client_tls_handshake (data);
+      g_socket_client_tls_handshake (attempt);
       return;
     }
 
-  protocol = g_proxy_address_get_protocol (data->proxy_addr);
+  protocol = g_proxy_address_get_protocol (attempt->proxy_addr);
 
   /* The connection should not be anything other than TCP,
    * but let's put a safety guard in case
    */
-  if (!G_IS_TCP_CONNECTION (data->connection))
+  if (!G_IS_TCP_CONNECTION (attempt->connection))
     {
       g_critical ("Trying to proxy over non-TCP connection, this is "
           "most likely a bug in GLib IO library.");
@@ -1672,22 +1678,24 @@ g_socket_client_connected_callback (GObject      *source,
           _("Proxying over a non-TCP connection is not supported."));
 
       enumerator_next_async (data, FALSE);
+      connection_attempt_unref (attempt);
+      return;
     }
   else if (g_hash_table_contains (data->client->priv->app_proxies, protocol))
     {
       /* Simply complete the connection, we don't want to do TLS handshake
        * as the application proxy handling may need proxy handshake first */
-      g_socket_client_async_connect_complete (data);
+      g_socket_client_async_connect_complete (attempt);
     }
   else if ((proxy = g_proxy_get_default_for_protocol (protocol)))
     {
-      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATING, data->connectable, 
data->connection);
+      g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATING, data->connectable, 
attempt->connection);
       g_proxy_connect_async (proxy,
-                             data->connection,
-                             data->proxy_addr,
+                             attempt->connection,
+                             attempt->proxy_addr,
                              g_task_get_cancellable (data->task),
                              g_socket_client_proxy_connect_callback,
-                             data);
+                             attempt);
       g_object_unref (proxy);
     }
   else
@@ -1699,7 +1707,11 @@ g_socket_client_connected_callback (GObject      *source,
           protocol);
 
       enumerator_next_async (data, FALSE);
+      connection_attempt_unref (attempt);
+      return;
     }
+
+  cancel_all_attempts (data);
 }
 
 static gboolean
@@ -1772,10 +1784,6 @@ g_socket_client_enumerator_callback (GObject      *object,
   g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_RESOLVED,
                              data->connectable, NULL);
 
-  if (G_IS_PROXY_ADDRESS (address) &&
-      data->client->priv->enable_proxy)
-    data->proxy_addr = g_object_ref (G_PROXY_ADDRESS (address));
-
   g_clear_error (&data->last_error);
 
   socket = create_socket (data->client, address, &data->last_error);
@@ -1793,6 +1801,10 @@ g_socket_client_enumerator_callback (GObject      *object,
   attempt->cancellable = g_cancellable_new ();
   attempt->connection = (GIOStream *)g_socket_connection_factory_create_connection (socket);
   attempt->timeout_source = g_timeout_source_new (HAPPY_EYEBALLS_CONNECTION_ATTEMPT_TIMEOUT_MS);
+
+  if (G_IS_PROXY_ADDRESS (address) && data->client->priv->enable_proxy)
+    attempt->proxy_addr = g_object_ref (G_PROXY_ADDRESS (address));
+
   g_source_set_callback (attempt->timeout_source, on_connection_attempt_timeout, attempt, NULL);
   g_source_attach (attempt->timeout_source, g_main_context_get_thread_default ());
   data->connection_attempts = g_slist_append (data->connection_attempts, attempt);


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]