Skip to content

Commit

Permalink
Fixes two bugs in the Windows thread / pthread translation layer
Browse files Browse the repository at this point in the history
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 facebook#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.
  • Loading branch information
yoniko committed Dec 17, 2022
1 parent fba704f commit 500f02e
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 9 deletions.
2 changes: 1 addition & 1 deletion lib/common/pool.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
} }
}

Expand Down
5 changes: 2 additions & 3 deletions lib/common/threading.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;

Expand All @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions lib/common/threading.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 */

Expand All @@ -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

Expand Down
4 changes: 2 additions & 2 deletions tests/fuzzer.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 500f02e

Please sign in to comment.