[glib/mcatanzaro/#2211: 8/8] gsocketclient: return best errors possible




commit b88b3712e0d4474ff55d3b94050285ea08580ddb
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Thu Oct 8 18:02:56 2020 -0500

    gsocketclient: return best errors possible
    
    Originally, GSocketClient returned whatever error occured last. Turns
    out this doesn't work well in practice. Consider the following case:
    DNS returns an IPv4 and IPv6 address. First we'll connect() to the
    IPv4 address, and say that succeeds, but TLS is enabled and the TLS
    handshake fails. Then we try the IPv6 address and receive ENETUNREACH
    because IPv6 isn't supported. We wind up returning NETWORK_UNREACHABLE
    even though the address can be pinged and a TLS error would be more
    appropriate. So instead, we now try to return the error corresponding
    to the latest attempted GSocketClientEvent in the connection process.
    TLS errors take precedence over proxy errors, which take precedence
    over connect() errors, which take precedence over DNS errors.
    
    In writing this commit, I made several mistakes that were caught by
    proxy-test.c, which tests using GSocketClient to make a proxy
    connection. So although adding a new test to ensure we get the
    best-possible error would be awkward, at least we have some test
    coverage for the code that helped avoid introducing bugs.
    
    Fixes #2211

 gio/gsocketclient.c | 209 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 130 insertions(+), 79 deletions(-)
---
diff --git a/gio/gsocketclient.c b/gio/gsocketclient.c
index fb68c0966..ce3c186fb 100644
--- a/gio/gsocketclient.c
+++ b/gio/gsocketclient.c
@@ -953,6 +953,72 @@ g_socket_client_emit_event (GSocketClient       *client,
                 event, connectable, connection);
 }
 
+/* Originally, GSocketClient returned whatever error occured last. Turns
+ * out this doesn't work well in practice. Consider the following case:
+ * DNS returns an IPv4 and IPv6 address. First we'll connect() to the
+ * IPv4 address, and say that succeeds, but TLS is enabled and the TLS
+ * handshake fails. Then we try the IPv6 address and receive ENETUNREACH
+ * because IPv6 isn't supported. We wind up returning NETWORK_UNREACHABLE
+ * even though the address can be pinged and a TLS error would be more
+ * appropriate. So instead, we now try to return the error corresponding
+ * to the latest attempted GSocketClientEvent in the connection process.
+ * TLS errors take precedence over proxy errors, which take precedence
+ * over connect() errors, which take precedence over DNS errors.
+ *
+ * Note that the example above considers a sync codepath, but this is an
+ * issue for the async codepath too, where events and errors may occur
+ * in confusing orders.
+ */
+typedef struct
+{
+  GError *tmp_error;
+  GError *best_error;
+  GSocketClientEvent best_error_event;
+} SocketClientErrorInfo;
+
+static SocketClientErrorInfo *
+socket_client_error_info_new (void)
+{
+  return g_new0 (SocketClientErrorInfo, 1);
+}
+
+static void
+socket_client_error_info_free (SocketClientErrorInfo *info)
+{
+  g_assert (info->tmp_error == NULL);
+  g_clear_error (&info->best_error);
+  g_free (info);
+}
+
+static void
+consider_tmp_error (SocketClientErrorInfo *info,
+                    GSocketClientEvent     event)
+{
+  if (info->tmp_error == NULL)
+    return;
+
+  /* If we ever add more GSocketClientEvents in the future, then we'll
+   * no longer be able to use >= for this comparison, because future
+   * events will compare greater than G_SOCKET_CLIENT_COMPLETE. Until
+   * then, this is convenient. Note G_SOCKET_CLIENT_RESOLVING is 0 so we
+   * need to use >= here or those errors would never be set. That means
+   * if we get two errors on the same GSocketClientEvent, we wind up
+   * preferring the last one, which is fine.
+   */
+  g_assert (event <= G_SOCKET_CLIENT_COMPLETE);
+  if (event >= info->best_error_event)
+    {
+      g_clear_error (&info->best_error);
+      info->best_error = info->tmp_error;
+      info->tmp_error = NULL;
+      info->best_error_event = event;
+    }
+  else
+    {
+      g_clear_error (&info->tmp_error);
+    }
+}
+
 /**
  * g_socket_client_connect:
  * @client: a #GSocketClient.
@@ -991,10 +1057,10 @@ g_socket_client_connect (GSocketClient       *client,
 {
   GIOStream *connection = NULL;
   GSocketAddressEnumerator *enumerator = NULL;
+  SocketClientErrorInfo *error_info;
   gboolean ever_resolved = FALSE;
-  GError *last_error, *tmp_error;
 
-  last_error = NULL;
+  error_info = socket_client_error_info_new ();
 
   if (can_use_proxy (client))
     {
@@ -1019,21 +1085,19 @@ g_socket_client_connect (GSocketClient       *client,
 
       if (g_cancellable_is_cancelled (cancellable))
        {
-         g_clear_error (error);
-         g_clear_error (&last_error);
-         g_cancellable_set_error_if_cancelled (cancellable, error);
+         g_clear_error (&error_info->best_error);
+         g_cancellable_set_error_if_cancelled (cancellable, &error_info->best_error);
          break;
        }
 
-      tmp_error = NULL;
-
       if (!ever_resolved)
        {
          g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVING,
                                      connectable, NULL);
        }
       address = g_socket_address_enumerator_next (enumerator, cancellable,
-                                                 &tmp_error);
+                                                 &error_info->tmp_error);
+      consider_tmp_error (error_info, G_SOCKET_CLIENT_RESOLVING);
       if (!ever_resolved)
        {
          g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVED,
@@ -1043,29 +1107,16 @@ g_socket_client_connect (GSocketClient       *client,
 
       if (address == NULL)
        {
-         if (tmp_error)
-           {
-             g_clear_error (&last_error);
-             g_propagate_error (error, tmp_error);
-           }
-         else if (last_error)
-           {
-             g_propagate_error (error, last_error);
-           }
-         else
-           {
-              g_assert_not_reached ();
-           }
+          /* Enumeration is finished. */
+          g_assert (&error_info->best_error != NULL);
          break;
        }
 
       using_proxy = (G_IS_PROXY_ADDRESS (address) &&
                     client->priv->enable_proxy);
 
-      /* clear error from previous attempt */
-      g_clear_error (&last_error);
-
-      socket = create_socket (client, address, &last_error);
+      socket = create_socket (client, address, &error_info->tmp_error);
+      consider_tmp_error (error_info, G_SOCKET_CLIENT_CONNECTING);
       if (socket == NULL)
        {
          g_object_unref (address);
@@ -1077,14 +1128,15 @@ g_socket_client_connect (GSocketClient       *client,
       g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTING, connectable, connection);
 
       if (g_socket_connection_connect (G_SOCKET_CONNECTION (connection),
-                                      address, cancellable, &last_error))
+                                      address, cancellable, &error_info->tmp_error))
        {
           g_socket_connection_set_cached_remote_address ((GSocketConnection*)connection, NULL);
          g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTED, connectable, connection);
        }
       else
        {
-         clarify_connect_error (last_error, connectable, address);
+         clarify_connect_error (error_info->tmp_error, connectable, address);
+          consider_tmp_error (error_info, G_SOCKET_CLIENT_CONNECTING);
          g_object_unref (connection);
          connection = NULL;
        }
@@ -1105,9 +1157,10 @@ g_socket_client_connect (GSocketClient       *client,
               g_critical ("Trying to proxy over non-TCP connection, this is "
                           "most likely a bug in GLib IO library.");
 
-              g_set_error_literal (&last_error,
+              g_set_error_literal (&error_info->tmp_error,
                   G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
                   _("Proxying over a non-TCP connection is not supported."));
+              consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
 
              g_object_unref (connection);
              connection = NULL;
@@ -1125,7 +1178,9 @@ g_socket_client_connect (GSocketClient       *client,
                                                  connection,
                                                  proxy_addr,
                                                  cancellable,
-                                                 &last_error);
+                                                 &error_info->tmp_error);
+             consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
+
              g_object_unref (connection);
              connection = proxy_connection;
              g_object_unref (proxy);
@@ -1135,9 +1190,10 @@ g_socket_client_connect (GSocketClient       *client,
            }
          else
            {
-             g_set_error (&last_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
+             g_set_error (&error_info->tmp_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
                           _("Proxy protocol ā€œ%sā€ is not supported."),
                           protocol);
+             consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
              g_object_unref (connection);
              connection = NULL;
            }
@@ -1147,7 +1203,7 @@ g_socket_client_connect (GSocketClient       *client,
        {
          GIOStream *tlsconn;
 
-         tlsconn = g_tls_client_connection_new (connection, connectable, &last_error);
+         tlsconn = g_tls_client_connection_new (connection, connectable, &error_info->tmp_error);
          g_object_unref (connection);
          connection = tlsconn;
 
@@ -1157,16 +1213,21 @@ g_socket_client_connect (GSocketClient       *client,
                                                             client->priv->tls_validation_flags);
              g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKING, connectable, connection);
              if (g_tls_connection_handshake (G_TLS_CONNECTION (tlsconn),
-                                             cancellable, &last_error))
+                                             cancellable, &error_info->tmp_error))
                {
                  g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKED, connectable, 
connection);
                }
              else
                {
+                 consider_tmp_error (error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
                  g_object_unref (tlsconn);
                  connection = NULL;
                }
            }
+          else
+            {
+              consider_tmp_error (error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
+            }
        }
 
       if (connection && !G_IS_SOCKET_CONNECTION (connection))
@@ -1183,6 +1244,10 @@ g_socket_client_connect (GSocketClient       *client,
     }
   g_object_unref (enumerator);
 
+  if (!connection)
+    g_propagate_error (error, g_steal_pointer (&error_info->best_error));
+  socket_client_error_info_free (error_info);
+
   g_socket_client_emit_event (client, G_SOCKET_CLIENT_COMPLETE, connectable, connection);
   return G_SOCKET_CONNECTION (connection);
 }
@@ -1360,7 +1425,7 @@ typedef struct
 
   GSList *connection_attempts;
   GSList *successful_connections;
-  GError *last_error;
+  SocketClientErrorInfo *error_info;
 
   gboolean enumerated_at_least_once;
   gboolean enumeration_completed;
@@ -1380,7 +1445,7 @@ g_socket_client_async_connect_data_free (GSocketClientAsyncConnectData *data)
   g_slist_free_full (data->connection_attempts, connection_attempt_unref);
   g_slist_free_full (data->successful_connections, connection_attempt_unref);
 
-  g_clear_error (&data->last_error);
+  g_clear_pointer (&data->error_info, socket_client_error_info_free);
 
   g_slice_free (GSocketClientAsyncConnectData, data);
 }
@@ -1502,14 +1567,6 @@ g_socket_client_enumerator_callback (GObject      *object,
                                     GAsyncResult *result,
                                     gpointer      user_data);
 
-static void
-set_last_error (GSocketClientAsyncConnectData *data,
-               GError *error)
-{
-  g_clear_error (&data->last_error);
-  data->last_error = error;
-}
-
 static void
 enumerator_next_async (GSocketClientAsyncConnectData *data,
                        gboolean                       add_task_ref)
@@ -1540,7 +1597,7 @@ g_socket_client_tls_handshake_callback (GObject      *object,
 
   if (g_tls_connection_handshake_finish (G_TLS_CONNECTION (object),
                                         result,
-                                        &data->last_error))
+                                        &data->error_info->tmp_error))
     {
       g_object_unref (attempt->connection);
       attempt->connection = G_IO_STREAM (object);
@@ -1553,7 +1610,9 @@ g_socket_client_tls_handshake_callback (GObject      *object,
     {
       g_object_unref (object);
       connection_attempt_unref (attempt);
-      g_debug ("GSocketClient: TLS handshake failed: %s", data->last_error->message);
+
+      g_debug ("GSocketClient: TLS handshake failed: %s", data->error_info->tmp_error->message);
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
       try_next_connection_or_finish (data, TRUE);
     }
 }
@@ -1573,7 +1632,7 @@ g_socket_client_tls_handshake (ConnectionAttempt *attempt)
   g_debug ("GSocketClient: Starting TLS handshake");
   tlsconn = g_tls_client_connection_new (attempt->connection,
                                         data->connectable,
-                                        &data->last_error);
+                                        &data->error_info->tmp_error);
   if (tlsconn)
     {
       g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (tlsconn),
@@ -1588,6 +1647,8 @@ g_socket_client_tls_handshake (ConnectionAttempt *attempt)
   else
     {
       connection_attempt_unref (attempt);
+
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
       try_next_connection_or_finish (data, TRUE);
     }
 }
@@ -1603,19 +1664,19 @@ g_socket_client_proxy_connect_callback (GObject      *object,
   g_object_unref (attempt->connection);
   attempt->connection = g_proxy_connect_finish (G_PROXY (object),
                                                 result,
-                                                &data->last_error);
+                                                &data->error_info->tmp_error);
   if (attempt->connection)
     {
       g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATED, data->connectable, 
attempt->connection);
+      g_socket_client_tls_handshake (attempt);
     }
   else
     {
       connection_attempt_unref (attempt);
+
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
       try_next_connection_or_finish (data, TRUE);
-      return;
     }
-
-  g_socket_client_tls_handshake (attempt);
 }
 
 static void
@@ -1683,9 +1744,10 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
       g_critical ("Trying to proxy over non-TCP connection, this is "
           "most likely a bug in GLib IO library.");
 
-      g_set_error_literal (&data->last_error,
+      g_set_error_literal (&data->error_info->tmp_error,
           G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
           _("Proxying over a non-TCP connection is not supported."));
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
     }
   else if (g_hash_table_contains (data->client->priv->app_proxies, protocol))
     {
@@ -1712,11 +1774,10 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
     }
   else
     {
-      g_clear_error (&data->last_error);
-
-      g_set_error (&data->last_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
+      g_set_error (&data->error_info->tmp_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
           _("Proxy protocol ā€œ%sā€ is not supported."),
           protocol);
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
     }
 
   data->connection_in_progress = FALSE;
@@ -1747,7 +1808,7 @@ try_next_connection_or_finish (GSocketClientAsyncConnectData *data,
       return;
     }
 
-  complete_connection_with_error (data, data->last_error);
+  complete_connection_with_error (data, g_steal_pointer (&data->error_info->best_error));
 }
 
 static void
@@ -1757,7 +1818,6 @@ g_socket_client_connected_callback (GObject      *source,
 {
   ConnectionAttempt *attempt = user_data;
   GSocketClientAsyncConnectData *data = attempt->data;
-  GError *error = NULL;
 
   if (task_completed_or_cancelled (data) || g_cancellable_is_cancelled (attempt->cancellable))
     {
@@ -1773,20 +1833,20 @@ g_socket_client_connected_callback (GObject      *source,
     }
 
   if (!g_socket_connection_connect_finish (G_SOCKET_CONNECTION (source),
-                                          result, &error))
+                                          result, &data->error_info->tmp_error))
     {
       if (!g_cancellable_is_cancelled (attempt->cancellable))
         {
-          clarify_connect_error (error, data->connectable, attempt->address);
-          set_last_error (data, error);
-          g_debug ("GSocketClient: Connection attempt failed: %s", error->message);
+          clarify_connect_error (data->error_info->tmp_error, data->connectable, attempt->address);
+          consider_tmp_error (data->error_info, G_SOCKET_CLIENT_CONNECTING);
+          g_debug ("GSocketClient: Connection attempt failed: %s", data->error_info->tmp_error->message);
           connection_attempt_remove (attempt);
           connection_attempt_unref (attempt);
           try_next_connection_or_finish (data, FALSE);
         }
       else /* Silently ignore cancelled attempts */
         {
-          g_clear_error (&error);
+          g_clear_error (&data->error_info->tmp_error);
           g_object_unref (data->task);
           connection_attempt_unref (attempt);
         }
@@ -1844,7 +1904,6 @@ g_socket_client_enumerator_callback (GObject      *object,
   GSocketAddress *address = NULL;
   GSocket *socket;
   ConnectionAttempt *attempt;
-  GError *error = NULL;
 
   if (task_completed_or_cancelled (data))
     {
@@ -1853,7 +1912,7 @@ g_socket_client_enumerator_callback (GObject      *object,
     }
 
   address = g_socket_address_enumerator_next_finish (data->enumerator,
-                                                    result, &error);
+                                                    result, &data->error_info->tmp_error);
   if (address == NULL)
     {
       if (G_UNLIKELY (data->enumeration_completed))
@@ -1862,7 +1921,7 @@ g_socket_client_enumerator_callback (GObject      *object,
       data->enumeration_completed = TRUE;
       g_debug ("GSocketClient: Address enumeration completed (out of addresses)");
 
-      /* As per API docs: We only care about error if its the first call,
+      /* As per API docs: We only care about error if it's the first call,
          after that the enumerator is done.
 
          Note that we don't care about cancellation errors because
@@ -1873,19 +1932,11 @@ g_socket_client_enumerator_callback (GObject      *object,
       if ((data->enumerated_at_least_once && !data->connection_attempts && !data->connection_in_progress) ||
           !data->enumerated_at_least_once)
         {
-          g_debug ("GSocketClient: Address enumeration failed: %s", error ? error->message : NULL);
-          if (data->last_error)
-            {
-              g_clear_error (&error);
-              error = data->last_error;
-              data->last_error = NULL;
-            }
-          else
-            {
-              g_assert (error);
-            }
-
-          complete_connection_with_error (data, error);
+          g_debug ("GSocketClient: Address enumeration failed: %s",
+                   data->error_info->tmp_error ? data->error_info->tmp_error->message : NULL);
+          consider_tmp_error (data->error_info, G_SOCKET_CLIENT_RESOLVING);
+          g_assert (data->error_info->best_error);
+          complete_connection_with_error (data, g_steal_pointer (&data->error_info->best_error));
         }
 
       /* Enumeration should never trigger again, drop our ref */
@@ -1901,12 +1952,11 @@ g_socket_client_enumerator_callback (GObject      *object,
       data->enumerated_at_least_once = TRUE;
     }
 
-  g_clear_error (&data->last_error);
-
-  socket = create_socket (data->client, address, &data->last_error);
+  socket = create_socket (data->client, address, &data->error_info->tmp_error);
   if (socket == NULL)
     {
       g_object_unref (address);
+      consider_tmp_error (data->error_info, G_SOCKET_CLIENT_CONNECTING);
       enumerator_next_async (data, FALSE);
       return;
     }
@@ -1978,6 +2028,7 @@ g_socket_client_connect_async (GSocketClient       *client,
   data = g_slice_new0 (GSocketClientAsyncConnectData);
   data->client = client;
   data->connectable = g_object_ref (connectable);
+  data->error_info = socket_client_error_info_new ();
 
   if (can_use_proxy (client))
     {


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