Merge branch 'mcatanzaro/#2597' into 'main'

Avoid crashing when GProxyResolver returns weird results, and related fixes

Closes #2597

See merge request GNOME/glib!2742

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/gproxyresolver.c

@@ -158,6 +158,7 @@ g_proxy_resolver_lookup (GProxyResolver  *resolver,
 			 GError         **error)
 {
   GProxyResolverInterface *iface;
+  gchar **proxy_uris;
 
   g_return_val_if_fail (G_IS_PROXY_RESOLVER (resolver), NULL);
   g_return_val_if_fail (uri != NULL, NULL);
@@ -171,7 +172,10 @@ g_proxy_resolver_lookup (GProxyResolver  *resolver,
 
   iface = G_PROXY_RESOLVER_GET_IFACE (resolver);
 
-  return (* iface->lookup) (resolver, uri, cancellable, error);
+  proxy_uris = (* iface->lookup) (resolver, uri, cancellable, error);
+  if (proxy_uris == NULL && error != NULL)
+    g_assert (*error != NULL);
+  return proxy_uris;
 }
 
 /**
@@ -237,10 +241,14 @@ g_proxy_resolver_lookup_finish (GProxyResolver     *resolver,
 				GError            **error)
 {
   GProxyResolverInterface *iface;
+  gchar **proxy_uris;
 
   g_return_val_if_fail (G_IS_PROXY_RESOLVER (resolver), NULL);
 
   iface = G_PROXY_RESOLVER_GET_IFACE (resolver);
 
-  return (* iface->lookup_finish) (resolver, result, error);
+  proxy_uris = (* iface->lookup_finish) (resolver, result, error);
+  if (proxy_uris == NULL && error != NULL)
+    g_assert (*error != NULL);
+  return proxy_uris;
 }

gio/gsimpleproxyresolver.c

@@ -417,7 +417,7 @@ g_simple_proxy_resolver_class_init (GSimpleProxyResolverClass *resolver_class)
   object_class->finalize = g_simple_proxy_resolver_finalize;
 
   /**
-   * GSimpleProxyResolver:default-proxy:
+   * GSimpleProxyResolver:default-proxy: (nullable)
    *
    * The default proxy URI that will be used for any URI that doesn't
    * match #GSimpleProxyResolver:ignore-hosts, and doesn't match any
@@ -520,7 +520,7 @@ g_simple_proxy_resolver_new (const gchar  *default_proxy,
 /**
  * g_simple_proxy_resolver_set_default_proxy:
  * @resolver: a #GSimpleProxyResolver
- * @default_proxy: the default proxy to use
+ * @default_proxy: (nullable): the default proxy to use
  *
  * Sets the default proxy on @resolver, to be used for any URIs that
  * don't match #GSimpleProxyResolver:ignore-hosts or a proxy set
@@ -537,6 +537,7 @@ g_simple_proxy_resolver_set_default_proxy (GSimpleProxyResolver *resolver,
                                            const gchar          *default_proxy)
 {
   g_return_if_fail (G_IS_SIMPLE_PROXY_RESOLVER (resolver));
+  g_return_if_fail (default_proxy == NULL || g_uri_is_valid (default_proxy, G_URI_FLAGS_NONE, NULL));
 
   g_free (resolver->priv->default_proxy);
   resolver->priv->default_proxy = g_strdup (default_proxy);

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 {
@@ -131,11 +136,33 @@ g_test_proxy_resolver_lookup (GProxyResolver  *resolver,
 
   proxies = g_new (gchar *, 3);
 
-  if (!strncmp (uri, "simple://", 4))
+  if (g_str_has_prefix (uri, "simple://"))
     {
       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);