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

Fix super block compression and stream raw blocks in decompression #1947

Merged
merged 1 commit into from Jan 11, 2020

Conversation

terrelln
Copy link
Contributor

@terrelln terrelln commented Jan 10, 2020

Super blocks must never violate the zstd block bound of input_size + ZSTD_blockHeaderSize. The individual sub-blocks may, but not the super block. If the superblock violates the block bound we are liable to violate ZSTD_compressBound(), which we must not do. Whenever the super block violates the block bound we instead emit an uncompressed block.

This means we increase the latency because of the single uncompressed block. I fix this by enabling streaming an uncompressed block, so the latency of an uncompressed block is 1 byte. This doesn't reduce the latency of the buffer-less API, but I don't think we really care.

  • I added a test case that verifies that the decompression has 1 byte latency.
  • I rely on existing zstreamtest / fuzzer / libfuzzer regression tests for correctness. During development I had several correctness bugs, and they easily caught them.
  • The added assert that the superblock doesn't violate the block bound will help us discover any missed conditions (though I think I got them all).

Credit to OSS-Fuzz.

@terrelln terrelln force-pushed the superblock-fix branch 4 times, most recently from 24a1394 to 7d0bd17 Compare January 10, 2020 19:52
@terrelln terrelln changed the title [WIP] SuperBlock fix [lib] Fix super block compression and stream raw blocks in decompression Jan 10, 2020
@terrelln terrelln changed the title [lib] Fix super block compression and stream raw blocks in decompression Fix super block compression and stream raw blocks in decompression Jan 10, 2020
@@ -944,22 +962,34 @@ size_t ZSTD_decompressContinue(ZSTD_DCtx* dctx, void* dst, size_t dstCapacity, c
case bt_compressed:
DEBUGLOG(5, "ZSTD_decompressContinue: case bt_compressed");
rSize = ZSTD_decompressBlock_internal(dctx, dst, dstCapacity, src, srcSize, /* frame */ 1);
dctx->expected = 0; /* Streaming not supported */
Copy link
Contributor

Choose a reason for hiding this comment

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

Note : it changes the meaning and role of dctx->expected.

Instead of being the amount of data of next block,
that a user must provide for decompression to happen,
it is now an indication of what it should provide, though less may also be authorized.
More importantly, this variable becomes a stage validator (it must be ==0 to move to next stage).

Nothing wrong with it, just, it's a fairly consequential change of meaning,
on a long-lived variable which is pretty close to global state,
so it deserves careful attention,
since there is a potential that it could mess with other parts of the code base where it's read or written.

@terrelln
Copy link
Contributor Author

Fix the travis failure due to a missing ZSTD_DCtx_reset() call

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.

None yet

3 participants