Data race on `ctx->threadLimit` between `POOL_thread()` and `POOL_create_advanced()`

[guoyuqi020]

It seems that the ctx->threadLimit variable is updated in POOL_create_advanced() without being protected by the ctx->queueMutex. This can lead to a data race. Will this be a problem?

zstd/lib/common/pool.c

Lines 67 to 90 in bfd8ad8

	static void* POOL_thread(void* opaque) {
	POOL_ctx* const ctx = (POOL_ctx*)opaque;
	if (!ctx) { return NULL; }
	for (;;) {
	/* Lock the mutex and wait for a non-empty queue or until shutdown */
	ZSTD_pthread_mutex_lock(&ctx->queueMutex);
	while ( ctx->queueEmpty
	|| (ctx->numThreadsBusy >= ctx->threadLimit) ) {
	if (ctx->shutdown) {
	/* even if !queueEmpty, (possible if numThreadsBusy >= threadLimit),
	* a few threads will be shutdown while !queueEmpty,
	* but enough threads will remain active to finish the queue */
	ZSTD_pthread_mutex_unlock(&ctx->queueMutex);
	return opaque;
	}
	ZSTD_pthread_cond_wait(&ctx->queuePopCond, &ctx->queueMutex);
	}
	/* Pop a job off the queue */
	{ POOL_job const job = ctx->queue[ctx->queueHead];
	ctx->queueHead = (ctx->queueHead + 1) % ctx->queueSize;
	ctx->numThreadsBusy++;
	ctx->queueEmpty = (ctx->queueHead == ctx->queueTail);
	/* Unlock the mutex, signal a pusher, and run the job */

zstd/lib/common/pool.c

Lines 143 to 159 in bfd8ad8

	ctx->threads = (ZSTD_pthread_t*)ZSTD_customCalloc(numThreads * sizeof(ZSTD_pthread_t), customMem);
	ctx->threadCapacity = 0;
	ctx->customMem = customMem;
	/* Check for errors */
	if (!ctx->threads || !ctx->queue) { POOL_free(ctx); return NULL; }
	/* Initialize the threads */
	{ 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;
	ctx->threadLimit = numThreads;
	}
	return ctx;

[sebres]

There is basically no RC at the moment, 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:

zstd/lib/common/pool.c

Line 72 in ae9f20c

ZSTD_pthread_mutex_lock(&ctx->queueMutex);

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:

zstd/lib/common/pool.c

Lines 74 to 75 in ae9f20c

	while ( ctx->queueEmpty
	|| (ctx->numThreadsBusy >= ctx->threadLimit) ) {

But to avoid possible RC in the future (if the logic would be not enough anymore) one could indeed rewrite it a bit.
The naive attempt to "fix" it with lock around set of #4558 is in my opinion not correct (even wise versa, can cause a sporadic deadlock).
I'll provide a new PR for that.