Fixes two bugs in the Windows thread / pthread translation layer

1. If threads are resized the threads' `ZSTD_pthread_t` might move
while the worker still holds a pointer into it (see more details in #3120).
2. The join operation was waiting for a thread and then return its `thread.arg`
as a return value, but since the `ZSTD_pthread_t thread` was passed by value it
would have a stale `arg` that wouldn't match the thread's actual return value.

This fix changes the `ZSTD_pthread_join` API and removes support for returning
a value. This means that we are diverging from the `pthread_join` API and this
is no longer just an alias.
In the future, if needed, we could return a Windows thread's return value using
`GetExitCodeThread`, but as this path wouldn't be excised in any case, it's
preferable to not add it right now.

diff --git a/lib/common/pool.c b/lib/common/pool.c
index bf21c57ed66343764bae433145fb0845758b33d0..63dc9d37789a48ef40797f09f819ecf207d55637 100644 (file)
--- a/lib/common/pool.c
+++ b/lib/common/pool.c
@@ -173,7 +173,7 @@ static void POOL_join(POOL_ctx* ctx) {
     /* Join all of the threads */
     {   size_t i;
         for (i = 0; i < ctx->threadCapacity; ++i) {
-            ZSTD_pthread_join(ctx->threads[i], NULL);  /* note : could fail */
+            ZSTD_pthread_join(ctx->threads[i]);  /* note : could fail */
     }   }
 }
 

diff --git a/lib/common/threading.c b/lib/common/threading.c
index c62c65d57e9be7a5b211461035eeb32d6ae218b3..6bbb1493734ecf1cc6d9951684be9d61990c601b 100644 (file)
--- a/lib/common/threading.c
+++ b/lib/common/threading.c
@@ -37,7 +37,7 @@ int g_ZSTD_threading_useless_symbol;
 static unsigned __stdcall worker(void *arg)
 {
     ZSTD_pthread_t* const thread = (ZSTD_pthread_t*) arg;
-    thread->arg = thread->start_routine(thread->arg);
+    thread->start_routine(thread->arg);
     return 0;
 }
 
@@ -55,7 +55,7 @@ int ZSTD_pthread_create(ZSTD_pthread_t* thread, const void* unused,
         return 0;
 }
 
-int ZSTD_pthread_join(ZSTD_pthread_t thread, void **value_ptr)
+int ZSTD_pthread_join(ZSTD_pthread_t thread)
 {
     DWORD result;
 
@@ -66,7 +66,6 @@ int ZSTD_pthread_join(ZSTD_pthread_t thread, void **value_ptr)
 
     switch (result) {
     case WAIT_OBJECT_0:
-        if (value_ptr) *value_ptr = thread.arg;
         return 0;
     case WAIT_ABANDONED:
         return EINVAL;

diff --git a/lib/common/threading.h b/lib/common/threading.h
index b1458054eee4cacfe59a56fe478b7b423d010624..603d479c7fad14754239bd46dd1b06a5dcfa8745 100644 (file)
--- a/lib/common/threading.h
+++ b/lib/common/threading.h
@@ -70,7 +70,7 @@ typedef struct {
 int ZSTD_pthread_create(ZSTD_pthread_t* thread, const void* unused,
                    void* (*start_routine) (void*), void* arg);
 
-int ZSTD_pthread_join(ZSTD_pthread_t thread, void** value_ptr);
+int ZSTD_pthread_join(ZSTD_pthread_t thread);
 
 /**
  * add here more wrappers as required
@@ -98,7 +98,7 @@ int ZSTD_pthread_join(ZSTD_pthread_t thread, void** value_ptr);
 
 #define ZSTD_pthread_t                  pthread_t
 #define ZSTD_pthread_create(a, b, c, d) pthread_create((a), (b), (c), (d))
-#define ZSTD_pthread_join(a, b)         pthread_join((a),(b))
+#define ZSTD_pthread_join(a)         pthread_join((a),NULL)
 
 #else /* DEBUGLEVEL >= 1 */
 
@@ -123,7 +123,7 @@ int ZSTD_pthread_cond_destroy(ZSTD_pthread_cond_t* cond);
 
 #define ZSTD_pthread_t                  pthread_t
 #define ZSTD_pthread_create(a, b, c, d) pthread_create((a), (b), (c), (d))
-#define ZSTD_pthread_join(a, b)         pthread_join((a),(b))
+#define ZSTD_pthread_join(a)         pthread_join((a),NULL)
 
 #endif
 

diff --git a/tests/fuzzer.c b/tests/fuzzer.c
index 9d1862077a0a67464c9c6bda5ada1c7e26ea3ad3..d58751a5cd863d9913b292582f03ec3d749823c7 100644 (file)
--- a/tests/fuzzer.c
+++ b/tests/fuzzer.c
@@ -431,8 +431,8 @@ static int threadPoolTests(void) {
 
         ZSTD_pthread_create(&t1, NULL, threadPoolTests_compressionJob, &p1);
         ZSTD_pthread_create(&t2, NULL, threadPoolTests_compressionJob, &p2);
-        ZSTD_pthread_join(t1, NULL);
-        ZSTD_pthread_join(t2, NULL);
+        ZSTD_pthread_join(t1);
+        ZSTD_pthread_join(t2);
 
         assert(!memcmp(decodedBuffer, decodedBuffer2, CNBuffSize));
         free(decodedBuffer2);