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

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 */