win32: use _endthreadex to terminate threads, not ExitThread

PATCH Superseded Rose via GitGitGadget 3 participants 5 messages Dec 22, 2022 – Dec 25, 2022

lore

Share & tools

lore

Rose via GitGitGadget

author

2022-12-22 20:59 UTC #0 lore raw

PATCH 1/1 win32: use _endthreadex to terminate threads, not ExitThread

From: Seija Kijin <[email protected]>

This is a pretty serious bug actually:
Because we use the C runtime and use _beginthreadex to create pthreads, pthread_exit MUST use _endthreadex.

Otherwise, according to Microsoft:
"Failure to do so results in small memory leaks when the thread calls ExitThread."

Simply put, this is not the same as ExitThread.

Signed-off-by Seija Kijin <[email protected]>

---
win32: use _endthreadex to terminate threads, not ExitThread

This is a pretty serious bug actually: Because we use the C runtime and use _beginthread to create pthreads, pthread_exit MUST use _endthread.

Otherwise, according to Microsoft: "Failure to do so results in small memory leaks when the thread calls ExitThread."

Simply put, this is not the same as ExitThread.

Signed-off-by Seija Kijin [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1414%2FAtariDreams%2Fsevere-bug-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1414/AtariDreams/severe-bug-v1
Pull-Request: https://github.com/git/git/pull/1414

 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

compat/win32/pthread.h +1 -1

index 737983d00ba..cc3221cb2c8 100644
@@ -66,7 +66,7 @@ pthread_t pthread_self(void);
 
 static inline void NORETURN pthread_exit(void *ret)
 {
-	ExitThread((DWORD)(intptr_t)ret);
+	_endthreadex((unsigned)(uintptr_t)ret);
 }
 
 typedef DWORD pthread_key_t;

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

Signature · 1 line

gitgitgadget

Johannes Sixt

Acked-by on v1 1/1

2022-12-24 08:01 UTC #1 lore raw

Am 22.12.22 um 21:59 schrieb Rose via GitGitGadget:

Quoted 25 lines

From: Seija Kijin <[email protected]>
This is a pretty serious bug actually:
Because we use the C runtime and use _beginthreadex to create pthreads, pthread_exit MUST use _endthreadex.
Otherwise, according to Microsoft:
"Failure to do so results in small memory leaks when the thread calls ExitThread."
Simply put, this is not the same as ExitThread.
Signed-off-by Seija Kijin <[email protected]>
---
 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
compat/win32/pthread.h +0 -0
index 737983d00ba..cc3221cb2c8 100644

compat/win32/pthread.h pthread_t pthread_self(void); · line 72

Quoted 8 lines

compat/win32/pthread.h +1 -1

@@ -66,7 +66,7 @@ pthread_t pthread_self(void);
 
 static inline void NORETURN pthread_exit(void *ret)
 {
-	ExitThread((DWORD)(intptr_t)ret);
+	_endthreadex((unsigned)(uintptr_t)ret);
 }
 
 typedef DWORD pthread_key_t;

Nice find! FWIW, this passes the test suite on Windows.

The patch text is:

Acked-by Johannes Sixt <[email protected]>

The commit message is highly exaggerated, though. The bug is by no means serious. After all, we've been living with it for a decade. Notice that pthread_exit() is only called when a thread runs into die(). Even though we have a small memory leak, it does not occur in a loop because, after one thread dies, we do not tend to keep starting many, many more that all die.

-- Hannes

Rose via GitGitGadget

author

2022-12-25 01:41 UTC #2 lore raw

PATCH v2 1/1 win32: use _endthreadex to terminate threads, not ExitThread

In reply to Rose via GitGitGadget

From: Seija Kijin <[email protected]>

Because we use the C runtime and use _beginthreadex to create pthreads, pthread_exit MUST use _endthreadex.

Otherwise, according to Microsoft:
"Failure to do so results in small memory leaks when the thread calls ExitThread."

Simply put, this is not the same as ExitThread.

Signed-off-by Seija Kijin <[email protected]>

---
win32: use _endthreadex to terminate threads, not ExitThread

Because we use the C runtime and use _beginthread to create pthreads, pthread_exit MUST use _endthread.

Otherwise, according to Microsoft: "Failure to do so results in small memory leaks when the thread calls ExitThread."

Simply put, this is not the same as ExitThread.

Signed-off-by Seija Kijin [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1414%2FAtariDreams%2Fsevere-bug-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1414/AtariDreams/severe-bug-v2
Pull-Request: https://github.com/git/git/pull/1414

Range-diff vs v1:

1: 78f9d54c304 ! 1: 3e8212fb9a7 win32: use _endthreadex to terminate threads, not ExitThread
@@ Metadata
## Commit message ##
win32: use _endthreadex to terminate threads, not ExitThread

- This is a pretty serious bug actually:
Because we use the C runtime and use _beginthreadex to create pthreads, pthread_exit MUST use _endthreadex.

 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

compat/win32/pthread.h +1 -1

index 737983d00ba..cc3221cb2c8 100644
@@ -66,7 +66,7 @@ pthread_t pthread_self(void);
 
 static inline void NORETURN pthread_exit(void *ret)
 {
-	ExitThread((DWORD)(intptr_t)ret);
+	_endthreadex((unsigned)(uintptr_t)ret);
 }
 
 typedef DWORD pthread_key_t;

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

Signature · 1 line

gitgitgadget

Johannes Sixt

Acked-by on v2 1/1

2022-12-25 08:54 UTC #3 lore raw

Am 25.12.22 um 02:41 schrieb Rose via GitGitGadget:

Quoted 61 lines

From: Seija Kijin <[email protected]>
Because we use the C runtime and use _beginthreadex to create pthreads, pthread_exit MUST use _endthreadex.
Otherwise, according to Microsoft:
"Failure to do so results in small memory leaks when the thread calls ExitThread."
Simply put, this is not the same as ExitThread.
Signed-off-by Seija Kijin <[email protected]>
---
win32: use _endthreadex to terminate threads, not ExitThread
Because we use the C runtime and use _beginthread to create pthreads, pthread_exit MUST use _endthread.
Otherwise, according to Microsoft: "Failure to do so results in small memory leaks when the thread calls ExitThread."
Simply put, this is not the same as ExitThread.
Signed-off-by Seija Kijin [email protected]
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1414%2FAtariDreams%2Fsevere-bug-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1414/AtariDreams/severe-bug-v2
Pull-Request: https://github.com/git/git/pull/1414
Range-diff vs v1:
1: 78f9d54c304 ! 1: 3e8212fb9a7 win32: use _endthreadex to terminate threads, not ExitThread
@@ Metadata
## Commit message ##
win32: use _endthreadex to terminate threads, not ExitThread
- This is a pretty serious bug actually:
Because we use the C runtime and use _beginthreadex to create pthreads, pthread_exit MUST use _endthreadex.
 compat/win32/pthread.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
compat/win32/pthread.h +1 -1
index 737983d00ba..cc3221cb2c8 100644
@@ -66,7 +66,7 @@ pthread_t pthread_self(void);
 
 static inline void NORETURN pthread_exit(void *ret)
 {
-	ExitThread((DWORD)(intptr_t)ret);
+	_endthreadex((unsigned)(uintptr_t)ret);
 }
 
 typedef DWORD pthread_key_t;

Quoted 1 line

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

Thank you! This patch is now

Acked-by Johannes Sixt <[email protected]>

-- Hannes

Junio C Hamano

2022-12-25 12:45 UTC #4 lore raw

Quoted 6 lines · Johannes Sixt

Am 25.12.22 um 02:41 schrieb Rose via GitGitGadget:
Quoted 2 lines
From: Seija Kijin <[email protected]>
...
Thank you! This patch is now
Acked-by Johannes Sixt <[email protected]>

Thanks. Will queue.