Skip to content
Permalink
Browse files

conncache: hold the lock longer when fiddling with the bundles

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
  • Loading branch information...
bagder committed Dec 2, 2017
1 parent 10bb0b4 commit d09f14772205b629a5fe1a030bae796f63e94625
Showing with 64 additions and 32 deletions.
  1. +36 −13 lib/conncache.c
  2. +1 −0 lib/conncache.h
  3. +24 −19 lib/url.c
  4. +3 −0 lib/urldata.h
@@ -40,11 +40,27 @@
#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

static void conn_llist_dtor(void *user, void *element)
{
@@ -165,18 +181,24 @@ static void hashkey(struct connectdata *conn, char *buf,
snprintf(buf, len, "%ld%s", conn->port, hostname);
}

void Curl_conncache_unlock(struct connectdata *conn)
{
CONN_UNLOCK(conn->data);
}

/* Look up the bundle with all the connections to the same host this
connectdata struct is setup to use. */
connectdata struct is setup to use.
**NOTE**: When it returns, it holds the connection cache lock! */
struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
struct conncache *connc)
{
struct connectbundle *bundle = NULL;
CONN_LOCK(conn->data);
if(connc) {
char key[128];
hashkey(conn, key, sizeof(key));
CONN_LOCK(conn->data);
bundle = Curl_hash_pick(&connc->hash, key, strlen(key));
CONN_UNLOCK(conn->data);
}

return bundle;
@@ -223,36 +245,34 @@ CURLcode Curl_conncache_add_conn(struct conncache *connc,
struct connectbundle *new_bundle = NULL;
struct Curl_easy *data = conn->data;

/* *find_bundle() locks the connection cache */
bundle = Curl_conncache_find_bundle(conn, data->state.conn_cache);
if(!bundle) {
int rc;
char key[128];

result = bundle_create(data, &new_bundle);
if(result)
return result;
if(result) {
goto unlock;
}

hashkey(conn, key, sizeof(key));
CONN_LOCK(data);
rc = conncache_add_bundle(data->state.conn_cache, key, new_bundle);
CONN_UNLOCK(data);

if(!rc) {
bundle_destroy(new_bundle);
return CURLE_OUT_OF_MEMORY;
result = CURLE_OUT_OF_MEMORY;
goto unlock;
}
bundle = new_bundle;
}

CONN_LOCK(data);
result = bundle_add_conn(bundle, conn);
if(result) {
if(new_bundle)
conncache_remove_bundle(data->state.conn_cache, new_bundle);
CONN_UNLOCK(data);
return result;
goto unlock;
}
CONN_UNLOCK(data);

conn->connection_id = connc->next_connection_id++;
connc->num_connections++;
@@ -261,6 +281,9 @@ CURLcode Curl_conncache_add_conn(struct conncache *connc,
"The cache now contains %" CURL_FORMAT_CURL_OFF_TU " members\n",
conn->connection_id, (curl_off_t) connc->num_connections));

unlock:
CONN_UNLOCK(data);

return CURLE_OK;
}

@@ -50,6 +50,7 @@ void Curl_conncache_destroy(struct conncache *connc);
/* return the correct bundle, to a host or a proxy */
struct connectbundle *Curl_conncache_find_bundle(struct connectdata *conn,
struct conncache *connc);
void Curl_conncache_unlock(struct connectdata *conn);

CURLcode Curl_conncache_add_conn(struct conncache *connc,
struct connectdata *conn);
@@ -1106,8 +1106,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 +1130,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 */
}

@@ -1477,6 +1478,7 @@ ConnectionExists(struct Curl_easy *data,
}
}
}
Curl_conncache_unlock(needle);

if(chosen) {
*usethis = chosen;
@@ -4455,7 +4457,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,26 +4471,30 @@ 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);
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;
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);
/* 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;
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;
}
}
Curl_conncache_unlock(conn);
}

if(connections_available &&
@@ -1325,6 +1325,9 @@ struct UrlState {
struct Curl_easy *stream_depends_on;
bool stream_depends_e; /* set or don't set the Exclusive bit */
int stream_weight;
#ifdef CURLDEBUG
bool conncache_lock;
#endif
};


0 comments on commit d09f147

Please sign in to comment.
You can’t perform that action at this time.