tool_doswin: don't use TerminateThread in stdin relay

It is a dangerous function. Remove usage by only starting
the thread after setup completed successfully.

Ref: https://learn.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-terminatethread
Ref: https://github.com/curl/curl/pull/21467#discussion_r3173013432
Ref: https://github.com/curl/curl/pull/18996#issuecomment-3389155817
Ref: https://github.com/curl/curl/pull/18451
Follow-up to 9a26633 #17572

Closes https://github.com/curl/curl/pull/22383

.github/workflows/windows.yml

@@ -535,9 +535,6 @@ jobs:
               unset CURL_TEST_SSH_KEYALGO  # libssh2 built with WinCNG does not support ssh-ed25519 hostkeys
               export CURL_TEST_SSH_ENABLE_KEX=diffie-hellman-group-exchange-sha256
             fi
-            if [[ "${TFLAGS}" = *'-t'* ]]; then
-              TFLAGS+=' !2300'  # Leaks memory and file handle via tool_doswin.c / win32_stdin_read_thread()
-            fi
             if [[ "${MATRIX_INSTALL} " = *'-libssh '* && \
                   "${MATRIX_ENV}" = *'x86_64'* ]]; then
               export CURL_TEST_SSH_DISABLE_KEX=mlkem768x25519-sha256  # broken with libssh 0.12.0 Windows x64

src/tool_doswin.c

@@ -30,6 +30,7 @@
 
 #ifdef _WIN32
 #  include "curlx/winapi.h" /* for curlx_win32_random() */
+#  include "curlx/nonblock.h" /* for curlx_nonblock() */
 #  include <tlhelp32.h>
 #elif !defined(__DJGPP__) || (__DJGPP__ < 2)  /* DJGPP 2.0 has _use_lfn() */
 #  define CURL_USE_LFN(f) 0  /* long filenames never available */
@@ -703,87 +704,123 @@ static void init_terminal(void)
 #ifdef USE_WINSOCK
 /* The following STDIN non - blocking read techniques are heavily inspired
    by nmap and ncat (https://nmap.org/ncat/) */
-struct win_thread_data {
+static struct win_thread_data {
   /* This is a copy of the true stdin file handle before any redirection. It is
      read by the thread. */
   HANDLE stdin_handle;
-  /* This is the listen socket for the thread. It is closed after the first
+  /* This is the socket the thread will forward stdin to. It is connected to
-     connection. */
+     the socket which replaces the stdin handle. */
-  curl_socket_t socket_l;
+  curl_socket_t socket_w;
-  /* This is the random number which the background thread uses to verify
+  /* This is a mutex-like object which we use to synchronize
-     the peer. */
+   * cleanup_tdata_sync() */
-  uint64_t expected_auth_val;
+  CRITICAL_SECTION crit_sect;
-};
+} tdata = { NULL, CURL_SOCKET_BAD, {0}};
+
+static void cleanup_tdata_sync(void)
+{
+  EnterCriticalSection(&tdata.crit_sect);
+  if(tdata.stdin_handle) {
+    CloseHandle(tdata.stdin_handle);
+    tdata.stdin_handle = NULL;
+  }
+
+  if(tdata.socket_w != CURL_SOCKET_BAD) {
+    sclose(tdata.socket_w);
+    tdata.socket_w = CURL_SOCKET_BAD;
+  }
+  LeaveCriticalSection(&tdata.crit_sect);
+}
 
 static DWORD WINAPI win_stdin_thread_func(void *thread_data)
 {
-  struct win_thread_data *tdata = (struct win_thread_data *)thread_data;
+  (void)thread_data;
-  struct sockaddr_in clientAddr;
-  int clientAddrLen = sizeof(clientAddr);
-  size_t nread = 0;
-  uint64_t auth_val = 0;
 
-  curl_socket_t socket_w = CURL_ACCEPT(tdata->socket_l,
-                                       (struct sockaddr *)&clientAddr,
-                                       &clientAddrLen);
-
-  if(socket_w == CURL_SOCKET_BAD) {
-    errorf("accept error: %d", SOCKERRNO);
-    goto ThreadCleanup;
-  }
-
-  sclose(tdata->socket_l);
-  tdata->socket_l = CURL_SOCKET_BAD;
-
-  do {
-    ssize_t ret = sread(socket_w, ((unsigned char *)&auth_val) + nread,
-                        sizeof(auth_val) - nread);
-    if(ret <= 0) {
-      if(!ret) {
-        errorf("relay peer disconnected");
-      }
-      else {
-        errorf("read error: %d", SOCKERRNO);
-      }
-
-      goto ThreadCleanup;
-    }
-    nread += ret;
-  } while(nread < sizeof(auth_val));
-
-  if(auth_val != tdata->expected_auth_val) {
-    errorf("relay peer auth failed");
-    goto ThreadCleanup;
-  }
-
-  if(shutdown(socket_w, SHUT_RD)) {
-    errorf("shutdown error: %d", SOCKERRNO);
-    goto ThreadCleanup;
-  }
   for(;;) {
     DWORD n;
     ssize_t nwritten;
     char buffer[BUFSIZ];
 
-    if(!ReadFile(tdata->stdin_handle, buffer, sizeof(buffer), &n, NULL))
+    if(!ReadFile(tdata.stdin_handle, buffer, sizeof(buffer), &n, NULL))
       break;
     if(n == 0)
       break;
-    nwritten = swrite(socket_w, buffer, n);
+    nwritten = swrite(tdata.socket_w, buffer, n);
     if(nwritten == -1)
       break;
     if((DWORD)nwritten != n)
       break;
   }
-ThreadCleanup:
-  if(tdata->socket_l != CURL_SOCKET_BAD) {
-    sclose(tdata->socket_l);
-    tdata->socket_l = CURL_SOCKET_BAD;
-  }
-  if(socket_w != CURL_SOCKET_BAD)
-    sclose(socket_w);
 
-  curlx_free(tdata);
+  cleanup_tdata_sync();
+
+  return 0;
+}
+
+static int swrite_blocking_on_nonblock(curl_socket_t nonblock_sock,
+                                       const unsigned char *data,
+                                       size_t nbytes)
+{
+  fd_set fdwrite;
+  fd_set fdexcep;
+  size_t nwritten = 0;
+
+  FD_ZERO(&fdwrite);
+  FD_ZERO(&fdexcep);
+
+  FD_SET(nonblock_sock, &fdwrite);
+
+  do {
+    ssize_t ret;
+
+    FD_SET(nonblock_sock, &fdexcep);
+
+    if(select(0, NULL, &fdwrite, &fdexcep, NULL) <= 0) {
+      errorf("select error: %d", SOCKERRNO);
+      return -1;
+    }
+
+    if(FD_ISSET(nonblock_sock, &fdexcep)) {
+      int sock_err = 0;
+      int sock_err_size = sizeof(sock_err);
+      getsockopt(nonblock_sock, SOL_SOCKET, SO_ERROR,
+                 (char *)&sock_err, &sock_err_size);
+      errorf("connect failure: %d", sock_err);
+      return -1;
+    }
+
+    ret = swrite(nonblock_sock, data + nwritten, nbytes - nwritten);
+    if(ret <= 0) {
+      if(SOCK_EAGAIN(SOCKERRNO))
+        continue;
+
+      errorf("socket write error: %d", SOCKERRNO);
+      return -1;
+    }
+
+    nwritten += ret;
+  } while(nwritten < nbytes);
+
+  return 0;
+}
+
+static int read_auth_val(curl_socket_t sock, uint64_t* auth_val_ptr)
+{
+  size_t nread = 0;
+
+  do {
+    ssize_t ret = sread(sock, (unsigned char *)auth_val_ptr + nread,
+                        sizeof(*auth_val_ptr) - nread);
+    if(ret <= 0) {
+      if(!ret)
+        errorf("stdin relay peer disconnected");
+      else
+        errorf("read error: %d", SOCKERRNO);
+
+      return -1;
+    }
+    nread += ret;
+  } while(nread < sizeof(*auth_val_ptr));
+
   return 0;
 }
 
@@ -791,34 +828,26 @@ ThreadCleanup:
 curl_socket_t win32_stdin_read_thread(void)
 {
   int rc = 0;
-  struct win_thread_data *tdata = NULL;
+  HANDLE stdin_thread = NULL;
-  HANDLE stdin_handle = NULL;
-  uint64_t auth_rnd;
-  size_t nwritten = 0;
-  static HANDLE stdin_thread = NULL;
   static curl_socket_t socket_r = CURL_SOCKET_BAD;
+  curl_socket_t socket_l = CURL_SOCKET_BAD;
+  uint64_t auth_rnd = 0;
+  uint64_t recvd_val = 1;
 
   if(socket_r != CURL_SOCKET_BAD) {
-    DEBUGASSERT(stdin_thread);
     return socket_r;
   }
-  DEBUGASSERT(!stdin_thread);
 
   do {
     curl_socklen_t socksize = 0;
     struct sockaddr_in selfaddr;
 
-    /* Prepare handles for thread */
+    InitializeCriticalSection(&tdata.crit_sect);
-    tdata = (struct win_thread_data *)
+
-      curlx_calloc(1, sizeof(struct win_thread_data));
+    /* Create the listening socket. It is used to create the writing socket by
-    if(!tdata) {
+     * accepting a connection from the reading socket. */
-      errorf("curlx_calloc() error");
+    socket_l = CURL_SOCKET(AF_INET, SOCK_STREAM, IPPROTO_TCP);
-      break;
+    if(socket_l == CURL_SOCKET_BAD) {
-    }
-    /* Create the listening socket for the thread. When it starts, it accepts
-     * our connection and begin writing STDIN data to the connection. */
-    tdata->socket_l = CURL_SOCKET(AF_INET, SOCK_STREAM, IPPROTO_TCP);
-    if(tdata->socket_l == CURL_SOCKET_BAD) {
       errorf("socket() error: %d", SOCKERRNO);
       break;
     }
@@ -828,50 +857,23 @@ curl_socket_t win32_stdin_read_thread(void)
     selfaddr.sin_family = AF_INET;
     selfaddr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
     /* Bind to any available loopback port */
-    if(bind(tdata->socket_l, (const struct sockaddr *)&selfaddr, socksize)) {
+    if(bind(socket_l, (const struct sockaddr *)&selfaddr, socksize)) {
       errorf("bind error: %d", SOCKERRNO);
       break;
     }
 
     /* Retrieve the assigned loopback port/address */
-    if(getsockname(tdata->socket_l, (struct sockaddr *)&selfaddr, &socksize)) {
+    if(getsockname(socket_l, (struct sockaddr *)&selfaddr, &socksize)) {
       errorf("getsockname error: %d", SOCKERRNO);
       break;
     }
 
-    if(listen(tdata->socket_l, 1)) {
+    if(listen(socket_l, 1)) {
       errorf("listen error: %d", SOCKERRNO);
       break;
     }
 
-    if(curlx_win32_random((unsigned char *)&auth_rnd, sizeof(auth_rnd))) {
+    /* Create the reading socket */
-      errorf("curlx_win32_random() error");
-      break;
-    }
-    tdata->expected_auth_val = auth_rnd;
-
-    /* Make a copy of the stdin handle to be used by win_stdin_thread_func */
-    if(!DuplicateHandle(GetCurrentProcess(), GetStdHandle(STD_INPUT_HANDLE),
-                        GetCurrentProcess(), &stdin_handle,
-                        0, FALSE, DUPLICATE_SAME_ACCESS)) {
-      errorf("DuplicateHandle error: 0x%08lx", GetLastError());
-      break;
-    }
-    tdata->stdin_handle = stdin_handle;
-
-    /* Start up the thread. We do not bother keeping a reference to it
-       because it runs until program termination. From here on out all reads
-       from the stdin handle or file descriptor 0 is reading from the
-       socket that is fed by the thread. */
-    stdin_thread = CreateThread(NULL, 0, win_stdin_thread_func,
-                                tdata, 0, NULL);
-    if(!stdin_thread) {
-      errorf("CreateThread error: 0x%08lx", GetLastError());
-      break;
-    }
-    tdata = NULL; /* win_stdin_thread_func owns it now */
-
-    /* Connect to the thread and rearrange our own STDIN handles */
     socket_r = CURL_SOCKET(AF_INET, SOCK_STREAM, IPPROTO_TCP);
     if(socket_r == CURL_SOCKET_BAD) {
       errorf("socket error: %d", SOCKERRNO);
@@ -881,69 +883,122 @@ curl_socket_t win32_stdin_read_thread(void)
     /* Hard close the socket on closesocket() */
     setsockopt(socket_r, SOL_SOCKET, SO_DONTLINGER, 0, 0);
 
-    if(connect(socket_r, (const struct sockaddr *)&selfaddr, socksize)) {
+    /* Make the reading socket nonblocking */
-      errorf("connect error: %d", SOCKERRNO);
+    if(curlx_nonblock(socket_r, TRUE)) {
+      errorf("curlx_nonblock() error");
       break;
     }
 
-    do {
+    /* Connect to the listening socket */
-      ssize_t ret = swrite(socket_r, ((unsigned char *)&auth_rnd) + nwritten,
+    if(connect(socket_r, (const struct sockaddr *)&selfaddr, socksize)) {
-                           sizeof(auth_rnd) - nwritten);
+      int sockerr = SOCKERRNO;
-
+      if(!SOCK_EAGAIN(sockerr)) {
-      if(ret <= 0) {
+        errorf("connect error: %d", sockerr);
-        errorf("socket write error: %d", SOCKERRNO);
+        break;
-        goto err;
       }
+    }
 
-      nwritten += ret;
+    /* Accept the connection on the other end, creating the writing socket
-    } while(nwritten < sizeof(auth_rnd));
+     * which will be given to the background thread */
+    tdata.socket_w = CURL_ACCEPT(socket_l, NULL, NULL);
+
+    if(tdata.socket_w == CURL_SOCKET_BAD) {
+      errorf("accept error: %d", SOCKERRNO);
+      break;
+    }
+
+    /* We don't need the listening socket anymore */
+    sclose(socket_l);
+    socket_l = CURL_SOCKET_BAD;
+
+    /* Authenticate the reading socket to the writing socket to make sure
+     * we don't leak information.*/
+    if(curlx_win32_random((unsigned char *)&auth_rnd, sizeof(auth_rnd))) {
+      errorf("curlx_win32_random() error");
+      break;
+    }
+
+    if(swrite_blocking_on_nonblock(socket_r, (unsigned char *)&auth_rnd,
+                                   sizeof(auth_rnd)))
+      break;
+
+    if(read_auth_val(tdata.socket_w, &recvd_val))
+      break;
+
+    if(recvd_val != auth_rnd) {
+      errorf("relay peer auth failed");
+      break;
+    }
+
+    if(shutdown(tdata.socket_w, SHUT_RD)) {
+      errorf("shutdown error: %d", SOCKERRNO);
+      break;
+    }
 
     if(shutdown(socket_r, SHUT_WR)) {
       errorf("shutdown error: %d", SOCKERRNO);
       break;
     }
 
+    /* Make a copy of the stdin handle to be used by win_stdin_thread_func */
+    if(!DuplicateHandle(GetCurrentProcess(), GetStdHandle(STD_INPUT_HANDLE),
+                        GetCurrentProcess(), &tdata.stdin_handle,
+                        0, FALSE, DUPLICATE_SAME_ACCESS)) {
+      errorf("DuplicateHandle error: 0x%08lx", GetLastError());
+      break;
+    }
+
     /* Set the stdin handle to read from the socket. */
     if(SetStdHandle(STD_INPUT_HANDLE, (HANDLE)socket_r) == 0) {
       errorf("SetStdHandle error: 0x%08lx", GetLastError());
       break;
     }
 
+    /* Start up the thread. We do not bother keeping a reference to it
+       because it runs until program termination. From here on out all reads
+       from the stdin handle or file descriptor 0 is reading from the
+       socket that is fed by the thread. */
+    stdin_thread = CreateThread(NULL, 0, win_stdin_thread_func,
+                                NULL, 0, NULL);
+    if(!stdin_thread) {
+      errorf("CreateThread error: 0x%08lx", GetLastError());
+      break;
+    }
+    CloseHandle(stdin_thread);
+
+    /* Starting the thread is the last thing we do, since there aren't any
+     * reliable ways to close it in case of subsequent errors. */
+
     rc = 1;
   } while(0);
 
-err:
   if(rc != 1) {
-    if(stdin_thread) {
+    /* we rely on the background thread not running at this point, as there
-      TerminateThread(stdin_thread, 1);
+     * could be TOCTOU bugs otherwise */
-      CloseHandle(stdin_thread);
-      stdin_thread = NULL;
-    }
-
     if(socket_r != CURL_SOCKET_BAD) {
       if(GetStdHandle(STD_INPUT_HANDLE) == (HANDLE)socket_r &&
-         stdin_handle) {
+         tdata.stdin_handle) {
         /* restore STDIN */
-        SetStdHandle(STD_INPUT_HANDLE, stdin_handle);
+        SetStdHandle(STD_INPUT_HANDLE, tdata.stdin_handle);
-        stdin_handle = NULL;
+        tdata.stdin_handle = NULL;
       }
 
       sclose(socket_r);
       socket_r = CURL_SOCKET_BAD;
     }
 
-    if(tdata) {
+    if(socket_l != CURL_SOCKET_BAD)
-      if(tdata->stdin_handle)
+      sclose(socket_l);
-        CloseHandle(tdata->stdin_handle);
-      if(tdata->socket_l != CURL_SOCKET_BAD)
-        sclose(tdata->socket_l);
 
-      curlx_free(tdata);
+    cleanup_tdata_sync();
-    }
+    DeleteCriticalSection(&tdata.crit_sect);
 
     return CURL_SOCKET_BAD;
   }
 
+  /* prevent mem leak warnings */
+  atexit(&cleanup_tdata_sync);
+
   DEBUGASSERT(socket_r != CURL_SOCKET_BAD);
   return socket_r;
 }

tests/data/test1498

@@ -26,9 +26,6 @@ blablabla
 <server>
 http
 </server>
-<features>
-!win32
-</features>
 <name>
 HTTP PUT from stdin using period
 </name>