Skip to content

Fix data race in POOL_create_advanced by protecting threadLimit #4558

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
richardsonnick wants to merge 1 commit into from
Closed

Fix data race in POOL_create_advanced by protecting threadLimit #4558

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/common/pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,15 +146,20 @@ POOL_ctx* POOL_create_advanced(size_t numThreads, size_t queueSize,
/* Check for errors */
if (!ctx->threads || !ctx->queue) { POOL_free(ctx); return NULL; }
/* Initialize the threads */
{ size_t i;
{
size_t i;
for (i = 0; i < numThreads; ++i) {
if (ZSTD_pthread_create(&ctx->threads[i], NULL, &POOL_thread, ctx)) {
ctx->threadCapacity = i;
POOL_free(ctx);
return NULL;
} }
}
}
ctx->threadCapacity = numThreads;

ZSTD_pthread_mutex_lock(&ctx->queueMutex);

@sebres sebres Mar 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding the lock around the set of threadLimit wouldn't prevent possible RC at all, because the threads going created before the main thread would enter the lock here...
Let alone such set is nearby to atomic operation, so it is strange to try to protect it (alone) with a mutex.
Not to mention that there is basically no RC, since the ctx (created here) isn't used somewhere else yet, excepting the workers in pool, where the first of them would hold the lock in:
https://github.com/facebook/zstd/blob/ae9f20ca2716f2605822ca375995b7d876389b64/lib/common/pool.c#L72
and then enter the conditional wait, because of ctx->queueEmpty, no matter what ctx->threadLimit is, because by || (or) the value of (ctx->numThreadsBusy >= ctx->threadLimit) is irrelevant:
https://github.com/facebook/zstd/blob/ae9f20ca2716f2605822ca375995b7d876389b64/lib/common/pool.c#L74-L75
So this lock would not necessarily change something, still worse it may cause a typical deadlock, if the first worker thread would manage to enter the lock, and the main also trying to do that, so nobody would be able to put something to the queue and notify conditional wait to wake the worker up (which could also unlock the mutex for this thread).

@terrelln terrelln Mar 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this could cause a deadlock. But, I agree that it is unnecessary.

@sebres sebres Mar 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this could cause a deadlock

But it can, take a closer look at that scenario:

  1. pool master thread creates a 1st-worker, which enters the POOL_thread and then in line 72 it locks the mutex.
  2. pool master thread creates a 2nd- ... Nth-workers, and the longer it takes (and the more context switches happens) the more likely the 1st-worker manages to enter the aforementioned lock.
  3. before leave the POOL_create_advanced the pool master thread also trying to lock the mutex, but it is locked by 1st-worker (which is waiting for the queue), so the master never leave POOL_create_advanced and ctx remains unknown for whole program, and nobody can add something to the queue to wake up the 1st-worker. It is typical deadlock.

Just for the record.

ctx->threadLimit = numThreads;
ZSTD_pthread_mutex_unlock(&ctx->queueMutex);
}
return ctx;
}
Expand Down