From: Stefan Eissing <stefan@eissing.org>
Date: Mon, 29 Jul 2024 08:23:20 +0000 (+0200)
Subject: connect: fix connection shutdown for event based processing
X-Git-Tag: curl-8_9_1~25
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=17e6f06ea37136c36d27;p=thirdparty%2Fcurl.git

connect: fix connection shutdown for event based processing

connections being shutdown would register sockets for events, but then
never remove these sockets again. Nor would the shutdown effectively
been performed.

- If a socket event involves a transfer, check if that is the
  connection cache internal handle and run its multi_perform()
  instead (the internal handle is used for all shutdowns).
- When a timer triggers for a transfer, check also if it is
  about the connection cache internal handle.
- During processing shutdowns in the connection cache, assess
  the shutdown timeouts. Register a Curl_expire() of the lowest
  value for the cache's internal handle.

Reported-by: Gordon Parke
Fixes #14280
Closes #14296
---

diff --git a/lib/conncache.c b/lib/conncache.c
index a1c44cf044..92fd60c4df 100644
--- a/lib/conncache.c
+++ b/lib/conncache.c
@@ -584,15 +584,15 @@ static void connc_close_all(struct conncache *connc)
     return;
 
   /* Move all connections to the shutdown list */
+  sigpipe_init(&pipe_st);
   conn = connc_find_first_connection(connc);
   while(conn) {
     connc_remove_conn(connc, conn);
-    sigpipe_ignore(data, &pipe_st);
+    sigpipe_apply(data, &pipe_st);
     /* This will remove the connection from the cache */
     connclose(conn, "kill all");
     Curl_conncache_remove_conn(connc->closure_handle, conn, TRUE);
     connc_discard_conn(connc, connc->closure_handle, conn, FALSE);
-    sigpipe_restore(&pipe_st);
 
     conn = connc_find_first_connection(connc);
   }
@@ -613,7 +613,7 @@ static void connc_close_all(struct conncache *connc)
   /* discard all connections in the shutdown list */
   connc_shutdown_discard_all(connc);
 
-  sigpipe_ignore(data, &pipe_st);
+  sigpipe_apply(data, &pipe_st);
   Curl_hostcache_clean(data, data->dns.hostcache);
   Curl_close(&data);
   sigpipe_restore(&pipe_st);
@@ -628,7 +628,6 @@ static void connc_shutdown_discard_oldest(struct conncache *connc)
 {
   struct Curl_llist_element *e;
   struct connectdata *conn;
-  SIGPIPE_VARIABLE(pipe_st);
 
   DEBUGASSERT(!connc->shutdowns.iter_locked);
   if(connc->shutdowns.iter_locked)
@@ -636,9 +635,11 @@ static void connc_shutdown_discard_oldest(struct conncache *connc)
 
   e = connc->shutdowns.conn_list.head;
   if(e) {
+    SIGPIPE_VARIABLE(pipe_st);
     conn = e->ptr;
     Curl_llist_remove(&connc->shutdowns.conn_list, e, NULL);
-    sigpipe_ignore(connc->closure_handle, &pipe_st);
+    sigpipe_init(&pipe_st);
+    sigpipe_apply(connc->closure_handle, &pipe_st);
     connc_disconnect(NULL, conn, connc, FALSE);
     sigpipe_restore(&pipe_st);
   }
@@ -900,6 +901,9 @@ static void connc_perform(struct conncache *connc)
   struct Curl_llist_element *e = connc->shutdowns.conn_list.head;
   struct Curl_llist_element *enext;
   struct connectdata *conn;
+  struct curltime *nowp = NULL;
+  struct curltime now;
+  timediff_t next_from_now_ms = 0, ms;
   bool done;
 
   if(!e)
@@ -922,9 +926,22 @@ static void connc_perform(struct conncache *connc)
       Curl_llist_remove(&connc->shutdowns.conn_list, e, NULL);
       connc_disconnect(NULL, conn, connc, FALSE);
     }
+    else {
+      /* Not done, when does this connection time out? */
+      if(!nowp) {
+        now = Curl_now();
+        nowp = &now;
+      }
+      ms = Curl_conn_shutdown_timeleft(conn, nowp);
+      if(ms && ms < next_from_now_ms)
+        next_from_now_ms = ms;
+    }
     e = enext;
   }
   connc->shutdowns.iter_locked = FALSE;
+
+  if(next_from_now_ms)
+    Curl_expire(data, next_from_now_ms, EXPIRE_RUN_NOW);
 }
 
 void Curl_conncache_multi_perform(struct Curl_multi *multi)
diff --git a/lib/connect.c b/lib/connect.c
index 6041629268..9b68f15da5 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -161,6 +161,7 @@ timediff_t Curl_shutdown_timeleft(struct connectdata *conn, int sockindex,
                                   struct curltime *nowp)
 {
   struct curltime now;
+  timediff_t left_ms;
 
   if(!conn->shutdown.start[sockindex].tv_sec || !conn->shutdown.timeout_ms)
     return 0; /* not started or no limits */
@@ -169,8 +170,30 @@ timediff_t Curl_shutdown_timeleft(struct connectdata *conn, int sockindex,
     now = Curl_now();
     nowp = &now;
   }
-  return conn->shutdown.timeout_ms -
-         Curl_timediff(*nowp, conn->shutdown.start[sockindex]);
+  left_ms = conn->shutdown.timeout_ms -
+            Curl_timediff(*nowp, conn->shutdown.start[sockindex]);
+  return left_ms? left_ms : -1;
+}
+
+timediff_t Curl_conn_shutdown_timeleft(struct connectdata *conn,
+                                       struct curltime *nowp)
+{
+  timediff_t left_ms = 0, ms;
+  struct curltime now;
+  int i;
+
+  for(i = 0; conn->shutdown.timeout_ms && (i < 2); ++i) {
+    if(!conn->shutdown.start[i].tv_sec)
+      continue;
+    if(!nowp) {
+      now = Curl_now();
+      nowp = &now;
+    }
+    ms = Curl_shutdown_timeleft(conn, i, nowp);
+    if(ms && (!left_ms || ms < left_ms))
+      left_ms = ms;
+  }
+  return left_ms;
 }
 
 void Curl_shutdown_clear(struct Curl_easy *data, int sockindex)
diff --git a/lib/connect.h b/lib/connect.h
index ffceeb87f6..e9a4f9c598 100644
--- a/lib/connect.h
+++ b/lib/connect.h
@@ -46,10 +46,15 @@ void Curl_shutdown_start(struct Curl_easy *data, int sockindex,
                          struct curltime *nowp);
 
 /* return how much time there is left to shutdown the connection at
- * sockindex. */
+ * sockindex. Returns 0 if there is no limit or shutdown has not started. */
 timediff_t Curl_shutdown_timeleft(struct connectdata *conn, int sockindex,
                                   struct curltime *nowp);
 
+/* return how much time there is left to shutdown the connection.
+ * Returns 0 if there is no limit or shutdown has not started. */
+timediff_t Curl_conn_shutdown_timeleft(struct connectdata *conn,
+                                       struct curltime *nowp);
+
 void Curl_shutdown_clear(struct Curl_easy *data, int sockindex);
 
 /* TRUE iff shutdown has been started */
diff --git a/lib/easy.c b/lib/easy.c
index 75bf091bcb..2ba9af0b3a 100644
--- a/lib/easy.c
+++ b/lib/easy.c
@@ -764,7 +764,8 @@ static CURLcode easy_perform(struct Curl_easy *data, bool events)
   /* assign this after curl_multi_add_handle() */
   data->multi_easy = multi;
 
-  sigpipe_ignore(data, &pipe_st);
+  sigpipe_init(&pipe_st);
+  sigpipe_apply(data, &pipe_st);
 
   /* run the transfer */
   result = events ? easy_events(multi) : easy_transfer(multi);
diff --git a/lib/multi.c b/lib/multi.c
index 81bd942ff9..896f730490 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -2714,6 +2714,7 @@ CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles)
   CURLMcode returncode = CURLM_OK;
   struct Curl_tree *t;
   struct curltime now = Curl_now();
+  SIGPIPE_VARIABLE(pipe_st);
 
   if(!GOOD_MULTI_HANDLE(multi))
     return CURLM_BAD_HANDLE;
@@ -2721,12 +2722,10 @@ CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles)
   if(multi->in_callback)
     return CURLM_RECURSIVE_API_CALL;
 
+  sigpipe_init(&pipe_st);
   data = multi->easyp;
   if(data) {
     CURLMcode result;
-    bool nosig = data->set.no_signal;
-    SIGPIPE_VARIABLE(pipe_st);
-    sigpipe_ignore(data, &pipe_st);
     /* Do the loop and only alter the signal ignore state if the next handle
        has a different NO_SIGNAL state than the previous */
     do {
@@ -2734,22 +2733,23 @@ CURLMcode curl_multi_perform(struct Curl_multi *multi, int *running_handles)
          pointer now */
       struct Curl_easy *datanext = data->next;
 
-      if(data->set.no_signal != nosig) {
-        sigpipe_restore(&pipe_st);
-        sigpipe_ignore(data, &pipe_st);
-        nosig = data->set.no_signal;
+    if(data != multi->conn_cache.closure_handle) {
+        /* connection cache handle is processed below */
+        sigpipe_apply(data, &pipe_st);
+        result = multi_runsingle(multi, &now, data);
+        if(result)
+          returncode = result;
       }
-      result = multi_runsingle(multi, &now, data);
-      if(result)
-        returncode = result;
 
       data = datanext; /* operate on next handle */
     } while(data);
-    sigpipe_restore(&pipe_st);
   }
 
+  sigpipe_apply(multi->conn_cache.closure_handle, &pipe_st);
   Curl_conncache_multi_perform(multi);
 
+  sigpipe_restore(&pipe_st);
+
   /*
    * Simply remove all expired timers from the splay since handles are dealt
    * with unconditionally by this function and curl_multi_timeout() requires
@@ -3189,8 +3189,7 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
   struct Curl_easy *data = NULL;
   struct Curl_tree *t;
   struct curltime now = Curl_now();
-  bool first = FALSE;
-  bool nosig = FALSE;
+  bool run_conn_cache = FALSE;
   SIGPIPE_VARIABLE(pipe_st);
 
   if(checkall) {
@@ -3235,11 +3234,15 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
         DEBUGASSERT(data);
         DEBUGASSERT(data->magic == CURLEASY_MAGIC_NUMBER);
 
-        if(data->conn && !(data->conn->handler->flags & PROTOPT_DIRLOCK))
-          /* set socket event bitmask if they are not locked */
-          data->state.select_bits |= (unsigned char)ev_bitmask;
+        if(data == multi->conn_cache.closure_handle)
+          run_conn_cache = TRUE;
+        else {
+          if(data->conn && !(data->conn->handler->flags & PROTOPT_DIRLOCK))
+            /* set socket event bitmask if they are not locked */
+            data->state.select_bits |= (unsigned char)ev_bitmask;
 
-        Curl_expire(data, 0, EXPIRE_RUN_NOW);
+          Curl_expire(data, 0, EXPIRE_RUN_NOW);
+        }
       }
 
       /* Now we fall-through and do the timer-based stuff, since we do not want
@@ -3266,19 +3269,13 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
    * to process in the splay and 'data' will be re-assigned for every expired
    * handle we deal with.
    */
+  sigpipe_init(&pipe_st);
   do {
+    if(data == multi->conn_cache.closure_handle)
+      run_conn_cache = TRUE;
     /* the first loop lap 'data' can be NULL */
-    if(data) {
-      if(!first) {
-        first = TRUE;
-        nosig = data->set.no_signal; /* initial state */
-        sigpipe_ignore(data, &pipe_st);
-      }
-      else if(data->set.no_signal != nosig) {
-        sigpipe_restore(&pipe_st);
-        sigpipe_ignore(data, &pipe_st);
-        nosig = data->set.no_signal; /* remember new state */
-      }
+    else if(data) {
+      sigpipe_apply(data, &pipe_st);
       result = multi_runsingle(multi, &now, data);
 
       if(CURLM_OK >= result) {
@@ -3300,8 +3297,13 @@ static CURLMcode multi_socket(struct Curl_multi *multi,
     }
 
   } while(t);
-  if(first)
-    sigpipe_restore(&pipe_st);
+
+  if(run_conn_cache) {
+    sigpipe_apply(multi->conn_cache.closure_handle, &pipe_st);
+    Curl_conncache_multi_perform(multi);
+  }
+
+  sigpipe_restore(&pipe_st);
 
   if(running_handles)
     *running_handles = (int)multi->num_alive;
diff --git a/lib/sigpipe.h b/lib/sigpipe.h
index 9b29403c28..b91a2f5133 100644
--- a/lib/sigpipe.h
+++ b/lib/sigpipe.h
@@ -36,6 +36,11 @@ struct sigpipe_ignore {
 
 #define SIGPIPE_VARIABLE(x) struct sigpipe_ignore x
 
+static void sigpipe_init(struct sigpipe_ignore *ig)
+{
+  memset(ig, 0, sizeof(*ig));
+}
+
 /*
  * sigpipe_ignore() makes sure we ignore SIGPIPE while running libcurl
  * internals, and then sigpipe_restore() will restore the situation when we
@@ -70,9 +75,20 @@ static void sigpipe_restore(struct sigpipe_ignore *ig)
     sigaction(SIGPIPE, &ig->old_pipe_act, NULL);
 }
 
+static void sigpipe_apply(struct Curl_easy *data,
+                          struct sigpipe_ignore *ig)
+{
+  if(data->set.no_signal != ig->no_signal) {
+    sigpipe_restore(ig);
+    sigpipe_ignore(data, ig);
+  }
+}
+
 #else
 /* for systems without sigaction */
 #define sigpipe_ignore(x,y) Curl_nop_stmt
+#define sigpipe_apply(x,y) Curl_nop_stmt
+#define sigpipe_init(x)  Curl_nop_stmt
 #define sigpipe_restore(x)  Curl_nop_stmt
 #define SIGPIPE_VARIABLE(x)
 #endif