luc14n0/glib
1
0
Fork 0
mirror of https://gitlab.gnome.org/GNOME/glib.git synced 2025-08-23 17:38:54 +02:00
Code Issues Packages Projects Releases Wiki Activity

gsocketlistener: Fix infinite blocking when accepting connections

Browse Source 
As the new comments in the code try to explain, this fixes infinite
blocking which could happen when calling
`g_socket_listener_accept_async()` multiple times in parallel, with more
parallel calls than there are pending incoming connections on any of the
`GSocket`s in the `GSocketListener`.

The way `g_socket_listener_accept_async()` works is to create a set of
`GSocketSource`s when it’s called, one for each of the `GSocket`s in the
`GSocketListener`. Those sources are attached to the main context,
polling for `G_IO_IN` (indicating that the socket has a pending incoming
connection to accept).

When one of the socket sources polls ready, `g_socket_accept()` is
called on it, and a new connection is created.

If there are multiple pending `g_socket_listener_accept_async()` calls,
there are correspondingly multiple `GSocketSource` sources for each
`GSocket` in the `GSocketListener`. They will all poll ready in a single
`GMainContext` iteration. The first one to be dispatched will
successfully call `g_socket_accept()`, and subsequent ones to dispatch
will do likewise until there are no more pending incoming connections.
At that point, any remaining socket sources polling ready in that
`GMainContext` iteration will call `g_socket_accept()` on a socket which
is *not* ready to accept, and that will block indefinitely, because
`GSocket` has its own blocking layer on top of `poll()`.

This is not great.

It seems like a better approach would be to disable `GSocket`’s blocking
code, because `GSocketListener` is using `poll()` directly. We only need
one source of poll truth. So, do that.

Unfortunately, that’s complicated by the fact that
`g_socket_listener_add_socket()` allows third party code to provide its
own `GSocket`s to listen on. We probably can’t unilaterally change those
to non-blocking mode, so users of that API will get what they ask for.
That might include blocking indefinitely. I’ve adjusted the
documentation to mention that, at least.

The changes are fairly simple; the accompanying unit test is less
simple. Shrug. It tests for the scenario fixed by this commit, plus the
scenario fixed by the previous commit.

Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3739
This commit is contained in:
Philip Withnall
2025-07-25 15:55:34 +01:00
parent 5ccce023de
commit 8b82fb34c9
2 changed files with 182 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
gio/gsocketlistener.c
View File

@@ -242,7 +242,8 @@ check_listener (GSocketListener *listener,
static void
add_socket (GSocketListener *listener,
GSocket *socket,
GObject *source_object)
GObject *source_object,
gboolean set_non_blocking)
{
if (source_object)
g_object_set_qdata_full (G_OBJECT (socket), source_quark,
@@ -250,6 +251,18 @@ add_socket (GSocketListener *listener,
g_object_unref);
g_ptr_array_add (listener->priv->sockets, g_object_ref (socket));
/* Because the implementation of `GSocketListener` uses polling and `GSource`s
* to wait for connections, we absolutely do *not* need `GSocket`’s internal
* implementation of blocking operations to get in the way. Otherwise we end
* up calling poll() on the results of poll(), which is racy and confusing.
*
* Unfortunately, the existence of g_socket_listener_add_socket() to add a
* socket which is used elsewhere, means that we need an escape hatch
* (`!set_non_blocking`) to allow sockets to remain in blocking mode if the
* caller really wants it. */
if (set_non_blocking)
g_socket_set_blocking (socket, FALSE);
}
/**
@@ -263,6 +276,9 @@ add_socket (GSocketListener *listener,
* new clients from. The socket must be bound to a local
* address and listened to.
*
* For parallel calls to [class@Gio.SocketListener] methods to work, the socket
* must be in non-blocking mode. (See [property@Gio.Socket:blocking].)
*
* @source_object will be passed out in the various calls
* to accept to identify this particular source, which is
* useful if you're listening on multiple addresses and do
@@ -295,7 +311,7 @@ g_socket_listener_add_socket (GSocketListener *listener,
return FALSE;
}
add_socket (listener, socket, source_object);
add_socket (listener, socket, source_object, FALSE);
if (G_SOCKET_LISTENER_GET_CLASS (listener)->changed)
G_SOCKET_LISTENER_GET_CLASS (listener)->changed (listener);
@@ -609,10 +625,10 @@ g_socket_listener_add_inet_port (GSocketListener *listener,
g_assert (socket6 != NULL || socket4 != NULL);
if (socket6 != NULL)
add_socket (listener, socket6, source_object);
add_socket (listener, socket6, source_object, TRUE);
if (socket4 != NULL)
add_socket (listener, socket4, source_object);
add_socket (listener, socket4, source_object, TRUE);
if (G_SOCKET_LISTENER_GET_CLASS (listener)->changed)
G_SOCKET_LISTENER_GET_CLASS (listener)->changed (listener);
@@ -848,6 +864,30 @@ accept_ready (GSocket *accept_socket,
}
else
{
/* This can happen if there are multiple pending g_socket_listener_accept_async()
* calls (say N), and fewer queued incoming connections (say C) than that
* on this `GSocket`, in a single `GMainContext` iteration.
* (There may still be queued incoming connections on other `GSocket`s in
* the `GSocketListener`.)
*
* If so, the `GSocketSource`s for that pending g_socket_listener_accept_async()
* call will all raise `G_IO_IN` in this `GMainContext` iteration. The
* first C calls to accept_ready() succeed, but the following N-C calls
* would block, as all the queued incoming connections on this `GSocket`
* have been accepted by then. The `GSocketSource`s for these remaining
* g_socket_listener_accept_async() calls will not have been destroyed,
* because there’s an independent set of `GSocketSource`s for each
* g_socket_listener_accept_async() call, rather than one `GSocketSource`
* for each socket in the `GSocketListener`.
*
* This is why we need sockets in the `GSocketListener` to be non-blocking:
* otherwise the above g_socket_accept() call would block. */
if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK))
{
g_clear_error (&error);
return G_SOURCE_CONTINUE;
}
g_task_return_error (task, error);
}
@@ -1268,7 +1308,7 @@ g_socket_listener_add_any_inet_port (GSocketListener *listener,
g_signal_emit (listener, signals[EVENT], 0,
G_SOCKET_LISTENER_LISTENED, socket6);
add_socket (listener, socket6, source_object);
add_socket (listener, socket6, source_object, TRUE);
}
else
{
@@ -1288,7 +1328,7 @@ g_socket_listener_add_any_inet_port (GSocketListener *listener,
g_signal_emit (listener, signals[EVENT], 0,
G_SOCKET_LISTENER_LISTENED, socket4);
add_socket (listener, socket4, source_object);
add_socket (listener, socket4, source_object, TRUE);
}
else
{

137
gio/tests/socket-listener.c
View File

@@ -23,13 +23,13 @@
#include "config.h"
#include <gio/gio.h>
#include <stdint.h>
#ifdef HAVE_RTLD_NEXT
#include <dlfcn.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <netinet/ip.h>
#include <stdint.h>
#include <sys/socket.h>
#endif
@@ -549,6 +549,140 @@ test_add_inet_port_listen_failures (void)
#endif
}
static void
async_result_cb (GObject *source_object,
GAsyncResult *result,
void *user_data)
{
GAsyncResult **result_out = user_data;
g_assert (*result_out == NULL);
*result_out = g_object_ref (result);
g_main_context_wakeup (NULL);
}
static gboolean
any_are_null (const void * const *ptr_array,
size_t n_elements)
{
for (size_t i = 0; i < n_elements; i++)
if (ptr_array[i] == NULL)
return TRUE;
return FALSE;
}
static void
test_accept_multi_simultaneously (void)
{
GSocketListener *listener = NULL;
GAsyncResult *accept_results[5] = { NULL, };
struct
{
uint16_t listening_port;
GSocketClient *client;
GAsyncResult *result;
GSocketConnection *connection;
}
clients[5] = { { 0, NULL, NULL, NULL }, };
GSocketConnection *server_connection = NULL;
GCancellable *cancellable = NULL;
GError *local_error = NULL;
g_test_summary ("Test that accepting multiple pending connections on the "
"same GMainContext iteration works");
g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/3739");
G_STATIC_ASSERT (G_N_ELEMENTS (clients) >= 2);
G_STATIC_ASSERT (G_N_ELEMENTS (accept_results) == G_N_ELEMENTS (clients));
listener = g_socket_listener_new ();
cancellable = g_cancellable_new ();
/* Listen on several ports at once. */
for (size_t i = 0; i < G_N_ELEMENTS (clients); i++)
{
clients[i].listening_port = g_socket_listener_add_any_inet_port (listener, NULL, &local_error);
g_assert_no_error (local_error);
}
/* Start to accept a connection, but don’t iterate the `GMainContext` yet. */
g_socket_listener_accept_async (listener, cancellable, async_result_cb, &accept_results[0]);
/* Connect to multiple ports before iterating the `GMainContext`, so that
* multiple sockets are ready in the first iteration. */
for (size_t i = 0; i < G_N_ELEMENTS (clients); i++)
{
clients[i].client = g_socket_client_new ();
g_socket_client_connect_to_host_async (clients[i].client,
"localhost", clients[i].listening_port,
cancellable, async_result_cb, &clients[i].result);
}
while (accept_results[0] == NULL)
g_main_context_iteration (NULL, TRUE);
/* Exactly one server connection should have been created, because we called
* g_socket_listener_accept_async() once. */
server_connection = g_socket_listener_accept_finish (listener, accept_results[0], NULL,
&local_error);
g_assert_no_error (local_error);
g_assert_nonnull (server_connection);
g_io_stream_close (G_IO_STREAM (server_connection), NULL, NULL);
g_clear_object (&server_connection);
/* Conversely, all the client connection requests should have succeeded
* because the kernel will queue them on the server side. */
for (size_t i = 0; i < G_N_ELEMENTS (clients); i++)
{
g_assert_nonnull (clients[i].result);
clients[i].connection = g_socket_client_connect_to_host_finish (clients[i].client,
clients[i].result,
&local_error);
g_assert_no_error (local_error);
g_assert_nonnull (clients[i].connection);
}
/* Accept the remaining connections. */
for (size_t i = 1; i < G_N_ELEMENTS (accept_results); i++)
g_socket_listener_accept_async (listener, cancellable, async_result_cb, &accept_results[i]);
while (any_are_null ((const void * const *) accept_results, G_N_ELEMENTS (accept_results)))
g_main_context_iteration (NULL, TRUE);
for (size_t i = 1; i < G_N_ELEMENTS (accept_results); i++)
{
server_connection = g_socket_listener_accept_finish (listener, accept_results[i], NULL,
&local_error);
g_assert_no_error (local_error);
g_assert_nonnull (server_connection);
g_io_stream_close (G_IO_STREAM (server_connection), NULL, NULL);
g_clear_object (&server_connection);
}
/* Clean up. */
g_socket_listener_close (listener);
g_cancellable_cancel (cancellable);
while (g_main_context_iteration (NULL, FALSE));
for (size_t i = 0; i < G_N_ELEMENTS (clients); i++)
{
g_io_stream_close (G_IO_STREAM (clients[i].connection), NULL, NULL);
g_clear_object (&clients[i].connection);
g_clear_object (&clients[i].result);
g_assert_finalize_object (clients[i].client);
}
for (size_t i = 0; i < G_N_ELEMENTS (accept_results); i++)
g_clear_object (&accept_results[i]);
g_assert_finalize_object (listener);
g_clear_object (&cancellable);
}
int
main (int argc,
char *argv[])
@@ -556,6 +690,7 @@ main (int argc,
g_test_init (&argc, &argv, NULL);
g_test_add_func ("/socket-listener/event-signal", test_event_signal);
g_test_add_func ("/socket-listener/accept/multi-simultaneously", test_accept_multi_simultaneously);
g_test_add_func ("/socket-listener/add-any-inet-port/listen-failures", test_add_any_inet_port_listen_failures);
g_test_add_func ("/socket-listener/add-inet-port/listen-failures", test_add_inet_port_listen_failures);