[glib/mcatanzaro/#2597: 110/110] proxyaddressenumerator: set error parameter more thoughtfully

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 89a295ba2d1741b58b2dee9e9f26afa625a8267a
Author: Michael Catanzaro <mcatanzaro redhat com>
Date:   Thu Jun 9 13:30:02 2022 -0500

    proxyaddressenumerator: set error parameter more thoughtfully
    
    It doesn't make sense for a proxy resolver to return NULL without an
    error on the first call. Whereas a DNS resolver would do this to
    indicate that a query completed successfully but found no results, a
    proxy resolver should return "direct://" instead. Therefore, if we are
    going to return NULL, we ought to have an error as well. Let's make sure
    this actually happens by adding some fallback errors just in case
    GProxyResolver feeds us weird results.
    
    Additionally, we should not return any errors except
    G_IO_ERROR_CANCELLED after the very first iteration. This is an API
    contract of GSocketAddressEnumerator. Let's add some checks to ensure
    this.
    
    Note that we have inadequate test coverage for GProxyAddressEnumerator.
    It's tested here only via GSocketClient. We could do a bit better by
    testing it directly as well. For example, I've added tests to see what
    happens when GProxyResolver returns both a valid and an invalid URI, but
    it's not so interesting here because GSocketClient always uses the valid
    result and ignores the error from GProxyAddressEnumerator.
    
    Fixes #2597

 gio/gproxyaddressenumerator.c |  42 ++++++++++--
 gio/tests/proxy-test.c        | 154 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 182 insertions(+), 14 deletions(-)
---
diff --git a/gio/gproxyaddressenumerator.c b/gio/gproxyaddressenumerator.c
index 322f0acaee..688a97359b 100644
--- a/gio/gproxyaddressenumerator.c
+++ b/gio/gproxyaddressenumerator.c
@@ -27,6 +27,7 @@
 
 #include "gasyncresult.h"
 #include "ginetaddress.h"
+#include "gioerror.h"
 #include "glibintl.h"
 #include "gnetworkaddress.h"
 #include "gnetworkingprivate.h"
@@ -89,6 +90,20 @@ struct _GProxyAddressEnumeratorPrivate
   gboolean                  supports_hostname;
   GList                    *next_dest_ip;
   GError                   *last_error;
+
+  /* ever_enumerated is TRUE after we've returned a result for the first time
+   * via g_proxy_address_enumerator_next() or _next_async(). If FALSE, we have
+   * never returned yet, and should return an error if returning NULL because
+   * it does not make sense for a proxy resolver to return NULL except on error.
+   * (Whereas a DNS resolver would return NULL with no error to indicate "no
+   * results", a proxy resolver would want to return "direct://" instead, so
+   * NULL without error does not make sense for us.)
+   *
+   * But if ever_enumerated is TRUE, then we must not report any further errors
+   * (except for G_IO_ERROR_CANCELLED), because this is an API contract of
+   * GSocketAddressEnumerator.
+   */
+  gboolean                  ever_enumerated;
 };
 
 G_DEFINE_TYPE_WITH_PRIVATE (GProxyAddressEnumerator, g_proxy_address_enumerator, 
G_TYPE_SOCKET_ADDRESS_ENUMERATOR)
@@ -173,8 +188,9 @@ g_proxy_address_enumerator_next (GSocketAddressEnumerator  *enumerator,
   GSocketAddress *result = NULL;
   GError *first_error = NULL;
 
-  if (priv->proxies == NULL)
+  if (!priv->ever_enumerated)
     {
+      g_assert (priv->proxies == NULL);
       priv->proxies = g_proxy_resolver_lookup (priv->proxy_resolver,
                                               priv->dest_uri,
                                               cancellable,
@@ -182,7 +198,12 @@ g_proxy_address_enumerator_next (GSocketAddressEnumerator  *enumerator,
       priv->next_proxy = priv->proxies;
 
       if (priv->proxies == NULL)
-       return NULL;
+       {
+         priv->ever_enumerated = TRUE;
+         if (error != NULL && *error == NULL)
+           g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, _("Unspecified proxy lookup failure"));
+         return NULL;
+       }
     }
 
   while (result == NULL && (*priv->next_proxy || priv->addr_enum))
@@ -296,29 +317,37 @@ g_proxy_address_enumerator_next (GSocketAddressEnumerator  *enumerator,
        }
     }
 
-  if (result == NULL && first_error)
+  if (result == NULL && first_error && (!priv->ever_enumerated || g_error_matches (first_error, G_IO_ERROR, 
G_IO_ERROR_CANCELLED)))
     g_propagate_error (error, first_error);
   else if (first_error)
     g_error_free (first_error);
 
-  return result;
-}
+  if (result == NULL && error != NULL && *error == NULL && !priv->ever_enumerated)
+    g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, _("Unspecified proxy lookup failure"));
 
+  priv->ever_enumerated = TRUE;
 
+  return result;
+}
 
 static void
 complete_async (GTask *task)
 {
   GProxyAddressEnumeratorPrivate *priv = g_task_get_task_data (task);
 
-  if (priv->last_error)
+  if (priv->last_error && (!priv->ever_enumerated || g_error_matches (priv->last_error, G_IO_ERROR, 
G_IO_ERROR_CANCELLED)))
     {
       g_task_return_error (task, priv->last_error);
       priv->last_error = NULL;
     }
+  else if (!priv->ever_enumerated)
+    g_task_return_new_error (task, G_IO_ERROR, G_IO_ERROR_FAILED, _("Unspecified proxy lookup failure"));
   else
     g_task_return_pointer (task, NULL, NULL);
 
+  priv->ever_enumerated = TRUE;
+
+  g_clear_error (&priv->last_error);
   g_object_unref (task);
 }
 
@@ -392,6 +421,7 @@ return_result (GTask *task)
        }
     }
 
+  priv->ever_enumerated = TRUE;
   g_task_return_pointer (task, result, g_object_unref);
   g_object_unref (task);
 }
diff --git a/gio/tests/proxy-test.c b/gio/tests/proxy-test.c
index ed0111def1..f936a9383c 100644
--- a/gio/tests/proxy-test.c
+++ b/gio/tests/proxy-test.c
@@ -43,9 +43,14 @@
  * connects to @server_addr anyway).
  *
  * The default GProxyResolver (GTestProxyResolver) looks at its URI
- * and returns [ "direct://" ] for "simple://" URIs, and [
- * proxy_a.uri, proxy_b.uri ] for other URIs. The other GProxyResolver
- * (GTestAltProxyResolver) always returns [ proxy_a.uri ].
+ * and returns [ "direct://" ] for "simple://" URIs, and
+ * [ proxy_a.uri, proxy_b.uri ] for most other URIs. It can also return
+ * invalid results for other URIs (empty://, invalid://,
+ * invalid-then-simple://, and simple-then-invalid://) to test error
+ * handling.
+ *
+ * The other GProxyResolver (GTestAltProxyResolver) always returns
+ * [ proxy_a.uri ].
  */
 
 typedef struct {
@@ -136,6 +141,28 @@ g_test_proxy_resolver_lookup (GProxyResolver  *resolver,
       proxies[0] = g_strdup ("direct://");
       proxies[1] = NULL;
     }
+  else if (g_str_has_prefix (uri, "empty://"))
+    {
+      proxies[0] = g_strdup ("");
+      proxies[1] = NULL;
+    }
+  else if (g_str_has_prefix (uri, "invalid://"))
+    {
+      proxies[0] = g_strdup ("😼");
+      proxies[1] = NULL;
+    }
+  else if (g_str_has_prefix (uri, "invalid-then-simple://"))
+    {
+      proxies[0] = g_strdup ("😼");
+      proxies[1] = g_strdup ("direct://");
+      proxies[2] = NULL;
+    }
+  else if (g_str_has_prefix (uri, "simple-then-invalid://"))
+    {
+      proxies[0] = g_strdup ("direct://");
+      proxies[1] = g_strdup ("😼");
+      proxies[2] = NULL;
+    }
   else
     {
       /* Proxy A can only deal with "alpha://" URIs, not
@@ -826,11 +853,8 @@ static void
 teardown_test (gpointer fixture,
               gconstpointer user_data)
 {
-  if (last_proxies)
-    {
-      g_strfreev (last_proxies);
-      last_proxies = NULL;
-    }
+  g_clear_pointer (&last_proxies, g_strfreev);
+
   g_clear_error (&proxy_a.last_error);
   g_clear_error (&proxy_b.last_error);
 }
@@ -1093,6 +1117,118 @@ test_multiple_async (gpointer fixture,
   g_object_unref (conn);
 }
 
+static void
+test_invalid_uris_sync (gpointer fixture,
+                       gconstpointer user_data)
+{
+  GSocketConnection *conn;
+  gchar *uri;
+  GError *error = NULL;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2597";);
+
+  /* The empty:// URI causes the proxy resolver to return an empty string. */
+  uri = g_strdup_printf ("empty://127.0.0.1:%u", server.server_port);
+  conn = g_socket_client_connect_to_uri (client, uri, 0, NULL, &error);
+  g_free (uri);
+  g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
+  g_assert_null (conn);
+  g_clear_error (&error);
+  g_clear_pointer (&last_proxies, g_strfreev);
+
+  /* The invalid:// URI causes the proxy resolver to return a cat emoji. */
+  uri = g_strdup_printf ("invalid://127.0.0.1:%u", server.server_port);
+  conn = g_socket_client_connect_to_uri (client, uri, 0, NULL, &error);
+  g_free (uri);
+  g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
+  g_assert_null (conn);
+  g_clear_error (&error);
+  g_clear_pointer (&last_proxies, g_strfreev);
+
+  /* If the proxy resolver returns an invalid URI before a valid URI,
+   * we should succeed.
+   */
+  uri = g_strdup_printf ("invalid-then-simple://127.0.0.1:%u", server.server_port);
+  conn = g_socket_client_connect_to_uri (client, uri, 0, NULL, &error);
+  g_free (uri);
+  g_assert_no_error (error);
+  do_echo_test (conn);
+  g_object_unref (conn);
+  g_clear_pointer (&last_proxies, g_strfreev);
+
+  /* If the proxy resolver returns a valid URI before an invalid URI,
+   * we should succeed.
+   */
+  uri = g_strdup_printf ("simple-then-invalid://127.0.0.1:%u", server.server_port);
+  conn = g_socket_client_connect_to_uri (client, uri, 0, NULL, &error);
+  g_free (uri);
+  g_assert_no_error (error);
+  do_echo_test (conn);
+  g_object_unref (conn);
+  g_clear_pointer (&last_proxies, g_strfreev);
+}
+
+static void
+test_invalid_uris_async (gpointer fixture,
+                        gconstpointer user_data)
+{
+  GSocketConnection *conn;
+  GError *error = NULL;
+  gchar *uri;
+
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2597";);
+
+  /* The empty:// URI causes the proxy resolver to return an empty string. */
+  uri = g_strdup_printf ("empty://127.0.0.1:%u", server.server_port);
+  g_socket_client_connect_to_uri_async (client, uri, 0, NULL,
+                                       async_got_error, &error);
+  g_free (uri);
+  while (error == NULL)
+    g_main_context_iteration (NULL, TRUE);
+  g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
+  g_clear_error (&error);
+  g_clear_pointer (&last_proxies, g_strfreev);
+
+  /* The invalid:// URI causes the proxy resolver to return a cat emoji. */
+  uri = g_strdup_printf ("invalid://127.0.0.1:%u", server.server_port);
+  g_socket_client_connect_to_uri_async (client, uri, 0, NULL,
+                                       async_got_error, &error);
+  g_free (uri);
+  while (error == NULL)
+    g_main_context_iteration (NULL, TRUE);
+  g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
+  g_clear_error (&error);
+  g_clear_pointer (&last_proxies, g_strfreev);
+
+  /* If the proxy resolver returns an invalid URI before a valid URI,
+   * we should succeed.
+   */
+  uri = g_strdup_printf ("invalid-then-simple://127.0.0.1:%u", server.server_port);
+  conn = NULL;
+  g_socket_client_connect_to_uri_async (client, uri, 0, NULL,
+                                       async_got_conn, &conn);
+  g_free (uri);
+  while (conn == NULL)
+    g_main_context_iteration (NULL, TRUE);
+  do_echo_test (conn);
+  g_object_unref (conn);
+  g_clear_pointer (&last_proxies, g_strfreev);
+
+  /* If the proxy resolver returns a valid URI before an invalid URI,
+   * we should succeed.
+   */
+  uri = g_strdup_printf ("simple-then-invalid://127.0.0.1:%u", server.server_port);
+  conn = NULL;
+  g_socket_client_connect_to_uri_async (client, uri, 0, NULL,
+                                       async_got_conn, &conn);
+  g_free (uri);
+  while (conn == NULL)
+    g_main_context_iteration (NULL, TRUE);
+  do_echo_test (conn);
+  g_object_unref (conn);
+  g_clear_pointer (&last_proxies, g_strfreev);
+}
+
 static void
 test_dns (gpointer fixture,
          gconstpointer user_data)
@@ -1372,6 +1508,8 @@ main (int   argc,
   g_test_add_vtable ("/proxy/single_async", 0, NULL, setup_test, test_single_async, teardown_test);
   g_test_add_vtable ("/proxy/multiple_sync", 0, NULL, setup_test, test_multiple_sync, teardown_test);
   g_test_add_vtable ("/proxy/multiple_async", 0, NULL, setup_test, test_multiple_async, teardown_test);
+  g_test_add_vtable ("/proxy/invalid-uris-sync", 0, NULL, setup_test, test_invalid_uris_sync, teardown_test);
+  g_test_add_vtable ("/proxy/invalid-uris-async", 0, NULL, setup_test, test_invalid_uris_async, 
teardown_test);
   g_test_add_vtable ("/proxy/dns", 0, NULL, setup_test, test_dns, teardown_test);
   g_test_add_vtable ("/proxy/override", 0, NULL, setup_test, test_override, teardown_test);
   g_test_add_func ("/proxy/enumerator-ports", test_proxy_enumerator_ports);