From a87e5833024a21a4e2aa845e039121377e921738 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 16 May 2010 03:31:38 -0400 Subject: [PATCH] Fix SoupSessionAsync to handle very early aborts better If soup_session_abort() was called while we were doing async DNS, things could get confused and it would end up trying to use a freed SoupSocket. Fix that up by always properly using GCancellables during SoupSocket connection, so that we can cancel any outstanding operations if the socket is destroyed, and add a regression test for that. That then fixes a known leak in misc-test's early-abort case, but makes it crash instead, so we fix that by using a weak pointer in SoupSessionAsync to detect when the session has been destroyed before the callback is invoked. This then creates/reveals additional leaks in that test case, which require additional fixes. The relevant APIs are obviously lousy (in the way most pre-gio async APIs in GNOME were), but can't really be fixed at this point without breaking ABI. To be fixed in the gio-based API... https://bugzilla.gnome.org/show_bug.cgi?id=618641 --- libsoup/soup-connection.c | 23 ++++++++--- libsoup/soup-session-async.c | 18 ++++++++- libsoup/soup-session.c | 7 ++-- libsoup/soup-socket.c | 94 +++++++++++++++++++++++++++++--------------- tests/misc-test.c | 36 +++++++++++++++++ 5 files changed, 134 insertions(+), 44 deletions(-) diff --git a/libsoup/soup-connection.c b/libsoup/soup-connection.c index b730b62..33e8024 100644 --- a/libsoup/soup-connection.c +++ b/libsoup/soup-connection.c @@ -423,7 +423,21 @@ static void socket_connect_result (SoupSocket *sock, guint status, gpointer user_data) { SoupConnectionAsyncConnectData *data = user_data; - SoupConnectionPrivate *priv = SOUP_CONNECTION_GET_PRIVATE (data->conn); + SoupConnectionPrivate *priv; + + if (!data->conn) { + if (data->callback) { + data->callback (NULL, SOUP_STATUS_CANCELLED, + data->callback_data); + } + g_slice_free (SoupConnectionAsyncConnectData, data); + return; + } + + g_object_remove_weak_pointer (G_OBJECT (data->conn), + (gpointer *)&data->conn); + + priv = SOUP_CONNECTION_GET_PRIVATE (data->conn); if (!SOUP_STATUS_IS_SUCCESSFUL (status)) goto done; @@ -477,6 +491,7 @@ soup_connection_connect_async (SoupConnection *conn, data->conn = conn; data->callback = callback; data->callback_data = user_data; + g_object_add_weak_pointer (G_OBJECT (conn), (gpointer *)&data->conn); priv->socket = soup_socket_new (SOUP_SOCKET_REMOTE_ADDRESS, priv->remote_addr, @@ -590,6 +605,7 @@ soup_connection_disconnect (SoupConnection *conn) g_return_if_fail (SOUP_IS_CONNECTION (conn)); priv = SOUP_CONNECTION_GET_PRIVATE (conn); + soup_connection_set_state (conn, SOUP_CONNECTION_DISCONNECTED); if (!priv->socket) return; @@ -599,11 +615,6 @@ soup_connection_disconnect (SoupConnection *conn) g_object_unref (priv->socket); priv->socket = NULL; - /* Don't emit "disconnected" if we aren't yet connected */ - if (priv->state < SOUP_CONNECTION_IDLE) - return; - - soup_connection_set_state (conn, SOUP_CONNECTION_DISCONNECTED); /* NB: this might cause conn to be destroyed. */ g_signal_emit (conn, signals[DISCONNECTED], 0); } diff --git a/libsoup/soup-session-async.c b/libsoup/soup-session-async.c index 53e1c29..b41aab8 100644 --- a/libsoup/soup-session-async.c +++ b/libsoup/soup-session-async.c @@ -253,10 +253,19 @@ tunnel_connect_restarted (SoupMessage *msg, gpointer user_data) } static void -got_connection (SoupConnection *conn, guint status, gpointer session) +got_connection (SoupConnection *conn, guint status, gpointer user_data) { + gpointer *session_p = user_data; + SoupSession *session = *session_p; SoupAddress *tunnel_addr; + if (!session) { + g_slice_free (gpointer, session_p); + return; + } + g_object_remove_weak_pointer (G_OBJECT (session), session_p); + g_slice_free (gpointer, session_p); + if (status != SOUP_STATUS_OK) { /* There may have been messages waiting for the * connection count to go down, so queue a run_queue. @@ -344,8 +353,13 @@ run_queue (SoupSessionAsync *sa) continue; if (soup_connection_get_state (conn) == SOUP_CONNECTION_NEW) { + gpointer *session_p; + + session_p = g_slice_new (gpointer); + *session_p = session; + g_object_add_weak_pointer (G_OBJECT (session), session_p); soup_connection_connect_async (conn, got_connection, - session); + session_p); } else soup_session_send_queue_item (session, item, conn); } diff --git a/libsoup/soup-session.c b/libsoup/soup-session.c index f77ea4c..5143a48 100644 --- a/libsoup/soup-session.c +++ b/libsoup/soup-session.c @@ -1265,11 +1265,10 @@ soup_session_connection_failed (SoupSession *session, if (host) connection_disconnected (conn, session); - else { + else if (conn) host = g_object_get_data (G_OBJECT (conn), "SoupSessionHost"); - if (!host) - return; - } + if (!host) + return; if (status == SOUP_STATUS_TRY_AGAIN) return; diff --git a/libsoup/soup-socket.c b/libsoup/soup-socket.c index 8f961a1..9c8fe87 100644 --- a/libsoup/soup-socket.c +++ b/libsoup/soup-socket.c @@ -95,6 +95,8 @@ typedef struct { GMutex *iolock, *addrlock; guint timeout; + + GCancellable *connect_cancel; } SoupSocketPrivate; #define SOUP_SOCKET_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), SOUP_TYPE_SOCKET, SoupSocketPrivate)) @@ -156,6 +158,8 @@ finalize (GObject *object) { SoupSocketPrivate *priv = SOUP_SOCKET_GET_PRIVATE (object); + if (priv->connect_cancel) + g_object_unref (priv->connect_cancel); if (priv->iochannel) disconnect_internal (priv); @@ -615,6 +619,16 @@ typedef struct { gpointer user_data; } SoupSocketAsyncConnectData; +static void +free_sacd (SoupSocketAsyncConnectData *sacd) +{ + if (sacd->cancel_id) + g_signal_handler_disconnect (sacd->cancellable, sacd->cancel_id); + g_object_unref (sacd->cancellable); + g_object_unref (sacd->sock); + g_slice_free (SoupSocketAsyncConnectData, sacd); +} + static gboolean idle_connect_result (gpointer user_data) { @@ -622,9 +636,8 @@ idle_connect_result (gpointer user_data) SoupSocketPrivate *priv = SOUP_SOCKET_GET_PRIVATE (sacd->sock); guint status; + priv->connect_cancel = NULL; priv->watch_src = NULL; - if (sacd->cancel_id) - g_signal_handler_disconnect (sacd->cancellable, sacd->cancel_id); if (priv->sockfd == -1) { if (g_cancellable_is_cancelled (sacd->cancellable)) @@ -635,7 +648,7 @@ idle_connect_result (gpointer user_data) status = SOUP_STATUS_OK; sacd->callback (sacd->sock, status, sacd->user_data); - g_slice_free (SoupSocketAsyncConnectData, sacd); + free_sacd (sacd); return FALSE; } @@ -684,16 +697,17 @@ static void got_address (SoupAddress *addr, guint status, gpointer user_data) { SoupSocketAsyncConnectData *sacd = user_data; + SoupSocketPrivate *priv = SOUP_SOCKET_GET_PRIVATE (sacd->sock); + + priv->connect_cancel = NULL; - if (!SOUP_STATUS_IS_SUCCESSFUL (status)) { + if (SOUP_STATUS_IS_SUCCESSFUL (status)) { + soup_socket_connect_async (sacd->sock, sacd->cancellable, + sacd->callback, sacd->user_data); + } else sacd->callback (sacd->sock, status, sacd->user_data); - g_slice_free (SoupSocketAsyncConnectData, sacd); - return; - } - soup_socket_connect_async (sacd->sock, sacd->cancellable, - sacd->callback, sacd->user_data); - g_slice_free (SoupSocketAsyncConnectData, sacd); + free_sacd (sacd); } static void @@ -774,15 +788,23 @@ soup_socket_connect_async (SoupSocket *sock, GCancellable *cancellable, g_return_if_fail (priv->remote_addr != NULL); sacd = g_slice_new0 (SoupSocketAsyncConnectData); - sacd->sock = sock; - sacd->cancellable = cancellable; + sacd->sock = g_object_ref (sock); + sacd->cancellable = cancellable ? g_object_ref (cancellable) : g_cancellable_new (); sacd->callback = callback; sacd->user_data = user_data; + priv->connect_cancel = sacd->cancellable; + + if (g_cancellable_is_cancelled (priv->connect_cancel)) { + priv->watch_src = soup_add_completion (priv->async_context, + idle_connect_result, sacd); + return; + } + if (!soup_address_get_sockaddr (priv->remote_addr, NULL)) { soup_address_resolve_async (priv->remote_addr, priv->async_context, - cancellable, + sacd->cancellable, got_address, sacd); return; } @@ -803,12 +825,10 @@ soup_socket_connect_async (SoupSocket *sock, GCancellable *cancellable, priv->timeout * 1000, connect_timeout, sacd); } - if (cancellable) { - sacd->cancel_id = - g_signal_connect (cancellable, "cancelled", - G_CALLBACK (async_cancel), - sacd); - } + sacd->cancel_id = + g_signal_connect (sacd->cancellable, "cancelled", + G_CALLBACK (async_cancel), + sacd); } else { priv->watch_src = soup_add_completion (priv->async_context, idle_connect_result, sacd); @@ -848,28 +868,35 @@ soup_socket_connect_sync (SoupSocket *sock, GCancellable *cancellable) g_return_val_if_fail (priv->sockfd == -1, SOUP_STATUS_MALFORMED); g_return_val_if_fail (priv->remote_addr != NULL, SOUP_STATUS_MALFORMED); + if (cancellable) + g_object_ref (cancellable); + else + cancellable = g_cancellable_new (); + priv->connect_cancel = cancellable; + if (!soup_address_get_sockaddr (priv->remote_addr, NULL)) { status = soup_address_resolve_sync (priv->remote_addr, cancellable); - if (!SOUP_STATUS_IS_SUCCESSFUL (status)) + if (!SOUP_STATUS_IS_SUCCESSFUL (status)) { + priv->connect_cancel = NULL; + g_object_unref (cancellable); return status; + } } - if (cancellable) { - cancel_id = g_signal_connect (cancellable, "cancelled", - G_CALLBACK (sync_cancel), sock); - } + cancel_id = g_signal_connect (cancellable, "cancelled", + G_CALLBACK (sync_cancel), sock); status = socket_connect_internal (sock); + priv->connect_cancel = NULL; - if (cancellable) { - if (status != SOUP_STATUS_OK && - g_cancellable_is_cancelled (cancellable)) { - status = SOUP_STATUS_CANCELLED; - disconnect_internal (priv); - } - g_signal_handler_disconnect (cancellable, cancel_id); + if (status != SOUP_STATUS_OK && + g_cancellable_is_cancelled (cancellable)) { + status = SOUP_STATUS_CANCELLED; + disconnect_internal (priv); } + g_signal_handler_disconnect (cancellable, cancel_id); + g_object_unref (cancellable); return status; } @@ -1080,7 +1107,10 @@ soup_socket_disconnect (SoupSocket *sock) g_return_if_fail (SOUP_IS_SOCKET (sock)); priv = SOUP_SOCKET_GET_PRIVATE (sock); - if (g_mutex_trylock (priv->iolock)) { + if (priv->connect_cancel) { + g_cancellable_cancel (priv->connect_cancel); + return; + } else if (g_mutex_trylock (priv->iolock)) { if (priv->iochannel) disconnect_internal (priv); else diff --git a/tests/misc-test.c b/tests/misc-test.c index c3b5d2d..ae97b99 100644 --- a/tests/misc-test.c +++ b/tests/misc-test.c @@ -194,6 +194,7 @@ do_callback_unref_test (void) soup_address_resolve_sync (addr, NULL); bad_server = soup_server_new (SOUP_SERVER_INTERFACE, addr, NULL); + g_object_unref (addr); bad_uri = g_strdup_printf ("http://127.0.0.1:%u/", soup_server_get_port (bad_server)); @@ -322,6 +323,9 @@ do_msg_reuse_test (void) g_free (signal_ids); } +/* Server handlers for "*" work but are separate from handlers for + * all other URIs. #590751 + */ static void do_star_test (void) { @@ -382,6 +386,19 @@ do_star_test (void) soup_uri_free (star_uri); } +/* Handle unexpectedly-early aborts. #596074, #618641 */ +static void +ea_msg_completed_one (SoupSession *session, SoupMessage *msg, gpointer loop) +{ + debug_printf (2, " Message 1 completed\n"); + if (msg->status_code != SOUP_STATUS_CANCELLED) { + debug_printf (1, " Unexpected status on Message 1: %d %s\n", + msg->status_code, msg->reason_phrase); + errors++; + } + g_main_loop_quit (loop); +} + static gboolean ea_abort_session (gpointer session) { @@ -416,16 +433,32 @@ do_early_abort_test (void) { SoupSession *session; SoupMessage *msg; + GMainContext *context; + GMainLoop *loop; debug_printf (1, "\nAbort with pending connection\n"); session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL); + msg = soup_message_new_from_uri ("GET", base_uri); + context = g_main_context_default (); + loop = g_main_loop_new (context, TRUE); + soup_session_queue_message (session, msg, ea_msg_completed_one, loop); + g_main_context_iteration (context, FALSE); + + soup_session_abort (session); + while (g_main_context_pending (context)) + g_main_context_iteration (context, FALSE); + g_main_loop_unref (loop); + soup_test_session_abort_unref (session); + + session = soup_test_session_new (SOUP_TYPE_SESSION_ASYNC, NULL); msg = soup_message_new_from_uri ("GET", base_uri); g_signal_connect (session, "connection-created", G_CALLBACK (ea_connection_created), NULL); soup_session_send_message (session, msg); + debug_printf (2, " Message 2 completed\n"); if (msg->status_code != SOUP_STATUS_CANCELLED) { debug_printf (1, " Unexpected response: %d %s\n", @@ -434,6 +467,9 @@ do_early_abort_test (void) } g_object_unref (msg); + while (g_main_context_pending (context)) + g_main_context_iteration (context, FALSE); + soup_test_session_abort_unref (session); } -- 2.7.4