From 094b5f3540fec1401f514bc470f11f441527d30a Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Wed, 5 Jun 2019 15:50:49 +0200 Subject: [PATCH] multi: make sure 'data' can present in several sockhash entries Since more than one socket can be used by each transfer at a given time, each sockhash entry how has its own hash table with transfers using that socket. In addition, the sockhash entry can now be marked 'blocked = TRUE'" which then makes the delete function just set 'removed = TRUE' instead of removing it "for real", as a way to not rip out the carpet under the feet of a parent function that iterates over the transfers of that same sockhash entry. Reported-by: Tom van der Woerdt Fixes #3961 Fixes #3986 Fixes #3995 Fixes #4004 Closes #3997 --- lib/multi.c | 159 +++++++++++++++++++++++++++++--------------------- lib/url.c | 9 +-- lib/urldata.h | 2 - 3 files changed, 95 insertions(+), 75 deletions(-) diff --git a/lib/multi.c b/lib/multi.c index dc780630b..33f0d9fd1 100644 --- a/lib/multi.c +++ b/lib/multi.c @@ -187,13 +187,16 @@ static void mstate(struct Curl_easy *data, CURLMstate state */ struct Curl_sh_entry { - struct curl_llist list; /* list of easy handles using this socket */ + struct curl_hash transfers; /* hash of transfers using this socket */ unsigned int action; /* what combined action READ/WRITE this socket waits for */ void *socketp; /* settable by users with curl_multi_assign() */ unsigned int users; /* number of transfers using this */ unsigned int readers; /* this many transfers want to read */ unsigned int writers; /* this many transfers want to write */ + unsigned int blocked:1; /* if TRUE, blocked from being removed */ + unsigned int removed:1; /* if TRUE, this entry is "removed" but prevented + from it by "blocked" being set! */ }; /* bits for 'action' having no bits means this socket is not expecting any action */ @@ -202,31 +205,67 @@ struct Curl_sh_entry { /* look up a given socket in the socket hash, skip invalid sockets */ static struct Curl_sh_entry *sh_getentry(struct curl_hash *sh, - curl_socket_t s) + curl_socket_t s, + bool also_hidden) { - if(s != CURL_SOCKET_BAD) + if(s != CURL_SOCKET_BAD) { /* only look for proper sockets */ - return Curl_hash_pick(sh, (char *)&s, sizeof(curl_socket_t)); + struct Curl_sh_entry *entry = + Curl_hash_pick(sh, (char *)&s, sizeof(curl_socket_t)); + if(entry && entry->removed && !also_hidden) + return NULL; + return entry; + } return NULL; } +#define TRHASH_SIZE 13 +static size_t trhash(void *key, size_t key_length, size_t slots_num) +{ + size_t keyval = (size_t)key; /* this is a data pointer */ + (void) key_length; + + return (keyval % slots_num); +} + +static size_t trhash_compare(void *k1, size_t k1_len, void *k2, size_t k2_len) +{ + (void)k1_len; + (void)k2_len; + + return k1 == k2; +} + +static void trhash_dtor(void *nada) +{ + (void)nada; +} + + /* make sure this socket is present in the hash for this handle */ static struct Curl_sh_entry *sh_addentry(struct curl_hash *sh, curl_socket_t s) { - struct Curl_sh_entry *there = sh_getentry(sh, s); + struct Curl_sh_entry *there = sh_getentry(sh, s, TRUE); struct Curl_sh_entry *check; - if(there) + if(there) { /* it is present, return fine */ + if(there->removed) + there->removed = FALSE; /* clear the removed bit */ return there; + } /* not present, add it */ check = calloc(1, sizeof(struct Curl_sh_entry)); if(!check) return NULL; /* major failure */ - Curl_llist_init(&check->list, NULL); + if(Curl_hash_init(&check->transfers, TRHASH_SIZE, trhash, + trhash_compare, trhash_dtor)) { + free(check); + return NULL; + } /* make/add new hash entry */ if(!Curl_hash_add(sh, (char *)&s, sizeof(curl_socket_t), check)) { @@ -242,17 +281,17 @@ static struct Curl_sh_entry *sh_addentry(struct curl_hash *sh, static void sh_delentry(struct Curl_sh_entry *entry, struct curl_hash *sh, curl_socket_t s) { - struct curl_llist *list = &entry->list; - struct curl_llist_element *e; - /* clear the list of transfers first */ - for(e = list->head; e; e = list->head) { - struct Curl_easy *dta = e->ptr; - Curl_llist_remove(&entry->list, e, NULL); - dta->sh_entry = NULL; + if(entry->blocked) { + entry->removed = TRUE; /* pretend */ + return; + } + else { + Curl_hash_destroy(&entry->transfers); + + /* We remove the hash entry. This will end up in a call to + sh_freeentry(). */ + Curl_hash_delete(sh, (char *)&s, sizeof(curl_socket_t)); } - /* We remove the hash entry. This will end up in a call to - sh_freeentry(). */ - Curl_hash_delete(sh, (char *)&s, sizeof(curl_socket_t)); } /* @@ -318,17 +357,6 @@ static CURLMcode multi_addmsg(struct Curl_multi *multi, return CURLM_OK; } -/* - * multi_freeamsg() - * - * Callback used by the llist system when a single list entry is destroyed. - */ -static void multi_freeamsg(void *a, void *b) -{ - (void)a; - (void)b; -} - struct Curl_multi *Curl_multi_handle(int hashsize, /* socket hash */ int chashsize) /* connection hash */ { @@ -348,8 +376,8 @@ struct Curl_multi *Curl_multi_handle(int hashsize, /* socket hash */ if(Curl_conncache_init(&multi->conn_cache, chashsize)) goto error; - Curl_llist_init(&multi->msglist, multi_freeamsg); - Curl_llist_init(&multi->pending, multi_freeamsg); + Curl_llist_init(&multi->msglist, NULL); + Curl_llist_init(&multi->pending, NULL); /* -1 means it not set by user, use the default value */ multi->maxconnects = -1; @@ -787,11 +815,6 @@ bool Curl_multiplex_wanted(const struct Curl_multi *multi) static void detach_connnection(struct Curl_easy *data) { struct connectdata *conn = data->conn; - if(data->sh_entry) { - /* still listed as a user of a socket hash entry, remove it */ - Curl_llist_remove(&data->sh_entry->list, &data->sh_queue, NULL); - data->sh_entry = NULL; - } if(conn) Curl_llist_remove(&conn->easyq, &data->conn_queue, NULL); data->conn = NULL; @@ -1264,6 +1287,9 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi, bool stream_error = FALSE; rc = CURLM_OK; + DEBUGASSERT((data->mstate <= CURLM_STATE_CONNECT) || + (data->mstate >= CURLM_STATE_DONE) || + data->conn); if(!data->conn && data->mstate > CURLM_STATE_CONNECT && data->mstate < CURLM_STATE_DONE) { @@ -2240,7 +2266,7 @@ static CURLMcode singlesocket(struct Curl_multi *multi, s = socks[i]; /* get it from the hash */ - entry = sh_getentry(&multi->sockhash, s); + entry = sh_getentry(&multi->sockhash, s, FALSE); if(curraction & GETSOCK_READSOCK(i)) action |= CURL_POLL_IN; @@ -2285,30 +2311,22 @@ static CURLMcode singlesocket(struct Curl_multi *multi, if(action & CURL_POLL_OUT) entry->writers++; - /* add 'data' to the list of handles using this socket! */ - Curl_llist_insert_next(&entry->list, entry->list.tail, - data, &data->sh_queue); - data->sh_entry = entry; + /* add 'data' to the transfer hash on this socket! */ + if(!Curl_hash_add(&entry->transfers, (char *)data, /* hash key */ + sizeof(struct Curl_easy *), data)) + return CURLM_OUT_OF_MEMORY; } comboaction = (entry->writers? CURL_POLL_OUT : 0) | (entry->readers ? CURL_POLL_IN : 0); -#if 0 - infof(data, "--- Comboaction: %u readers %u writers\n", - entry->readers, entry->writers); -#endif - /* check if it has the same action set */ - if(entry->action == comboaction) + /* socket existed before and has the same action set as before */ + if(sincebefore && (entry->action == comboaction)) /* same, continue */ continue; - /* we know (entry != NULL) at this point, see the logic above */ if(multi->socket_cb) - multi->socket_cb(data, - s, - comboaction, - multi->socket_userp, + multi->socket_cb(data, s, comboaction, multi->socket_userp, entry->socketp); entry->action = comboaction; /* store the current action state */ @@ -2332,7 +2350,7 @@ static CURLMcode singlesocket(struct Curl_multi *multi, if(stillused) continue; - entry = sh_getentry(&multi->sockhash, s); + entry = sh_getentry(&multi->sockhash, s, FALSE); /* if this is NULL here, the socket has been closed and notified so already by Curl_multi_closed() */ if(entry) { @@ -2350,6 +2368,11 @@ static CURLMcode singlesocket(struct Curl_multi *multi, entry->socketp); sh_delentry(entry, &multi->sockhash, s); } + else { + /* still users, but remove this handle as a user of this socket */ + Curl_hash_delete(&entry->transfers, (char *)data, + sizeof(struct Curl_easy *)); + } } } /* for loop over numsocks */ @@ -2383,7 +2406,7 @@ void Curl_multi_closed(struct Curl_easy *data, curl_socket_t s) if(multi) { /* this is set if this connection is part of a handle that is added to a multi handle, and only then this is necessary */ - struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s); + struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s, FALSE); if(entry) { if(multi->socket_cb) @@ -2483,7 +2506,7 @@ static CURLMcode multi_socket(struct Curl_multi *multi, return result; } if(s != CURL_SOCKET_TIMEOUT) { - struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s); + struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s, FALSE); if(!entry) /* Unmatched socket, we can't act on it but we ignore this fact. In @@ -2493,18 +2516,20 @@ static CURLMcode multi_socket(struct Curl_multi *multi, and just move on. */ ; else { - struct curl_llist *list = &entry->list; - struct curl_llist_element *e; - struct curl_llist_element *enext; + struct curl_hash_iterator iter; + struct curl_hash_element *he; SIGPIPE_VARIABLE(pipe_st); - /* the socket can be shared by many transfers, iterate */ - for(e = list->head; e; e = enext) { - data = (struct Curl_easy *)e->ptr; + /* block this sockhash entry from being removed in a sub function called + from here */ + entry->blocked = TRUE; + DEBUGASSERT(!entry->removed); - /* assign 'enext' here since the 'e' struct might be cleared - further down in the singlesocket() call */ - enext = e->next; + /* the socket can be shared by many transfers, iterate */ + Curl_hash_start_iterate(&entry->transfers, &iter); + for(he = Curl_hash_next_element(&iter); he; + he = Curl_hash_next_element(&iter)) { + data = (struct Curl_easy *)he->ptr; DEBUGASSERT(data); DEBUGASSERT(data->magic == CURLEASY_MAGIC_NUMBER); @@ -2529,6 +2554,10 @@ static CURLMcode multi_socket(struct Curl_multi *multi, return result; } } + if(entry->removed) { + entry->blocked = FALSE; /* unblock */ + sh_delentry(entry, &multi->sockhash, s); /* delete for real */ + } /* Now we fall-through and do the timer-based stuff, since we don't want to force the user to have to deal with timeouts as long as at least @@ -2976,7 +3005,7 @@ CURLMcode curl_multi_assign(struct Curl_multi *multi, curl_socket_t s, if(multi->in_callback) return CURLM_RECURSIVE_API_CALL; - there = sh_getentry(&multi->sockhash, s); + there = sh_getentry(&multi->sockhash, s, FALSE); if(!there) return CURLM_BAD_SOCKET; @@ -3065,7 +3094,7 @@ void Curl_multi_dump(struct Curl_multi *multi) statename[data->mstate], data->numsocks); for(i = 0; i < data->numsocks; i++) { curl_socket_t s = data->sockets[i]; - struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s); + struct Curl_sh_entry *entry = sh_getentry(&multi->sockhash, s, FALSE); fprintf(stderr, "%d ", (int)s); if(!entry) { diff --git a/lib/url.c b/lib/url.c index eb22dcc37..c37ce0494 100644 --- a/lib/url.c +++ b/lib/url.c @@ -1673,13 +1673,6 @@ static void free_idnconverted_hostname(struct hostname *host) #endif } -static void llist_dtor(void *user, void *element) -{ - (void)user; - (void)element; - /* Do nothing */ -} - /* * Allocate and initialize a new connectdata object. */ @@ -1791,7 +1784,7 @@ static struct connectdata *allocate_conn(struct Curl_easy *data) #endif /* Initialize the easy handle list */ - Curl_llist_init(&conn->easyq, (curl_llist_dtor) llist_dtor); + Curl_llist_init(&conn->easyq, NULL); #ifdef HAVE_GSSAPI conn->data_prot = PROT_CLEAR; diff --git a/lib/urldata.h b/lib/urldata.h index f8ba591dd..fdc185b22 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -1778,8 +1778,6 @@ struct Curl_easy { struct connectdata *conn; struct curl_llist_element connect_queue; - struct curl_llist_element sh_queue; /* list per Curl_sh_entry */ - struct Curl_sh_entry *sh_entry; /* the socket hash this was added to */ struct curl_llist_element conn_queue; /* list per connectdata */ CURLMstate mstate; /* the handle's state */ -- 2.40.0