[libsoup] SoupConnection: fix a race condition with non-keepalive messages

[Dan Winship <danw src gnome org>]

commit aa8e1dc5127b56bd96e9d3068a6bfeb7c1a07a94
Author: Dan Winship <danw gnome org>
Date:   Tue Oct 2 08:57:52 2012 -0400

    SoupConnection: fix a race condition with non-keepalive messages
    
    When a SoupSession sets a connection back to IDLE on a non-keepalive
    message, the connection then disconnects itself. However, it had been
    briefly setting its state to IDLE before disconnecting. Although this
    wasn't supposed to be observable (it doesn't emit a notification), in
    a SoupSessionSync, it could be observed by another thread, which might
    then try to claim the connection to send another request on, causing
    problems when the first thread then disconnected it.
    
    Fix this by inlining clear_current_item() into
    soup_connection_set_state() and reordering the code to not change
    priv->state until after deciding whether or not it's going to
    disconnect.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=684238

 libsoup/soup-connection.c |   93 ++++++++++++++++++++------------------------
 1 files changed, 42 insertions(+), 51 deletions(-)
---
diff --git a/libsoup/soup-connection.c b/libsoup/soup-connection.c
index d2746e8..1889a94 100644
--- a/libsoup/soup-connection.c
+++ b/libsoup/soup-connection.c
@@ -66,7 +66,6 @@ enum {
 };
 
 static void stop_idle_timer (SoupConnectionPrivate *priv);
-static void clear_current_item (SoupConnection *conn);
 
 /* Number of seconds after which we close a connection that hasn't yet
  * been used.
@@ -99,13 +98,7 @@ soup_connection_dispose (GObject *object)
 	SoupConnectionPrivate *priv = SOUP_CONNECTION_GET_PRIVATE (conn);
 
 	stop_idle_timer (priv);
-	/* Make sure clear_current_item doesn't re-establish the timeout */
-	priv->idle_timeout = 0;
 
-	if (priv->cur_item) {
-		g_warning ("Disposing connection with cur_item set");
-		clear_current_item (conn);
-	}
 	if (priv->socket) {
 		g_warning ("Disposing connection while connected");
 		soup_connection_disconnect (conn);
@@ -417,45 +410,6 @@ set_current_item (SoupConnection *conn, SoupMessageQueueItem *item)
 }
 
 static void
-clear_current_item (SoupConnection *conn)
-{
-	SoupConnectionPrivate *priv = SOUP_CONNECTION_GET_PRIVATE (conn);
-
-	g_object_freeze_notify (G_OBJECT (conn));
-
-	priv->unused_timeout = 0;
-	start_idle_timer (conn);
-
-	if (priv->cur_item) {
-		SoupMessageQueueItem *item;
-
-		item = priv->cur_item;
-		priv->cur_item = NULL;
-		g_object_notify (G_OBJECT (conn), "message");
-
-		g_signal_handlers_disconnect_by_func (item->msg, G_CALLBACK (current_item_restarted), conn);
-
-		if (item->msg->method == SOUP_METHOD_CONNECT &&
-		    SOUP_STATUS_IS_SUCCESSFUL (item->msg->status_code)) {
-			soup_connection_event (conn, G_SOCKET_CLIENT_PROXY_NEGOTIATED, NULL);
-
-			/* We're now effectively no longer proxying */
-			soup_uri_free (priv->proxy_uri);
-			priv->proxy_uri = NULL;
-
-			/* Nor are we actually IDLE... */
-			if (priv->state == SOUP_CONNECTION_IDLE)
-				priv->state = SOUP_CONNECTION_IN_USE;
-		}
-
-		if (!soup_message_is_keepalive (item->msg) || !priv->reusable)
-			soup_connection_disconnect (conn);
-	}
-
-	g_object_thaw_notify (G_OBJECT (conn));
-}
-
-static void
 proxy_socket_event (SoupSocket          *socket,
 		    GSocketClientEvent   event,
 		    GIOStream           *connection,
@@ -938,6 +892,7 @@ void
 soup_connection_set_state (SoupConnection *conn, SoupConnectionState state)
 {
 	SoupConnectionPrivate *priv;
+	SoupConnectionState old_state;
 
 	g_return_if_fail (SOUP_IS_CONNECTION (conn));
 	g_return_if_fail (state >= SOUP_CONNECTION_NEW &&
@@ -946,13 +901,49 @@ soup_connection_set_state (SoupConnection *conn, SoupConnectionState state)
 	g_object_freeze_notify (G_OBJECT (conn));
 
 	priv = SOUP_CONNECTION_GET_PRIVATE (conn);
-	priv->state = state;
-	if (state == SOUP_CONNECTION_IDLE ||
-	    state == SOUP_CONNECTION_DISCONNECTED)
-		clear_current_item (conn);
+	old_state = priv->state;
+
+	if (old_state == SOUP_CONNECTION_IN_USE)
+		priv->unused_timeout = 0;
+
+	if (priv->cur_item) {
+		SoupMessageQueueItem *item;
+
+		g_warn_if_fail (state == SOUP_CONNECTION_IDLE ||
+				state == SOUP_CONNECTION_DISCONNECTED);
+
+		item = priv->cur_item;
+		priv->cur_item = NULL;
+		g_object_notify (G_OBJECT (conn), "message");
+
+		g_signal_handlers_disconnect_by_func (item->msg, G_CALLBACK (current_item_restarted), conn);
+
+		if (item->msg->method == SOUP_METHOD_CONNECT &&
+		    SOUP_STATUS_IS_SUCCESSFUL (item->msg->status_code)) {
+			soup_connection_event (conn, G_SOCKET_CLIENT_PROXY_NEGOTIATED, NULL);
+
+			/* We're now effectively no longer proxying */
+			soup_uri_free (priv->proxy_uri);
+			priv->proxy_uri = NULL;
+
+			/* Nor are we actually IDLE... */
+			if (state == SOUP_CONNECTION_IDLE)
+				state = SOUP_CONNECTION_IN_USE;
+		}
+
+		if (!soup_message_is_keepalive (item->msg) || !priv->reusable)
+			soup_connection_disconnect (conn);
+	}
+
+	if (priv->state == old_state && priv->state != state) {
+		priv->state = state;
+
+		if (state == SOUP_CONNECTION_IDLE)
+			start_idle_timer (conn);
 
-	if (priv->state == state)
 		g_object_notify (G_OBJECT (conn), "state");
+	}
+
 	g_object_thaw_notify (G_OBJECT (conn));
 }