← Commit list

67221a7882

11soda11

2026-08-25

SUCCESS Duration: 386s

Subject

tool_doswin: don't use `TerminateThread` in stdin relay

Binary

No binary (build failed)

Architecture & Compiler Flags

Architecture: x86_64-linux-gnu Compiler: gcc (Ubuntu 11.4.0-1ubuntu1~22.04.3) 11.4.0

—

Build Log

(log yok)

Commit Diff GitHub →

.github/workflows/windows.yml +0 -3 src/tool_doswin.c +188 -133 tests/data/test1498 +0 -3

.github/workflows/windows.yml+0 -3

@@ -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+188 -133

@@ -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
-	connection. */
-	curl_socket_t socket_l;
-	/* This is the random number which the background thread uses to verify
-	the peer. */
-	uint64_t expected_auth_val;
-	};
-
-	static DWORD WINAPI win_stdin_thread_func(void *thread_data)
+	/* This is the socket the thread will forward stdin to. It is connected to
+	the socket which replaces the stdin handle. */
+	curl_socket_t socket_w;
+	/* This is a mutex-like object which we use to synchronize
+	* cleanup_tdata_sync() */
+	CRITICAL_SECTION crit_sect;
+	} tdata = { NULL, CURL_SOCKET_BAD, {0}};
+
+	static void cleanup_tdata_sync(void)
	{
-	struct win_thread_data tdata = (struct win_thread_data )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;
+	EnterCriticalSection(&tdata.crit_sect);
+	if(tdata.stdin_handle) {
+	CloseHandle(tdata.stdin_handle);
+	tdata.stdin_handle = NULL;
	}

-	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(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)
+	{
+	(void)thread_data;

-	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_handle = NULL;
-	uint64_t auth_rnd;
-	size_t nwritten = 0;
-	static HANDLE stdin_thread = NULL;
+	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 */
-	tdata = (struct win_thread_data *)
-	curlx_calloc(1, sizeof(struct win_thread_data));
-	if(!tdata) {
-	errorf("curlx_calloc() error");
-	break;
-	}
-	/* 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) {
+	InitializeCriticalSection(&tdata.crit_sect);
+
+	/* Create the listening socket. It is used to create the writing socket by
+	* accepting a connection from the reading socket. */
+	socket_l = CURL_SOCKET(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+	if(socket_l == CURL_SOCKET_BAD) {
	errorf("socket() error: %d", SOCKERRNO);
	break;
	}
@@ -828,122 +857,148 @@ 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))) {
-	errorf("curlx_win32_random() error");
+	/* Create the reading socket */
+	socket_r = CURL_SOCKET(AF_INET, SOCK_STREAM, IPPROTO_TCP);
+	if(socket_r == CURL_SOCKET_BAD) {
+	errorf("socket error: %d", SOCKERRNO);
	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());
+	/* Hard close the socket on closesocket() */
+	setsockopt(socket_r, SOL_SOCKET, SO_DONTLINGER, 0, 0);
+
+	/* Make the reading socket nonblocking */
+	if(curlx_nonblock(socket_r, TRUE)) {
+	errorf("curlx_nonblock() error");
	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;
+	/* Connect to the listening socket */
+	if(connect(socket_r, (const struct sockaddr *)&selfaddr, socksize)) {
+	int sockerr = SOCKERRNO;
+	if(!SOCK_EAGAIN(sockerr)) {
+	errorf("connect error: %d", sockerr);
+	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);
+	/* Accept the connection on the other end, creating the writing socket
+	* 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;
	}

-	/* Hard close the socket on closesocket() */
-	setsockopt(socket_r, SOL_SOCKET, SO_DONTLINGER, 0, 0);
+	/* We don't need the listening socket anymore */
+	sclose(socket_l);
+	socket_l = CURL_SOCKET_BAD;

-	if(connect(socket_r, (const struct sockaddr *)&selfaddr, socksize)) {
-	errorf("connect error: %d", SOCKERRNO);
+	/* 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;
	}

-	do {
-	ssize_t ret = swrite(socket_r, ((unsigned char *)&auth_rnd) + nwritten,
-	sizeof(auth_rnd) - nwritten);
+	if(swrite_blocking_on_nonblock(socket_r, (unsigned char *)&auth_rnd,
+	sizeof(auth_rnd)))
+	break;

-	if(ret <= 0) {
-	errorf("socket write error: %d", SOCKERRNO);
-	goto err;
-	}
+	if(read_auth_val(tdata.socket_w, &recvd_val))
+	break;
+
+	if(recvd_val != auth_rnd) {
+	errorf("relay peer auth failed");
+	break;
+	}

-	nwritten += ret;
-	} while(nwritten < sizeof(auth_rnd));
+	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) {
-	TerminateThread(stdin_thread, 1);
-	CloseHandle(stdin_thread);
-	stdin_thread = NULL;
-	}
-
+	/* we rely on the background thread not running at this point, as there
+	* could be TOCTOU bugs otherwise */
	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);
-	stdin_handle = NULL;
+	SetStdHandle(STD_INPUT_HANDLE, tdata.stdin_handle);
+	tdata.stdin_handle = NULL;
	}

	sclose(socket_r);
	socket_r = CURL_SOCKET_BAD;
	}

-	if(tdata) {
-	if(tdata->stdin_handle)
-	CloseHandle(tdata->stdin_handle);
-	if(tdata->socket_l != CURL_SOCKET_BAD)
-	sclose(tdata->socket_l);
+	if(socket_l != CURL_SOCKET_BAD)
+	sclose(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+0 -3

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