[Eshan05]

What and why

Fixes #4862.

With nbWorkers >= 1, a caller can abandon a frame while one of its jobs is queued or running. The context then frees memory the job still uses, without waiting for it. This happens in four places:

• ZSTD_freeCCtx() frees the dictionary loaded with ZSTD_CCtx_loadDictionary*() before ZSTDMT_freeCCtx() joins the workers. This is the case reported in ZSTD_freeCCtx() frees a dictionary loaded with ZSTD_CCtx_loadDictionary() before stopping the worker threads #4862.
• ZSTD_CCtx_reset(session_and_parameters) frees the same dictionary while the job runs. So does ZSTD_CCtx_reset(session_only) followed by ZSTD_CCtx_loadDictionary*(), refCDict() or refPrefix(). The reorder suggested in the issue does not cover these.
• With ZSTD_CCtx_refThreadPool(), ZSTDMT_freeCCtx() does not stop the shared pool. It frees the jobs table and the CCtx pool while a worker still holds a CCtx from that pool. No dictionary is needed for this one.
• After ZSTD_CCtx_reset(session_only), raising nbWorkers makes ZSTDMT_initCStream_internal() resize the CCtx pool before it waits for the abandoned jobs. The resize frees the CCtx a worker is using.

AddressSanitizer reports a heap-use-after-free for all four on dev.

The change moves the existing "previous compression not correctly finished" block of ZSTDMT_initCStream_internal() into a new function, ZSTDMT_waitForPendingJobs(). It calls that function in three places:

• in ZSTD_CCtx_reset() when the session is reset, so no job of an abandoned frame is still running when the reset returns;
• at the start of ZSTDMT_freeCCtx(), before anything is freed, which also covers a shared pool;
• in ZSTDMT_initCStream_internal() before ZSTDMT_resize() instead of after it.

ZSTD_freeCCtxContent() now frees the MT context before the dictionaries.

After a completed frame, allJobsCompleted is already 1 and these calls do nothing, so the normal path does not change. A session reset now blocks until the abandoned frame's running jobs finish. Before this change, ZSTDMT_initCStream_internal() already waited for them at the start of the next frame, and ZSTD_freeCCtx() already joined an owned pool. I added a line about this to the ZSTD_CCtx_reset() comment in zstd.h.

Test

I added a test to tests/zstreamtest.c, "abandon a multi-threaded frame with a job in progress". It starts a 1 MB job with a 1 MB dictionary, and then:

1. frees the context;
2. frees the context when it uses a shared pool;
3. resets the session, loads a new dictionary and round-trips a frame;
4. resets the session, sets nbWorkers to 4 and round-trips a frame.

Without the fix, the test fails under AddressSanitizer with heap-use-after-free in 3 of 3 runs. With the fix, it passes.

On Windows x64 with MSVC 14.44 and /fsanitize=address:

• the reproducer from ZSTD_freeCCtx() frees a dictionary loaded with ZSTD_CCtx_loadDictionary() before stopping the worker threads #4862 and each path above run clean, 5 runs each;
• zstreamtest -v -T90s, zstreamtest --newapi -t1 -T90s and fuzzer -v -T200s pass with no AddressSanitizer reports;
• the changed files compile with gcc 13 (MinGW) using the lib/ and tests/ warning flags and -Werror, with and without ZSTD_MULTITHREAD.

On Ubuntu 24.04 with clang 18:

• make uasan-test-zstream passes;
• make tsan-test-zstream with -T3mn and make tsan-fuzztest pass with no ThreadSanitizer reports. My VM has 2 GB of memory, and the two hashLog = 29 tests run out of memory under TSan, so I disabled them locally. These are zstreamtest test 70 and fuzzer test 214;
• the reproducer and each path above run clean under TSan.

Checklist

• I searched open pull requests and issues: this problem is not already being addressed.
• Bug fix: it comes with a reproducer or a test that fails without the fix.
• Performance change: numbers from repeated runs are included, with CPU, compiler, flags and data.
• I stated below whether AI or other automated tools were used.

Tools used

Note

Claude Code was used to reproduce the issue, to find the shared-pool and resize cases while reading the code, and to write the change and the test.

I used AddressSanitizer to check each case before and after the change, and ThreadSanitizer to check the fixed code.

[meta-cla]

Hi @Eshan05!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[meta-cla]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[Cyan4973]

Thank you for this fix, the clear description and the test. We keep the change as is: under AddressSanitizer, each of the four scenarios of the test fails without the library change, and passes with it.

Your change waits for the jobs with ZSTDMT_waitForAllJobsCompleted(), whose loop runs while consumed < src.size. This is already false for an empty job before its worker starts. I pushed three commits on top of your branch:

• compress: wait for an empty job before releasing it. An empty frame has one empty job. When ZSTD_e_end returns because the output buffer is full and the frame is abandoned, ZSTD_freeCCtx() now clears this job before POOL_free() joins the worker, and the worker dereferences a NULL pool. Before this PR, POOL_free() joined the worker first. The worker now sets a completed flag when it no longer uses the job, and the wait uses this flag.
• compress: clear jobReady when the jobs are released. When a frame is abandoned while a prepared job waits for a free worker, ZSTDMT_releaseAllJobResources() clears that job but leaves jobReady set, so the next frame posts the cleared job. This was already the case before this PR.
• tests: shorten the abandoned multi-threaded frame test. The test has one scenario per library change, and each scenario fails when its change is removed: 0 to 2 free the context (with a dictionary, with a shared pool, after an empty frame), 3 and 4 reset the session and compress a new frame (with a new dictionary, with a job waiting for the worker). I removed the scenario that raises nbWorkers after a reset: since ZSTD_CCtx_reset() now waits for the jobs, it can only fail when the new dictionary scenario fails too. The new frames are no longer decompressed, since these bugs show as memory errors in the worker.