GNOME.org
 

[libsoup] Fix SoupSessionAsync to handle very early aborts better

commit a87e5833024a21a4e2aa845e039121377e921738
Author: Dan Winship <danw gnome org>
Date:   Sun May 16 03:31:38 2010 -0400

    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);
 }
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]