luc14n0/glib
1
0
Fork 0
mirror of https://gitlab.gnome.org/GNOME/glib.git synced 2024-12-26 15:36:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

gsocketclient: return best errors possible

Browse Source 
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
This commit is contained in:
Michael Catanzaro 2020-10-08 18:02:56 -05:00
parent 14f7b5e590
commit b88b3712e0
1 changed files with 131 additions and 80 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

211
gio/gsocketclient.c
View File

@ -953,6 +953,72 @@ g_socket_client_emit_event (GSocketClient *client,
event, connectable, connection); 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: * g_socket_client_connect:
* @client: a #GSocketClient. * @client: a #GSocketClient.
@ -991,10 +1057,10 @@ g_socket_client_connect (GSocketClient *client,
{ {
GIOStream *connection = NULL; GIOStream *connection = NULL;
GSocketAddressEnumerator *enumerator = NULL; GSocketAddressEnumerator *enumerator = NULL;
SocketClientErrorInfo *error_info;
gboolean ever_resolved = FALSE; gboolean ever_resolved = FALSE;
GError *last_error, *tmp_error;
last_error = NULL; error_info = socket_client_error_info_new ();
if (can_use_proxy (client)) if (can_use_proxy (client))
{ {
@ -1019,21 +1085,19 @@ g_socket_client_connect (GSocketClient *client,
if (g_cancellable_is_cancelled (cancellable)) if (g_cancellable_is_cancelled (cancellable))
{ {
g_clear_error (error); g_clear_error (&error_info->best_error);
g_clear_error (&last_error); g_cancellable_set_error_if_cancelled (cancellable, &error_info->best_error);
g_cancellable_set_error_if_cancelled (cancellable, error);
break; break;
} }
tmp_error = NULL;
if (!ever_resolved) if (!ever_resolved)
{ {
g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVING, g_socket_client_emit_event (client, G_SOCKET_CLIENT_RESOLVING,
connectable, NULL); connectable, NULL);
} }
address = g_socket_address_enumerator_next (enumerator, cancellable, 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) if (!ever_resolved)
{ {
g_socket_client_emit_event (client, G_SOCKET_CLIENT_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 (address == NULL)
{ {
if (tmp_error) /* Enumeration is finished. */
{ g_assert (&error_info->best_error != NULL);
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 ();
}
break; break;
} }
using_proxy = (G_IS_PROXY_ADDRESS (address) && using_proxy = (G_IS_PROXY_ADDRESS (address) &&
client->priv->enable_proxy); client->priv->enable_proxy);
/* clear error from previous attempt */ socket = create_socket (client, address, &error_info->tmp_error);
g_clear_error (&last_error); consider_tmp_error (error_info, G_SOCKET_CLIENT_CONNECTING);
socket = create_socket (client, address, &last_error);
if (socket == NULL) if (socket == NULL)
{ {
g_object_unref (address); 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); g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTING, connectable, connection);
if (g_socket_connection_connect (G_SOCKET_CONNECTION (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_connection_set_cached_remote_address ((GSocketConnection*)connection, NULL);
g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTED, connectable, connection); g_socket_client_emit_event (client, G_SOCKET_CLIENT_CONNECTED, connectable, connection);
} }
else 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); g_object_unref (connection);
connection = NULL; connection = NULL;
} }
@ -1105,9 +1157,10 @@ g_socket_client_connect (GSocketClient *client,
g_critical ("Trying to proxy over non-TCP connection, this is " g_critical ("Trying to proxy over non-TCP connection, this is "
"most likely a bug in GLib IO library."); "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, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
_("Proxying over a non-TCP connection is 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); g_object_unref (connection);
connection = NULL; connection = NULL;
@ -1125,7 +1178,9 @@ g_socket_client_connect (GSocketClient *client,
connection, connection,
proxy_addr, proxy_addr,
cancellable, cancellable,
&last_error); &error_info->tmp_error);
consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
g_object_unref (connection); g_object_unref (connection);
connection = proxy_connection; connection = proxy_connection;
g_object_unref (proxy); g_object_unref (proxy);
@ -1135,9 +1190,10 @@ g_socket_client_connect (GSocketClient *client,
} }
else 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."), _("Proxy protocol “%s” is not supported."),
protocol); protocol);
consider_tmp_error (error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
g_object_unref (connection); g_object_unref (connection);
connection = NULL; connection = NULL;
} }
@ -1147,7 +1203,7 @@ g_socket_client_connect (GSocketClient *client,
{ {
GIOStream *tlsconn; 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); g_object_unref (connection);
connection = tlsconn; connection = tlsconn;
@ -1157,16 +1213,21 @@ g_socket_client_connect (GSocketClient *client,
client->priv->tls_validation_flags); client->priv->tls_validation_flags);
g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKING, connectable, connection); g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKING, connectable, connection);
if (g_tls_connection_handshake (G_TLS_CONNECTION (tlsconn), 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); g_socket_client_emit_event (client, G_SOCKET_CLIENT_TLS_HANDSHAKED, connectable, connection);
} }
else else
{ {
consider_tmp_error (error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
g_object_unref (tlsconn); g_object_unref (tlsconn);
connection = NULL; connection = NULL;
} }
} }
else
{
consider_tmp_error (error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
}
} }
if (connection && !G_IS_SOCKET_CONNECTION (connection)) if (connection && !G_IS_SOCKET_CONNECTION (connection))
@ -1183,6 +1244,10 @@ g_socket_client_connect (GSocketClient *client,
} }
g_object_unref (enumerator); 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); g_socket_client_emit_event (client, G_SOCKET_CLIENT_COMPLETE, connectable, connection);
return G_SOCKET_CONNECTION (connection); return G_SOCKET_CONNECTION (connection);
} }
@ -1360,7 +1425,7 @@ typedef struct
GSList *connection_attempts; GSList *connection_attempts;
GSList *successful_connections; GSList *successful_connections;
GError *last_error; SocketClientErrorInfo *error_info;
gboolean enumerated_at_least_once; gboolean enumerated_at_least_once;
gboolean enumeration_completed; 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->connection_attempts, connection_attempt_unref);
g_slist_free_full (data->successful_connections, 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); g_slice_free (GSocketClientAsyncConnectData, data);
} }
@ -1502,14 +1567,6 @@ g_socket_client_enumerator_callback (GObject *object,
GAsyncResult *result, GAsyncResult *result,
gpointer user_data); gpointer user_data);
static void
set_last_error (GSocketClientAsyncConnectData *data,
GError *error)
{
g_clear_error (&data->last_error);
data->last_error = error;
}
static void static void
enumerator_next_async (GSocketClientAsyncConnectData *data, enumerator_next_async (GSocketClientAsyncConnectData *data,
gboolean add_task_ref) 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), if (g_tls_connection_handshake_finish (G_TLS_CONNECTION (object),
result, result,
&data->last_error)) &data->error_info->tmp_error))
{ {
g_object_unref (attempt->connection); g_object_unref (attempt->connection);
attempt->connection = G_IO_STREAM (object); attempt->connection = G_IO_STREAM (object);
@ -1553,7 +1610,9 @@ g_socket_client_tls_handshake_callback (GObject *object,
{ {
g_object_unref (object); g_object_unref (object);
connection_attempt_unref (attempt); 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); try_next_connection_or_finish (data, TRUE);
} }
} }
@ -1573,7 +1632,7 @@ g_socket_client_tls_handshake (ConnectionAttempt *attempt)
g_debug ("GSocketClient: Starting TLS handshake"); g_debug ("GSocketClient: Starting TLS handshake");
tlsconn = g_tls_client_connection_new (attempt->connection, tlsconn = g_tls_client_connection_new (attempt->connection,
data->connectable, data->connectable,
&data->last_error); &data->error_info->tmp_error);
if (tlsconn) if (tlsconn)
{ {
g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (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 else
{ {
connection_attempt_unref (attempt); connection_attempt_unref (attempt);
consider_tmp_error (data->error_info, G_SOCKET_CLIENT_TLS_HANDSHAKING);
try_next_connection_or_finish (data, TRUE); try_next_connection_or_finish (data, TRUE);
} }
} }
@ -1603,19 +1664,19 @@ g_socket_client_proxy_connect_callback (GObject *object,
g_object_unref (attempt->connection); g_object_unref (attempt->connection);
attempt->connection = g_proxy_connect_finish (G_PROXY (object), attempt->connection = g_proxy_connect_finish (G_PROXY (object),
result, result,
&data->last_error); &data->error_info->tmp_error);
if (attempt->connection) if (attempt->connection)
{ {
g_socket_client_emit_event (data->client, G_SOCKET_CLIENT_PROXY_NEGOTIATED, data->connectable, 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 else
{ {
connection_attempt_unref (attempt); connection_attempt_unref (attempt);
try_next_connection_or_finish (data, TRUE);
return;
}
g_socket_client_tls_handshake (attempt); consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
try_next_connection_or_finish (data, TRUE);
}
} }
static void static void
@ -1683,9 +1744,10 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
g_critical ("Trying to proxy over non-TCP connection, this is " g_critical ("Trying to proxy over non-TCP connection, this is "
"most likely a bug in GLib IO library."); "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, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
_("Proxying over a non-TCP connection is 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)) else if (g_hash_table_contains (data->client->priv->app_proxies, protocol))
{ {
@ -1712,11 +1774,10 @@ try_next_successful_connection (GSocketClientAsyncConnectData *data)
} }
else else
{ {
g_clear_error (&data->last_error); g_set_error (&data->error_info->tmp_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
g_set_error (&data->last_error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
_("Proxy protocol “%s” is not supported."), _("Proxy protocol “%s” is not supported."),
protocol); protocol);
consider_tmp_error (data->error_info, G_SOCKET_CLIENT_PROXY_NEGOTIATING);
} }
data->connection_in_progress = FALSE; data->connection_in_progress = FALSE;
@ -1747,7 +1808,7 @@ try_next_connection_or_finish (GSocketClientAsyncConnectData *data,
return; return;
} }
complete_connection_with_error (data, data->last_error); complete_connection_with_error (data, g_steal_pointer (&data->error_info->best_error));
} }
static void static void
@ -1757,7 +1818,6 @@ g_socket_client_connected_callback (GObject *source,
{ {
ConnectionAttempt *attempt = user_data; ConnectionAttempt *attempt = user_data;
GSocketClientAsyncConnectData *data = attempt->data; GSocketClientAsyncConnectData *data = attempt->data;
GError *error = NULL;
if (task_completed_or_cancelled (data) || g_cancellable_is_cancelled (attempt->cancellable)) 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), 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)) if (!g_cancellable_is_cancelled (attempt->cancellable))
{ {
clarify_connect_error (error, data->connectable, attempt->address); clarify_connect_error (data->error_info->tmp_error, data->connectable, attempt->address);
set_last_error (data, error); consider_tmp_error (data->error_info, G_SOCKET_CLIENT_CONNECTING);
g_debug ("GSocketClient: Connection attempt failed: %s", error->message); g_debug ("GSocketClient: Connection attempt failed: %s", data->error_info->tmp_error->message);
connection_attempt_remove (attempt); connection_attempt_remove (attempt);
connection_attempt_unref (attempt); connection_attempt_unref (attempt);
try_next_connection_or_finish (data, FALSE); try_next_connection_or_finish (data, FALSE);
} }
else /* Silently ignore cancelled attempts */ else /* Silently ignore cancelled attempts */
{ {
g_clear_error (&error); g_clear_error (&data->error_info->tmp_error);
g_object_unref (data->task); g_object_unref (data->task);
connection_attempt_unref (attempt); connection_attempt_unref (attempt);
} }
@ -1844,7 +1904,6 @@ g_socket_client_enumerator_callback (GObject *object,
GSocketAddress *address = NULL; GSocketAddress *address = NULL;
GSocket *socket; GSocket *socket;
ConnectionAttempt *attempt; ConnectionAttempt *attempt;
GError *error = NULL;
if (task_completed_or_cancelled (data)) 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, address = g_socket_address_enumerator_next_finish (data->enumerator,
result, &error); result, &data->error_info->tmp_error);
if (address == NULL) if (address == NULL)
{ {
if (G_UNLIKELY (data->enumeration_completed)) if (G_UNLIKELY (data->enumeration_completed))
@ -1862,7 +1921,7 @@ g_socket_client_enumerator_callback (GObject *object,
data->enumeration_completed = TRUE; data->enumeration_completed = TRUE;
g_debug ("GSocketClient: Address enumeration completed (out of addresses)"); 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. after that the enumerator is done.
Note that we don't care about cancellation errors because 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) || if ((data->enumerated_at_least_once && !data->connection_attempts && !data->connection_in_progress) ||
!data->enumerated_at_least_once) !data->enumerated_at_least_once)
{ {
g_debug ("GSocketClient: Address enumeration failed: %s", error ? error->message : NULL); g_debug ("GSocketClient: Address enumeration failed: %s",
if (data->last_error) data->error_info->tmp_error ? data->error_info->tmp_error->message : NULL);
{ consider_tmp_error (data->error_info, G_SOCKET_CLIENT_RESOLVING);
g_clear_error (&error); g_assert (data->error_info->best_error);
error = data->last_error; complete_connection_with_error (data, g_steal_pointer (&data->error_info->best_error));
data->last_error = NULL;
}
else
{
g_assert (error);
}
complete_connection_with_error (data, error);
} }
/* Enumeration should never trigger again, drop our ref */ /* 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; data->enumerated_at_least_once = TRUE;
} }
g_clear_error (&data->last_error); socket = create_socket (data->client, address, &data->error_info->tmp_error);
socket = create_socket (data->client, address, &data->last_error);
if (socket == NULL) if (socket == NULL)
{ {
g_object_unref (address); g_object_unref (address);
consider_tmp_error (data->error_info, G_SOCKET_CLIENT_CONNECTING);
enumerator_next_async (data, FALSE); enumerator_next_async (data, FALSE);
return; return;
} }
@ -1978,6 +2028,7 @@ g_socket_client_connect_async (GSocketClient *client,
data = g_slice_new0 (GSocketClientAsyncConnectData); data = g_slice_new0 (GSocketClientAsyncConnectData);
data->client = client; data->client = client;
data->connectable = g_object_ref (connectable); data->connectable = g_object_ref (connectable);
data->error_info = socket_client_error_info_new ();
if (can_use_proxy (client)) if (can_use_proxy (client))
{ {