Skip to content
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.

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

Bugfixes for the Block-Level Sequence Producer API #3433

Merged
merged 2 commits into from
Jan 19, 2023
Merged

Conversation

embg
Copy link
Contributor

@embg embg commented Jan 18, 2023

This PR fixes four bugs which we were very lucky to catch before the 1.5.4 release, mostly thanks to testing by @GarenJian-Intel (who caught three of the bugs).

Some of these bugs, it turns out, can be caught by our fuzzers with only a small modification to FUZZ_setRandomParameters(). I got fuzzing working on my machine, was able to reproduce bugs 1, 2, and 4, and will put that code up as a separate PR. I ran the fuzzers for a couple hours and they haven't found any additional bugs (we'll see if OSS-fuzz gets a different result). Will discuss with @terrelln whether it makes sense to modify our fuzzers so they can catch bug 3.

Bug 1

The external matchfinder API is not compatible with nbWorkers > 0 right now. This PR fails compression in that case, rather than failing an assert (debug builds) or crashing the application (release builds).

  • Fix: see changes in ZSTD_CCtx_init_compressStream2()
  • Test: see changes in zstreamtest.c
  • Docs: updated zstd.h

Bug 2

Prior to this PR, some input sizes would lead to an incorrect estimate of memory required for the ZSTD_cwksp. This would cause compression to fail, since the workspace would run out of space for sub-allocations after making the initial shared allocation.

  • Fix: see changes in ZSTD_estimateCCtxSize_usingCCtxParams_internal()
  • Test: this will be covered by the fuzzers in my next PR
  • Docs: n/a

Bug 3

The external matchfinder doesn't interact with older compression APIs, which means that this assert isn't always true. libzstd is behaving correctly, it's just the assert which is incorrect.

  • Fix: delete the assert
  • Test: see changes in zstreamtest.c
  • Docs: updated zstd.h

Bug 4

When I was working on the fuzzers, I realized that I need a way to clear the external matchfinder. After discussing with @Cyan4973, I realized that there was a bug with calls of the form ZSTD_registerExternalMatchFinder(cctx, NULL, NULL), which should clear the matchfinder.

  • Fix: modify ZSTD_registerExternalMatchFinder() such that it clears any previously registered external matchfinder when called with a NULL function pointer.
  • Test: see changes in zstreamtest.c
  • Docs: updated zstd.h

@embg embg requested a review from Cyan4973 January 18, 2023 23:20
@embg embg merged commit bce0382 into facebook:dev Jan 19, 2023
@embg embg deleted the emf_fixes branch January 19, 2023 15:45
@embg embg changed the title Bugfixes for the External Matchfinder API Bugfixes for the External Sequence Producer API Feb 8, 2023
@embg embg changed the title Bugfixes for the External Sequence Producer API Bugfixes for the Block-Level Sequence Producer API Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants