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

diff --git a/libsoup/soup-connection.c b/libsoup/soup-connection.c
index b730b62..33e8024 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
 }