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.

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

Jump to bottom

[fuzz] Superblock fuzz issues #1891

Merged
merged 15 commits into from
Dec 10, 2019
Merged

[fuzz] Superblock fuzz issues #1891

merged 15 commits into from
Dec 10, 2019

Conversation

bimbashrestha
Copy link
Contributor

@bimbashrestha bimbashrestha commented Nov 16, 2019 •
edited
Loading

Fixing 2 fuzzer issues

  • makes sure enough room is present for checksum in superblock
  • output a regular uncompressed block if compressSequences fails on dstSize_tooSmall error

@Cyan4973
Copy link
Contributor

Great fix @bimbashrestha !

Could you also add a unit test able to catch this bug please ?

@bimbashrestha bimbashrestha changed the title [fuzz] Leaving 4 bytes of room for the checksum in superblock [fuzz] Superblock fuzz issues Nov 18, 2019
terrelln
terrelln requested changes Nov 18, 2019
View reviewed changes
lib/compress/zstd_compress.c Outdated Show resolved Hide resolved
lib/compress/zstd_compress.c Outdated Show resolved Hide resolved
terrelln
terrelln requested changes Nov 18, 2019
View reviewed changes
lib/compress/zstd_compress.c Show resolved Hide resolved
lib/compress/zstd_compress.c Outdated Show resolved Hide resolved
@bimbashrestha
Copy link
Contributor Author

I can't seem to figure out why the dictionary_round_trip is now failing. Here is the end of the debug trace. Any thoughts?

../../lib/decompress/zstd_decompress_block.c: ZSTD_decompressSequences_body ../../lib/decompress/zstd_decompress_block.c: ZSTD_decompressSequences_body: after decode loop, remaining nbSeq : 0 ../../lib/decompress/zstd_decompress.c: ZSTD_copyRawBlock ../../lib/decompress/zstd_decompress_block.c: ZSTD_decompressBlock_internal (size : 62) ../../lib/decompress/zstd_decompress_block.c: ZSTD_decodeLiteralsBlock ../../lib/decompress/zstd_decompress_block.c: ZSTD_decodeLiteralsBlock : 11 ../../lib/decompress/zstd_decompress_block.c: ZSTD_decodeSeqHeaders ../../lib/decompress/zstd_decompress_block.c: ZSTD_decompressSequences ../../lib/decompress/zstd_decompress_block.c: ZSTD_decompressSequences_body ../../lib/decompress/zstd_decompress_block.c:675: ERROR!: check oMatchEnd > oend failed, returning ERROR(dstSize_tooSmall): last match must fit within dstBuffer dictionary_round_trip.c: 97: Assertion: !ZSTD_isError(result) failed. Destination buffer is too small Aborted (core dumped)

@terrelln
Copy link
Contributor

I would check and see what new code is being triggered that causes the problem. Is (dstCapacity - cSize) < 4? Are we emitting an uncompressed block?

@bimbashrestha
Copy link
Contributor Author

Ah I think I found it. We can't rely on cSize == 0 to discriminate noCompress blocks when we confirm rep codes. Checking explicitly now.

bimbashrestha
bimbashrestha commented Nov 23, 2019
View reviewed changes
tests/fuzzer.c
assert(!ZSTD_isError(ZSTD_compress2(cctx, compressedBuffer, 1339, CNBuffer, 1278)));
ZSTD_freeCCtx(cctx);
}
DISPLAYLEVEL(3, "OK \n");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked that this fails before this patch.

bimbashrestha
bimbashrestha commented Nov 27, 2019
View reviewed changes
lib/compress/zstd_compress.c Outdated
@@ -2513,7 +2515,7 @@ static size_t ZSTD_compressBlock_targetCBlockSize(ZSTD_CCtx* zc,
}
}

if (!ZSTD_isError(cSize) && !usingNoCompressSuperBlock) {
if (!ZSTD_isError(cSize) && compressSuperBlock) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

However, when (!usingNoCompressSuperBlock), there is still a possibility that a compress block was sent. It's just no longer divided into sub-blocks.

I think this was a mistake. Now we should only enter this branch if the block sent is compressed. And this should take care of the rep codes too I believe.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Dec 3, 2019

Let's discuss that again tomorrow.

@Cyan4973
Copy link
Contributor

Cyan4973 commented Dec 4, 2019

Good ! This is getting pretty close !

Only a few minor details remaining.

@Cyan4973 Cyan4973 merged commit d73e2fb into facebook:dev Dec 10, 2019
@terrelln
Copy link
Contributor

Looks like there are some new OSS-Fuzz issues with this patch.

@terrelln
Copy link
Contributor

It may help to run the fuzzers locally for ~1 hour once you have the fix, in case there are more bugs. You can follow the instructions in tests/fuzz/README.md.

@terrelln
Copy link
Contributor

It could be a case of this bug blocking oss-fuzz from making progress and now it can find a new one.

@bimbashrestha bimbashrestha deleted the oss branch March 17, 2020 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@terrelln terrelln terrelln requested changes

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bimbashrestha @Cyan4973 @terrelln @facebook-github-bot