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

Cap hashlog for row based matchfinder, chainlog for short cache matchfinders #3336

Closed
embg opened this issue Dec 8, 2022 · 0 comments · Fixed by #3438
Closed

Cap hashlog for row based matchfinder, chainlog for short cache matchfinders #3336

embg opened this issue Dec 8, 2022 · 0 comments · Fixed by #3438
Assignees
Labels
release-blocking Must be done by the release

Comments

@embg
Copy link
Contributor

embg commented Dec 8, 2022

This assert which was added as part of short cache has uncovered two bugs:

  • The short cache PR only validates that hashLog is <= 24 bits. We need to do the same for chainLog, this has showed up in some CI failures.
  • The row based matchfinder needs to have hashLog capped at 28 bits (24 + 4). The assert linked above can currently be triggered by ./zstd -o /dev/null -7 < ~/silesia.tar --zstd=hlog=29

These bugs can't lead to data corruption, but they do have some bad effects:

  • Can regress compression ratio by corrupting the hashtable
  • Every time the assert fails, that is undefined behavior (shift by larger than width of type)
  • Cause CI failures for debug builds

Code pointer:

if (cPar.hashLog > maxShortCacheHashLog) {

@embg embg self-assigned this Dec 8, 2022
@embg embg added the release-blocking Must be done by the release label Dec 8, 2022
@embg embg assigned terrelln and unassigned embg Jan 17, 2023
terrelln added a commit to terrelln/zstd that referenced this issue Jan 20, 2023
* Cap shortCache chainLog to 24
* Cap row match finder hashLog so that rowLog <= 24
* Add unit tests to expose all cases. The row match finder unit tests
  are only run in 64-bit mode, because they allocate ~1GB.

Fixes facebook#3336
terrelln added a commit to terrelln/zstd that referenced this issue Jan 20, 2023
* Cap shortCache chainLog to 24
* Cap row match finder hashLog so that rowLog <= 24
* Add unit tests to expose all cases. The row match finder unit tests
  are only run in 64-bit mode, because they allocate ~1GB.

Fixes facebook#3336
terrelln added a commit to terrelln/zstd that referenced this issue Jan 20, 2023
* Cap shortCache chainLog to 24
* Cap row match finder hashLog so that rowLog <= 24
* Add unit tests to expose all cases. The row match finder unit tests
  are only run in 64-bit mode, because they allocate ~1GB.

Fixes facebook#3336
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocking Must be done by the release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants