[glib/mcatanzaro/#2211: 8/8] gsocketclient: return best errors possible
- From: Michael Catanzaro <mcatanzaro src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [glib/mcatanzaro/#2211: 8/8] gsocketclient: return best errors possible
- Date: Fri, 9 Oct 2020 15:50:27 +0000 (UTC)
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]
