From f00b85dbf45c574271a2cf36f5a863d7b76c1d25 Mon Sep 17 00:00:00 2001
From: Tobias Brunner <tobias@strongswan.org>
Date: Tue, 14 Jul 2026 13:12:33 +0200
Subject: [PATCH] windows: Fix potential use-after-free when joining a thread

Because the flag was set before running the TLS cleanup, a thread
waiting in `join()` could exit the loop and destroy the thread object
before `docleanup()` is called in `end_thread()`.

This change removes the `terminated` flag and instead properly waits for
the thread to exit in `join()`.  By always removing the threads from the
hashtable in `end_thread()`, we also avoid requiring to check any flags
in `cleanup_tls()`, as it now only finds an object for external threads.

We now also make sure to call `docleanup()` before removing the thread
from the hashtable.  Otherwise, if a TLS cleanup callback calls
`thread_current[_id]()`, a new thread object would get created that is
never cleaned up.

Fixes: 0fa9c958114f ("windows: Provide a complete native Windows threading backend")
---
 src/libstrongswan/threading/windows/thread.c | 51 +++++++-------------
 1 file changed, 18 insertions(+), 33 deletions(-)

diff --git a/src/libstrongswan/threading/windows/thread.c b/src/libstrongswan/threading/windows/thread.c
index d2a1e83d1..03925c9f4 100644
--- a/src/libstrongswan/threading/windows/thread.c
+++ b/src/libstrongswan/threading/windows/thread.c
@@ -1,4 +1,5 @@
 /*
+ * Copyright (C) 2014-2026 Tobias Brunner
  * Copyright (C) 2013 Martin Willi
  *
  * Copyright (C) secunet Security Networks AG
@@ -72,11 +73,6 @@ struct private_thread_t {
 	 */
 	hashtable_t *tls;
 
-	/**
-	 * Thread terminated?
-	 */
-	bool terminated;
-
 	/**
 	 * Thread detached?
 	 */
@@ -287,14 +283,12 @@ static void docleanup(private_thread_t *this)
 }
 
 /**
- * Clean up and destroy a thread
+ * Destroy a thread
  */
 static void destroy(private_thread_t *this)
 {
 	bool old;
 
-	docleanup(this);
-
 	old = set_leak_detective(FALSE);
 
 	array_destroy(this->cleanup);
@@ -309,20 +303,17 @@ static void destroy(private_thread_t *this)
 }
 
 /**
- * End a thread, destroy when detached
+ * End and cleanup a thread, destroy when detached
  */
 static void end_thread(private_thread_t *this)
 {
+	docleanup(this);
+	remove_thread(this);
+
 	if (this->detached)
 	{
-		remove_thread(this);
 		destroy(this);
 	}
-	else
-	{
-		this->terminated = TRUE;
-		docleanup(this);
-	}
 }
 
 /**
@@ -398,15 +389,14 @@ METHOD(thread_t, join, void*,
 		return NULL;
 	}
 
-	while (!this->terminated)
+	/* join is a cancellation point, use alertable wait until thread exited */
+	while (WaitForSingleObjectEx(this->handle, INFINITE,
+								 TRUE) == WAIT_IO_COMPLETION)
 	{
-		/* join is a cancellation point, use alertable wait */
-		WaitForSingleObjectEx(this->handle, INFINITE, TRUE);
 	}
 
 	ret = this->ret;
 
-	remove_thread(this);
 	destroy(this);
 
 	return ret;
@@ -631,7 +621,6 @@ void thread_exit(void *val)
 static void cleanup_tls()
 {
 	private_thread_t *this;
-	bool old;
 
 	/* ignore this if called for the thread that called threads_deinit() */
 	if (!threads_lock)
@@ -639,23 +628,17 @@ static void cleanup_tls()
 		return;
 	}
 
-	old = set_leak_detective(FALSE);
+	/* we target externally spawned threads only here, our threads remove
+	 * themselves in end_thread(), which runs before DLL_THREAD_DETACH */
 	threads_lock->lock(threads_lock);
-
-	this = threads->remove(threads, (void*)(uintptr_t)GetCurrentThreadId());
-
+	this = threads->get(threads, (void*)(uintptr_t)GetCurrentThreadId());
 	threads_lock->unlock(threads_lock);
-	set_leak_detective(old);
 
 	if (this)
 	{
-		/* If the thread exited, but has not been joined, it is in terminated
-		 * state. We must not mangle it, as we target externally spawned
-		 * threads only. */
-		if (!this->terminated && !this->detached)
-		{
-			destroy(this);
-		}
+		docleanup(this);
+		remove_thread(this);
+		destroy(this);
 	}
 }
 
@@ -696,7 +679,9 @@ void threads_deinit()
 {
 	private_thread_t *this;
 
-	this = threads->remove(threads, (void*)(uintptr_t)GetCurrentThreadId());
+	this = get_current_thread();
+	docleanup(this);
+	remove_thread(this);
 	destroy(this);
 
 	threads_lock->destroy(threads_lock);