Mercurial > dovecot > core-2.2

changeset 20348:3f0091573aa3

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
lib-http: client: Made peer object reference-counted to prevent invalid memory access in request handling routine. Resetting the peer->handling_requests flag risked triggering a segfault, since the peer object could be deleted from within the request handler loop.
author Stephan Bosch <stephan@dovecot.fi>
date Sat, 21 May 2016 00:16:21 +0200
parents 7e8ed1fa25bd
children c5983d711b33
files src/lib-http/http-client-peer.c src/lib-http/http-client-private.h src/lib-http/http-client.c
diffstat 3 files changed, 70 insertions(+), 37 deletions(-) [+]
line wrap: on
line diff
--- a/src/lib-http/http-client-peer.c	Sat May 21 13:16:08 2016 +0200
+++ b/src/lib-http/http-client-peer.c	Sat May 21 00:16:21 2016 +0200
@@ -128,7 +128,7 @@
 	timeout_remove(&peer->to_backoff);
 
 	if (array_count(&peer->queues) == 0) {
-		http_client_peer_free(&peer);
+		http_client_peer_close(&peer);
 		return;
 	}
 
@@ -183,23 +183,6 @@
 	return FALSE;
 }
 
-static void
-http_client_peer_disconnect(struct http_client_peer *peer)
-{
-	struct http_client_connection **conn;
-	ARRAY_TYPE(http_client_connection) conns;
-
-	http_client_peer_debug(peer, "Peer disconnect");
-
-	/* make a copy of the connection array; freed connections modify it */
-	t_array_init(&conns, array_count(&peer->conns));
-	array_copy(&conns.arr, 0, &peer->conns.arr, 0, array_count(&peer->conns));
-	array_foreach_modifiable(&conns, conn) {
-		http_client_connection_unref(conn);
-	}
-	i_assert(array_count(&peer->conns) == 0);
-}
-
 static unsigned int
 http_client_peer_requests_pending(struct http_client_peer *peer,
 				  unsigned int *num_urgent_r)
@@ -225,7 +208,7 @@
 	if (array_count(&peer->conns) == 0 &&
 		http_client_peer_requests_pending(peer, &num_urgent) == 0) {
 		/* no connections or pending requests; die immediately */
-		http_client_peer_free(&peer);
+		http_client_peer_close(&peer);
 		return;
 	}
 
@@ -247,6 +230,7 @@
 	struct _conn_available *conn_avail_idx;
 	unsigned int connecting, closing, idle;
 	unsigned int num_pending, num_urgent, new_connections, 	working_conn_count;
+	struct http_client_peer *tmp_peer;
 	bool statistics_dirty = TRUE;
 
 	/* FIXME: limit the number of requests handled in one run to prevent
@@ -258,7 +242,7 @@
 		http_client_peer_debug(peer,
 			"Peer no longer used; will now disconnect "
 			"(%u connections exist)", array_count(&peer->conns));
-		http_client_peer_disconnect(peer);
+		http_client_peer_close(&peer);
 		return;
 	}
 
@@ -272,6 +256,7 @@
 		return;
 	}
 
+	http_client_peer_ref(peer);
 	peer->handling_requests = TRUE;
 	t_array_init(&conns_avail, array_count(&peer->conns));
 	do {
@@ -364,6 +349,10 @@
 			break;
 		}
 	} while (statistics_dirty);
+
+	tmp_peer = peer;
+	if (!http_client_peer_unref(&tmp_peer))
+		return;
 	peer->handling_requests = FALSE;
 
 	if (num_pending == 0)
@@ -494,6 +483,7 @@
 	struct http_client_peer *peer;
 
 	peer = i_new(struct http_client_peer, 1);
+	peer->refcount = 1;
 	peer->client = client;
 	peer->addr = *addr;
 
@@ -528,33 +518,72 @@
 	return peer;
 }
 
-void http_client_peer_free(struct http_client_peer **_peer)
+static void
+http_client_peer_disconnect(struct http_client_peer *peer)
 {
-	struct http_client_peer *peer = *_peer;
+	struct http_client_connection **conn;
+	ARRAY_TYPE(http_client_connection) conns;
 
-	*_peer = NULL;
+	if (peer->disconnected)
+		return;
+	peer->disconnected = TRUE;
 
-	if (peer->destroyed)
-		return;
-	peer->destroyed = TRUE;
+	http_client_peer_debug(peer, "Peer disconnect");
 
-	http_client_peer_debug(peer, "Peer destroy");
+	/* make a copy of the connection array; freed connections modify it */
+	t_array_init(&conns, array_count(&peer->conns));
+	array_copy(&conns.arr, 0, &peer->conns.arr, 0, array_count(&peer->conns));
+	array_foreach_modifiable(&conns, conn) {
+		http_client_connection_unref(conn);
+	}
+	i_assert(array_count(&peer->conns) == 0);
 
 	if (peer->to_req_handling != NULL)
 		timeout_remove(&peer->to_req_handling);
 	if (peer->to_backoff != NULL)
 		timeout_remove(&peer->to_backoff);
 
-	http_client_peer_disconnect(peer);
-	array_free(&peer->conns);
-	array_free(&peer->queues);
-
 	hash_table_remove
 		(peer->client->peers, (const struct http_client_peer_addr *)&peer->addr);
 	DLLIST_REMOVE(&peer->client->peers_list, peer);
+}
 
+void http_client_peer_ref(struct http_client_peer *peer)
+{
+	peer->refcount++;
+}
+
+bool http_client_peer_unref(struct http_client_peer **_peer)
+{
+	struct http_client_peer *peer = *_peer;
+
+	i_assert(peer->refcount > 0);
+
+	*_peer = NULL;
+
+	if (--peer->refcount > 0)
+		return TRUE;
+
+	http_client_peer_debug(peer, "Peer destroy");
+
+	http_client_peer_disconnect(peer);
+
+	array_free(&peer->conns);
+	array_free(&peer->queues);
 	i_free(peer->addr_name);
 	i_free(peer);
+	return FALSE;
+}
+
+void http_client_peer_close(struct http_client_peer **_peer)
+{
+	struct http_client_peer *peer = *_peer;
+
+	http_client_peer_debug(peer, "Peer close");
+
+	http_client_peer_disconnect(peer);
+
+	(void)http_client_peer_unref(_peer);
 }
 
 struct http_client_peer *
@@ -604,7 +633,7 @@
 					http_client_peer_trigger_request_handler(peer);
 				} else {
 					/* drop peer immediately */
-					http_client_peer_free(&peer);
+					http_client_peer_close(&peer);
 				}
 			}
 			return;
@@ -702,7 +731,7 @@
 	   connect itself fails, http_client_peer_connection_failure() is
 	   called instead. */
 
-	if (peer->destroyed)
+	if (peer->disconnected)
 		return;
 
 	http_client_peer_debug(peer, "Lost a connection (%d connections left)",
@@ -716,7 +745,7 @@
 	/* check if peer is still relevant */
 	if (array_count(&peer->conns) == 0 &&
 		http_client_peer_requests_pending(peer, &num_urgent) == 0) {
-		http_client_peer_free(&peer);
+		http_client_peer_close(&peer);
 		return;
 	}
 
--- a/src/lib-http/http-client-private.h	Sat May 21 13:16:08 2016 +0200
+++ b/src/lib-http/http-client-private.h	Sat May 21 00:16:21 2016 +0200
@@ -165,6 +165,7 @@
 };
 
 struct http_client_peer {
+	unsigned int refcount;
 	struct http_client_peer_addr addr;
 	char *addr_name;
 
@@ -185,7 +186,7 @@
 	struct timeout *to_backoff;
 	unsigned int backoff_time_msecs;
 
-	unsigned int destroyed:1;        /* peer is being destroyed */
+	unsigned int disconnected:1;     /* peer is already disconnected */
 	unsigned int no_payload_sync:1;  /* expect: 100-continue failed before */
 	unsigned int seen_100_response:1;/* expect: 100-continue succeeded before */
 	unsigned int allows_pipelining:1;/* peer is known to allow persistent
@@ -337,7 +338,10 @@
 struct http_client_peer *
 	http_client_peer_get(struct http_client *client,
 		const struct http_client_peer_addr *addr);
-void http_client_peer_free(struct http_client_peer **_peer);
+void http_client_peer_ref(struct http_client_peer *peer);
+bool http_client_peer_unref(struct http_client_peer **_peer);
+void http_client_peer_close(struct http_client_peer **_peer);
+
 bool http_client_peer_have_queue(struct http_client_peer *peer,
 				struct http_client_queue *queue);
 void http_client_peer_link_queue(struct http_client_peer *peer,
--- a/src/lib-http/http-client.c	Sat May 21 13:16:08 2016 +0200
+++ b/src/lib-http/http-client.c	Sat May 21 00:16:21 2016 +0200
@@ -187,7 +187,7 @@
 	/* free peers */
 	while (client->peers_list != NULL) {
 		peer = client->peers_list;
-		http_client_peer_free(&peer);
+		http_client_peer_close(&peer);
 	}
 	hash_table_destroy(&client->peers);