From dbe5359084be7938582b5b97860122d9a7bbc178 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Thu, 9 Jun 2022 13:30:02 -0500 Subject: [PATCH] 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. (Backport to 2.72: Dropped new translatable strings in error messages to avoid additional translation work on a stable branch. The error messages are unlikely to be seen by users.) Fixes #2597 --- gio/gproxyaddressenumerator.c | 40 +++++++-- gio/tests/proxy-test.c | 154 ++++++++++++++++++++++++++++++++-- 2 files changed, 180 insertions(+), 14 deletions(-) diff --git a/gio/gproxyaddressenumerator.c b/gio/gproxyaddressenumerator.c index 654baade5..de932ff91 100644 --- a/gio/gproxyaddressenumerator.c +++ b/gio/gproxyaddressenumerator.c @@ -25,6 +25,7 @@ #include "gasyncresult.h" #include "ginetaddress.h" +#include "gioerror.h" #include "glibintl.h" #include "gnetworkaddress.h" #include "gnetworkingprivate.h" @@ -87,6 +88,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) @@ -171,8 +186,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, @@ -180,7 +196,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)) @@ -294,29 +313,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); } @@ -388,6 +415,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 eec4bf7ca..d4b71a432 100644 --- a/gio/tests/proxy-test.c +++ b/gio/tests/proxy-test.c @@ -41,9 +41,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 { @@ -134,6 +139,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 @@ -824,11 +851,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); } @@ -1091,6 +1115,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) @@ -1370,6 +1506,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);