aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDaniel Stenberg <daniel@haxx.se>2019-12-09 11:53:54 +0100
committerDaniel Stenberg <daniel@haxx.se>2019-12-09 15:30:09 +0100
commitee263de7a378e701f15e58879f36fdcfe8742006 (patch)
tree5fdf1aafe027c7e659cfaa989f429b5159645a79
parent9e891ff54de34d0e4c9aec502eb53e5b64a6dd1f (diff)
conncache: fix multi-thread use of shared connection cache
It could accidentally let the connection get used by more than one thread, leading to double-free and more. Reported-by: Christopher Reid Fixes #4544 Closes #4557
-rw-r--r--lib/conncache.c30
-rw-r--r--lib/conncache.h24
-rw-r--r--lib/http.c2
-rw-r--r--lib/http2.c5
-rw-r--r--lib/http2.h2
-rwxr-xr-xlib/multi.c20
-rw-r--r--lib/url.c21
-rw-r--r--tests/data/test15546
8 files changed, 58 insertions, 52 deletions
diff --git a/lib/conncache.c b/lib/conncache.c
index 57d6061fd..a23244cf6 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -40,27 +40,6 @@
#include "curl_memory.h"
#include "memdebug.h"
-#ifdef CURLDEBUG
-/* the debug versions of these macros make extra certain that the lock is
- never doubly locked or unlocked */
-#define CONN_LOCK(x) if((x)->share) { \
- Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE); \
- DEBUGASSERT(!(x)->state.conncache_lock); \
- (x)->state.conncache_lock = TRUE; \
- }
-
-#define CONN_UNLOCK(x) if((x)->share) { \
- DEBUGASSERT((x)->state.conncache_lock); \
- (x)->state.conncache_lock = FALSE; \
- Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT); \
- }
-#else
-#define CONN_LOCK(x) if((x)->share) \
- Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE)
-#define CONN_UNLOCK(x) if((x)->share) \
- Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT)
-#endif
-
#define HASHKEY_SIZE 128
static void conn_llist_dtor(void *user, void *element)
@@ -122,6 +101,7 @@ static int bundle_remove_conn(struct connectbundle *cb_ptr,
}
curr = curr->next;
}
+ DEBUGASSERT(0);
return 0;
}
@@ -428,17 +408,15 @@ conncache_find_first_connection(struct conncache *connc)
*
* Return TRUE if stored, FALSE if closed.
*/
-bool Curl_conncache_return_conn(struct connectdata *conn)
+bool Curl_conncache_return_conn(struct Curl_easy *data,
+ struct connectdata *conn)
{
- struct Curl_easy *data = conn->data;
-
/* data->multi->maxconnects can be negative, deal with it. */
size_t maxconnects =
(data->multi->maxconnects < 0) ? data->multi->num_easy * 4:
data->multi->maxconnects;
struct connectdata *conn_candidate = NULL;
- conn->data = NULL; /* no owner anymore */
conn->lastused = Curl_now(); /* it was used up until now */
if(maxconnects > 0 &&
Curl_conncache_size(data) > maxconnects) {
@@ -541,7 +519,7 @@ Curl_conncache_extract_oldest(struct Curl_easy *data)
while(curr) {
conn = curr->ptr;
- if(!CONN_INUSE(conn) && !conn->data) {
+ if(!CONN_INUSE(conn) && !conn->data && !conn->bits.close) {
/* Set higher score for the age passed since the connection was used */
score = Curl_timediff(now, conn->lastused);
diff --git a/lib/conncache.h b/lib/conncache.h
index 58f902409..5fe80b4c8 100644
--- a/lib/conncache.h
+++ b/lib/conncache.h
@@ -42,6 +42,27 @@ struct conncache {
#define BUNDLE_UNKNOWN 0 /* initial value */
#define BUNDLE_MULTIPLEX 2
+#ifdef CURLDEBUG
+/* the debug versions of these macros make extra certain that the lock is
+ never doubly locked or unlocked */
+#define CONN_LOCK(x) if((x)->share) { \
+ Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE); \
+ DEBUGASSERT(!(x)->state.conncache_lock); \
+ (x)->state.conncache_lock = TRUE; \
+ }
+
+#define CONN_UNLOCK(x) if((x)->share) { \
+ DEBUGASSERT((x)->state.conncache_lock); \
+ (x)->state.conncache_lock = FALSE; \
+ Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT); \
+ }
+#else
+#define CONN_LOCK(x) if((x)->share) \
+ Curl_share_lock((x), CURL_LOCK_DATA_CONNECT, CURL_LOCK_ACCESS_SINGLE)
+#define CONN_UNLOCK(x) if((x)->share) \
+ Curl_share_unlock((x), CURL_LOCK_DATA_CONNECT)
+#endif
+
struct connectbundle {
int multiuse; /* supports multi-use */
size_t num_connections; /* Number of connections in the bundle */
@@ -61,7 +82,8 @@ void Curl_conncache_unlock(struct Curl_easy *data);
size_t Curl_conncache_size(struct Curl_easy *data);
size_t Curl_conncache_bundle_size(struct connectdata *conn);
-bool Curl_conncache_return_conn(struct connectdata *conn);
+bool Curl_conncache_return_conn(struct Curl_easy *data,
+ struct connectdata *conn);
CURLcode Curl_conncache_add_conn(struct conncache *connc,
struct connectdata *conn) WARN_UNUSED_RESULT;
void Curl_conncache_remove_conn(struct Curl_easy *data,
diff --git a/lib/http.c b/lib/http.c
index 885704560..35d2e9fb0 100644
--- a/lib/http.c
+++ b/lib/http.c
@@ -1617,7 +1617,7 @@ CURLcode Curl_http_done(struct connectdata *conn,
Curl_add_buffer_free(&http->send_buffer);
}
- Curl_http2_done(conn, premature);
+ Curl_http2_done(data, premature);
Curl_quic_done(data, premature);
Curl_mime_cleanpart(&http->form);
diff --git a/lib/http2.c b/lib/http2.c
index b741aed48..65f3513ee 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -1169,11 +1169,10 @@ static void populate_settings(struct connectdata *conn,
httpc->local_settings_num = 3;
}
-void Curl_http2_done(struct connectdata *conn, bool premature)
+void Curl_http2_done(struct Curl_easy *data, bool premature)
{
- struct Curl_easy *data = conn->data;
struct HTTP *http = data->req.protop;
- struct http_conn *httpc = &conn->proto.httpc;
+ struct http_conn *httpc = &data->conn->proto.httpc;
/* there might be allocated resources done before this got the 'h2' pointer
setup */
diff --git a/lib/http2.h b/lib/http2.h
index 93058ccb3..12d36eef9 100644
--- a/lib/http2.h
+++ b/lib/http2.h
@@ -50,7 +50,7 @@ CURLcode Curl_http2_switched(struct connectdata *conn,
/* called from http_setup_conn */
void Curl_http2_setup_conn(struct connectdata *conn);
void Curl_http2_setup_req(struct Curl_easy *data);
-void Curl_http2_done(struct connectdata *conn, bool premature);
+void Curl_http2_done(struct Curl_easy *data, bool premature);
CURLcode Curl_http2_done_sending(struct connectdata *conn);
CURLcode Curl_http2_add_child(struct Curl_easy *parent,
struct Curl_easy *child,
diff --git a/lib/multi.c b/lib/multi.c
index f30e41a65..1fa6b092b 100755
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -547,6 +547,8 @@ static CURLcode multi_done(struct Curl_easy *data,
/* Stop if multi_done() has already been called */
return CURLE_OK;
+ conn->data = data; /* ensure the connection uses this transfer now */
+
/* Stop the resolver and free its own resources (but not dns_entry yet). */
Curl_resolver_kill(conn);
@@ -583,15 +585,17 @@ static CURLcode multi_done(struct Curl_easy *data,
process_pending_handles(data->multi); /* connection / multiplex */
+ CONN_LOCK(data);
detach_connnection(data);
if(CONN_INUSE(conn)) {
/* Stop if still used. */
+ CONN_UNLOCK(data);
DEBUGF(infof(data, "Connection still in use %zu, "
"no more multi_done now!\n",
conn->easyq.size));
return CURLE_OK;
}
-
+ conn->data = NULL; /* the connection now has no owner */
data->state.done = TRUE; /* called just now! */
if(conn->dns_entry) {
@@ -634,7 +638,10 @@ static CURLcode multi_done(struct Curl_easy *data,
#endif
) || conn->bits.close
|| (premature && !(conn->handler->flags & PROTOPT_STREAM))) {
- CURLcode res2 = Curl_disconnect(data, conn, premature);
+ CURLcode res2;
+ conn->bits.close = TRUE; /* forcibly prevents reuse */
+ CONN_UNLOCK(data);
+ res2 = Curl_disconnect(data, conn, premature);
/* If we had an error already, make sure we return that one. But
if we got a new error, return that. */
@@ -651,9 +658,9 @@ static CURLcode multi_done(struct Curl_easy *data,
conn->bits.httpproxy ? conn->http_proxy.host.dispname :
conn->bits.conn_to_host ? conn->conn_to_host.dispname :
conn->host.dispname);
-
/* the connection is no longer in use by this transfer */
- if(Curl_conncache_return_conn(conn)) {
+ CONN_UNLOCK(data);
+ if(Curl_conncache_return_conn(data, conn)) {
/* remember the most recently used connection */
data->state.lastconnect = conn;
infof(data, "%s\n", buffer);
@@ -760,10 +767,8 @@ CURLMcode curl_multi_remove_handle(struct Curl_multi *multi,
vanish with this handle */
/* Remove the association between the connection and the handle */
- if(data->conn) {
- data->conn->data = NULL;
+ if(data->conn)
detach_connnection(data);
- }
#ifdef USE_LIBPSL
/* Remove the PSL association. */
@@ -1349,6 +1354,7 @@ static CURLcode multi_do(struct Curl_easy *data, bool *done)
DEBUGASSERT(conn);
DEBUGASSERT(conn->handler);
+ DEBUGASSERT(conn->data == data);
if(conn->handler->do_it) {
/* generic protocol-specific function pointer set in curl_connect() */
diff --git a/lib/url.c b/lib/url.c
index 1af9f90e0..4111eec3a 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -1082,16 +1082,15 @@ ConnectionExists(struct Curl_easy *data,
check = curr->ptr;
curr = curr->next;
- if(check->bits.connect_only)
- /* connect-only connections will not be reused */
+ if(check->bits.connect_only || check->bits.close)
+ /* connect-only or to-be-closed connections will not be reused */
continue;
multiplexed = CONN_INUSE(check) &&
(bundle->multiuse == BUNDLE_MULTIPLEX);
if(canmultiplex) {
- if(check->bits.protoconnstart && check->bits.close)
- continue;
+ ;
}
else {
if(multiplexed) {
@@ -1111,12 +1110,9 @@ ConnectionExists(struct Curl_easy *data,
}
}
- if((check->sock[FIRSTSOCKET] == CURL_SOCKET_BAD) ||
- check->bits.close) {
- if(!check->bits.close)
- foundPendingCandidate = TRUE;
- /* Don't pick a connection that hasn't connected yet or that is going
- to get closed. */
+ if(check->sock[FIRSTSOCKET] == CURL_SOCKET_BAD) {
+ foundPendingCandidate = TRUE;
+ /* Don't pick a connection that hasn't connected yet */
infof(data, "Connection #%ld isn't open enough, can't reuse\n",
check->connection_id);
continue;
@@ -1194,8 +1190,7 @@ ConnectionExists(struct Curl_easy *data,
already in use so we skip it */
continue;
- if(CONN_INUSE(check) && check->data &&
- (check->data->multi != needle->data->multi))
+ if(check->data && (check->data->multi != needle->data->multi))
/* this could be subject for multiplex use, but only if they belong to
* the same multi handle */
continue;
@@ -1643,6 +1638,7 @@ static struct connectdata *allocate_conn(struct Curl_easy *data)
it may live on without (this specific) Curl_easy */
conn->fclosesocket = data->set.fclosesocket;
conn->closesocket_client = data->set.closesocket_client;
+ conn->lastused = Curl_now(); /* used now */
return conn;
error:
@@ -3612,7 +3608,6 @@ static CURLcode create_conn(struct Curl_easy *data,
reuse = FALSE;
infof(data, "We can reuse, but we want a new connection anyway\n");
- Curl_conncache_return_conn(conn_temp);
}
}
}
diff --git a/tests/data/test1554 b/tests/data/test1554
index be48e02eb..06f189724 100644
--- a/tests/data/test1554
+++ b/tests/data/test1554
@@ -38,6 +38,8 @@ run 1: foobar and so on fun!
<- Mutex unlock
-> Mutex lock
<- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
run 1: foobar and so on fun!
-> Mutex lock
<- Mutex unlock
@@ -47,6 +49,8 @@ run 1: foobar and so on fun!
<- Mutex unlock
-> Mutex lock
<- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
run 1: foobar and so on fun!
-> Mutex lock
<- Mutex unlock
@@ -54,6 +58,8 @@ run 1: foobar and so on fun!
<- Mutex unlock
-> Mutex lock
<- Mutex unlock
+-> Mutex lock
+<- Mutex unlock
</datacheck>
</reply>