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

@@ -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,10 @@ g_proxy_address_enumerator_next (GSocketAddressEnumerator  *enumerator,
       priv->next_proxy = priv->proxies;
 
       if (priv->proxies == NULL)
-	return NULL;
+	{
+	  priv->ever_enumerated = TRUE;
+	  return NULL;
+	}
     }
 
   while (result == NULL && (*priv->next_proxy || priv->addr_enum))
@@ -296,29 +315,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);
 
+  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 +419,7 @@ return_result (GTask *task)
 	}
     }
 
+  priv->ever_enumerated = TRUE;
   g_task_return_pointer (task, result, g_object_unref);
   g_object_unref (task);
 }

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