From 07cb27c98e92649e74a312faf976271fa7da609c Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sat, 2 Dec 2017 14:27:00 +0100 Subject: conncache: fix several lock issues If the lock is released before the dealings with the bundle is over, it may have changed by another thread in the mean time. Fixes #2132 Fixes #2151 Closes #2139 --- lib/url.c | 174 +++++++++++++++++++++++++++----------------------------------- 1 file changed, 77 insertions(+), 97 deletions(-) (limited to 'lib/url.c') diff --git a/lib/url.c b/lib/url.c index 731e67e17..b38e88eec 100644 --- a/lib/url.c +++ b/lib/url.c @@ -127,10 +127,6 @@ bool curl_win32_idn_to_ascii(const char *in, char **out); #include "curl_memory.h" #include "memdebug.h" -/* Local static prototypes */ -static struct connectdata * -find_oldest_idle_connection_in_bundle(struct Curl_easy *data, - struct connectbundle *bundle); static void conn_free(struct connectdata *conn); static void free_fixed_hostname(struct hostname *host); static void signalPipeClose(struct curl_llist *pipeline, bool pipe_broke); @@ -722,7 +718,6 @@ static void conn_free(struct connectdata *conn) #ifdef USE_SSL Curl_safefree(conn->ssl_extra); #endif - free(conn); /* free all the connection oriented data */ } @@ -777,7 +772,7 @@ CURLcode Curl_disconnect(struct connectdata *conn, bool dead_connection) /* unlink ourselves! */ infof(data, "Closing connection %ld\n", conn->connection_id); - Curl_conncache_remove_conn(data->state.conn_cache, conn); + Curl_conncache_remove_conn(conn, TRUE); free_fixed_hostname(&conn->host); free_fixed_hostname(&conn->conn_to_host); @@ -943,56 +938,17 @@ proxy_info_matches(const struct proxy_info* data, return FALSE; } - /* - * This function finds the connection in the connection - * bundle that has been unused for the longest time. + * This function checks if the given connection is dead and extracts it from + * the connection cache if so. * - * Returns the pointer to the oldest idle connection, or NULL if none was - * found. - */ -static struct connectdata * -find_oldest_idle_connection_in_bundle(struct Curl_easy *data, - struct connectbundle *bundle) -{ - struct curl_llist_element *curr; - timediff_t highscore = -1; - timediff_t score; - struct curltime now; - struct connectdata *conn_candidate = NULL; - struct connectdata *conn; - - (void)data; - - now = Curl_now(); - - curr = bundle->conn_list.head; - while(curr) { - conn = curr->ptr; - - if(!conn->inuse) { - /* Set higher score for the age passed since the connection was used */ - score = Curl_timediff(now, conn->now); - - if(score > highscore) { - highscore = score; - conn_candidate = conn; - } - } - curr = curr->next; - } - - return conn_candidate; -} - -/* - * This function checks if given connection is dead and disconnects if so. - * (That also removes it from the connection cache.) + * When this is called as a Curl_conncache_foreach() callback, the connection + * cache lock is held! * - * Returns TRUE if the connection actually was dead and disconnected. + * Returns TRUE if the connection was dead and extracted. */ -static bool disconnect_if_dead(struct connectdata *conn, - struct Curl_easy *data) +static bool extract_if_dead(struct connectdata *conn, + struct Curl_easy *data) { size_t pipeLen = conn->send_pipe.size + conn->recv_pipe.size; if(!pipeLen && !conn->inuse) { @@ -1017,25 +973,30 @@ static bool disconnect_if_dead(struct connectdata *conn, if(dead) { conn->data = data; infof(data, "Connection %ld seems to be dead!\n", conn->connection_id); - - /* disconnect resources */ - Curl_disconnect(conn, /* dead_connection */TRUE); + Curl_conncache_remove_conn(conn, FALSE); return TRUE; } } return FALSE; } +struct prunedead { + struct Curl_easy *data; + struct connectdata *extracted; +}; + /* - * Wrapper to use disconnect_if_dead() function in Curl_conncache_foreach() + * Wrapper to use extract_if_dead() function in Curl_conncache_foreach() * - * Returns always 0. */ -static int call_disconnect_if_dead(struct connectdata *conn, - void *param) +static int call_extract_if_dead(struct connectdata *conn, void *param) { - struct Curl_easy* data = (struct Curl_easy*)param; - disconnect_if_dead(conn, data); + struct prunedead *p = (struct prunedead *)param; + if(extract_if_dead(conn, p->data)) { + /* stop the iteration here, pass back the connection that was extracted */ + p->extracted = conn; + return 1; + } return 0; /* continue iteration */ } @@ -1050,8 +1011,14 @@ static void prune_dead_connections(struct Curl_easy *data) time_t elapsed = Curl_timediff(now, data->state.conn_cache->last_cleanup); if(elapsed >= 1000L) { - Curl_conncache_foreach(data, data->state.conn_cache, data, - call_disconnect_if_dead); + struct prunedead prune; + prune.data = data; + prune.extracted = NULL; + while(Curl_conncache_foreach(data, data->state.conn_cache, &prune, + call_extract_if_dead)) { + /* disconnect it */ + (void)Curl_disconnect(prune.extracted, /* dead_connection */TRUE); + } data->state.conn_cache->last_cleanup = now; } } @@ -1106,8 +1073,8 @@ ConnectionExists(struct Curl_easy *data, Curl_pipeline_site_blacklisted(data, needle)) canpipe &= ~ CURLPIPE_HTTP1; - /* Look up the bundle with all the connections to this - particular host */ + /* Look up the bundle with all the connections to this particular host. + Locks the connection cache, beware of early returns! */ bundle = Curl_conncache_find_bundle(needle, data->state.conn_cache); if(bundle) { /* Max pipe length is zero (unlimited) for multiplexed connections */ @@ -1130,6 +1097,7 @@ ConnectionExists(struct Curl_easy *data, if((bundle->multiuse == BUNDLE_UNKNOWN) && data->set.pipewait) { infof(data, "Server doesn't support multi-use yet, wait\n"); *waitpipe = TRUE; + Curl_conncache_unlock(needle); return FALSE; /* no re-use */ } @@ -1161,8 +1129,11 @@ ConnectionExists(struct Curl_easy *data, check = curr->ptr; curr = curr->next; - if(disconnect_if_dead(check, data)) + if(extract_if_dead(check, data)) { + /* disconnect it */ + (void)Curl_disconnect(check, /* dead_connection */TRUE); continue; + } pipeLen = check->send_pipe.size + check->recv_pipe.size; @@ -1479,9 +1450,13 @@ ConnectionExists(struct Curl_easy *data, } if(chosen) { + /* mark it as used before releasing the lock */ + chosen->inuse = TRUE; + Curl_conncache_unlock(needle); *usethis = chosen; return TRUE; /* yes, we found one to use! */ } + Curl_conncache_unlock(needle); if(foundPendingCandidate && data->set.pipewait) { infof(data, @@ -4409,20 +4384,21 @@ static CURLcode create_conn(struct Curl_easy *data, else reuse = ConnectionExists(data, conn, &conn_temp, &force_reuse, &waitpipe); - /* If we found a reusable connection, we may still want to - open a new connection if we are pipelining. */ + /* If we found a reusable connection that is now marked as in use, we may + still want to open a new connection if we are pipelining. */ if(reuse && !force_reuse && IsPipeliningPossible(data, conn_temp)) { size_t pipelen = conn_temp->send_pipe.size + conn_temp->recv_pipe.size; if(pipelen > 0) { infof(data, "Found connection %ld, with requests in the pipe (%zu)\n", conn_temp->connection_id, pipelen); - if(conn_temp->bundle->num_connections < max_host_connections && - data->state.conn_cache->num_connections < max_total_connections) { + if(Curl_conncache_bundle_size(conn_temp) < max_host_connections && + Curl_conncache_size(data) < max_total_connections) { /* We want a new connection anyway */ reuse = FALSE; infof(data, "We can reuse, but we want a new connection anyway\n"); + Curl_conncache_return_conn(conn_temp); } } } @@ -4434,8 +4410,6 @@ static CURLcode create_conn(struct Curl_easy *data, * just allocated before we can move along and use the previously * existing one. */ - conn_temp->inuse = TRUE; /* mark this as being in use so that no other - handle in a multi stack may nick it */ reuse_conn(conn, conn_temp); #ifdef USE_SSL free(conn->ssl_extra); @@ -4455,7 +4429,6 @@ static CURLcode create_conn(struct Curl_easy *data, /* We have decided that we want a new connection. However, we may not be able to do that if we have reached the limit of how many connections we are allowed to open. */ - struct connectbundle *bundle = NULL; if(conn->handler->flags & PROTOPT_ALPN_NPN) { /* The protocol wants it, so set the bits if enabled in the easy handle @@ -4470,35 +4443,42 @@ static CURLcode create_conn(struct Curl_easy *data, /* There is a connection that *might* become usable for pipelining "soon", and we wait for that */ connections_available = FALSE; - else - bundle = Curl_conncache_find_bundle(conn, data->state.conn_cache); - - if(max_host_connections > 0 && bundle && - (bundle->num_connections >= max_host_connections)) { - struct connectdata *conn_candidate; - - /* The bundle is full. Let's see if we can kill a connection. */ - conn_candidate = find_oldest_idle_connection_in_bundle(data, bundle); - - if(conn_candidate) { - /* Set the connection's owner correctly, then kill it */ - conn_candidate->data = data; - (void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE); - } - else { - infof(data, "No more connections allowed to host: %d\n", - max_host_connections); - connections_available = FALSE; + else { + /* this gets a lock on the conncache */ + struct connectbundle *bundle = + Curl_conncache_find_bundle(conn, data->state.conn_cache); + + if(max_host_connections > 0 && bundle && + (bundle->num_connections >= max_host_connections)) { + struct connectdata *conn_candidate; + + /* The bundle is full. Extract the oldest connection. */ + conn_candidate = Curl_conncache_extract_bundle(data, bundle); + Curl_conncache_unlock(conn); + + if(conn_candidate) { + /* Set the connection's owner correctly, then kill it */ + conn_candidate->data = data; + (void)Curl_disconnect(conn_candidate, /* dead_connection */ FALSE); + } + else { + infof(data, "No more connections allowed to host: %d\n", + max_host_connections); + connections_available = FALSE; + } } + else + Curl_conncache_unlock(conn); + } if(connections_available && (max_total_connections > 0) && - (data->state.conn_cache->num_connections >= max_total_connections)) { + (Curl_conncache_size(data) >= max_total_connections)) { struct connectdata *conn_candidate; /* The cache is full. Let's see if we can kill a connection. */ - conn_candidate = Curl_conncache_oldest_idle(data); + conn_candidate = Curl_conncache_extract_oldest(data); if(conn_candidate) { /* Set the connection's owner correctly, then kill it */ @@ -4521,6 +4501,9 @@ static CURLcode create_conn(struct Curl_easy *data, goto out; } else { + /* Mark the connection as used, before we add it */ + conn->inuse = TRUE; + /* * This is a brand new connection, so let's store it in the connection * cache of ours! @@ -4548,9 +4531,6 @@ static CURLcode create_conn(struct Curl_easy *data, #endif } - /* Mark the connection as used */ - conn->inuse = TRUE; - /* Setup and init stuff before DO starts, in preparing for the transfer. */ Curl_init_do(data, conn); -- cgit v1.2.1