Fuzz on maxBlockSize #3418

daniellerozenblit · 2023-01-11T18:46:37Z

This PR extends our fuzz testing in response to a recently found bug in our repcode update logic (#3127). This bug occurred in scenarios where both offset_1 and offset_2 were invalid going into a block, and no matches were found. This bug was latent and luckily could not cause corruption, but we want to ensure that our fuzzer tests scenarios where both repcodes are invalidated to protect against these sorts of edge cases.

In order to test these scenarios, we want to generate cases with several blocks where block-size < window-size. It is currently not possible to do this in our non-streaming tests without generating large inputs. Testing with random flushing in streaming mode (which we currently do) should generate these scenarios, but we want to extend this to our non-streaming tests.

This PR includes a new cctx parameter, maxBlockSize which can be used to override the default ZSTD_BLOCKSIZE_MAX (128KB). This parameter is currently only meant to be used for testing purposes and is bounded by [1KB, ZSTD_BLOCKSIZE_MAX].

…k size

lib/compress/zstd_compress.c

Cyan4973 · 2023-01-12T00:29:14Z

Can you imagine one (or several) test which would be able to check that maxBlockSize works as intended ?

Cyan4973 · 2023-01-12T17:27:54Z

tests/zstreamtest.c

+        ZSTD_freeCCtx(cctx);
+        free(checkBuf);
+    }
+    DISPLAYLEVEL(3, "OK \n");


👍 Great !

One more test I would like to see is what happens when maxBlockSize > windowSize.
In such case, we want to be sure that the code will not produce blocks larger than windowSize.
I think your code is fine and will respect this condition.
The purpose of the test is to ensure it remains this way, a future modification could break this condition.

terrelln · 2023-01-12T20:28:16Z

lib/compress/zstd_compress.c

@@ -964,6 +982,11 @@ size_t ZSTD_CCtxParams_setParameter(ZSTD_CCtx_params* CCtxParams,
        CCtxParams->enableMatchFinderFallback = value;
        return CCtxParams->enableMatchFinderFallback;

+    case ZSTD_c_maxBlockSize:
+        BOUNDCHECK(ZSTD_c_maxBlockSize, value);


What happens if you set the value to 0?

I think we can leave the bounds as-is, but also allow setting to zero to use the default maxBlockSize. And we should also call that out in the docs.

terrelln · 2023-01-12T20:33:55Z

lib/zstd.h

+ *
+ *  Default is ZSTD_BLOCKSIZE_MAX.
+ *


Please add a little more detail here. Say what this parameter controls, what the min/max are (and why), and explain what setting to 0 means.

… set to 0 (goes to default)

terrelln

Just one minor nit in the zstd.h header, otherwise looks good!

terrelln · 2023-01-12T23:28:45Z

lib/zstd.h

@@ -141,6 +141,7 @@ ZSTDLIB_API const char* ZSTD_versionString(void);

 #define ZSTD_BLOCKSIZELOG_MAX  17
 #define ZSTD_BLOCKSIZE_MAX     (1<<ZSTD_BLOCKSIZELOG_MAX)
+#define ZSTD_BLOCKSIZE_MAX_MIN (1 << 10) /* The minimum valid max blocksize. Maximum blocksizes smaller than this make compressBound() inaccurate. */


Please move this down below into the static linking only section

Putting this constant here means that it will always be present, and we don't like to do that until it has baked in the unstable section.

terrelln

Noticed some bugs we'll have to fix. They're not your fault, our CCtxParams have grown significantly, and now a lot of them need to be "resolved" from some default value. And this resolution has to happen in many different places. We need to refactor the cctx params, so we can centralize all the logic for resolving & copying parameters.

Can you add a test case for the estimation functions that sets the maxBlockSize to 0? I think that we don't actually have any tests for these functions, because they should be failing right now.

You can add them to the test cases for the other static cctx functions here: https://github.com/facebook/zstd/blob/dev/tests/fuzzer.c#L1646

Basically you'd:

Create some ZSTD_CCtxParams, using the default values
Call ZSTD_estimateCCtxSize_usingCCtxParams()
Pass a buffer of that size to ZSTD_initStaticCCtx()
Do a compression & decompress the data to make sure it round trips
Do the same thing but with ZSTD_estimateCStreamSize_usingCCtxParams()
Do the same tests, but with an explicitly set maxBlockSize

You should write the test case before making the fix, so you can make sure that the test case exposes the bugs.

terrelln · 2023-01-13T19:24:58Z

lib/compress/zstd_compress.c

@@ -1944,7 +1972,7 @@ static size_t ZSTD_resetCCtx_internal(ZSTD_CCtx* zc,
    }

    {   size_t const windowSize = MAX(1, (size_t)MIN(((U64)1 << params->cParams.windowLog), pledgedSrcSize));
-        size_t const blockSize = MIN(ZSTD_BLOCKSIZE_MAX, windowSize);
+        size_t const blockSize = MIN(params->maxBlockSize, windowSize);


Can you add an assert that params->maxBlockSize != 0 up by the group of asserts on lines 1964-1966?

terrelln · 2023-01-13T19:28:16Z

lib/compress/zstd_compress.c

@@ -1651,7 +1679,7 @@ size_t ZSTD_estimateCStreamSize_usingCCtxParams(const ZSTD_CCtx_params* params)
    RETURN_ERROR_IF(params->nbWorkers > 0, GENERIC, "Estimate CCtx size is supported for single-threaded compression only.");
    {   ZSTD_compressionParameters const cParams =
                ZSTD_getCParamsFromCCtxParams(params, ZSTD_CONTENTSIZE_UNKNOWN, 0, ZSTD_cpm_noAttachDict);
-        size_t const blockSize = MIN(ZSTD_BLOCKSIZE_MAX, (size_t)1 << cParams.windowLog);


I think params->maxBlockSize could be equal to zero here, you have to resolve it.

terrelln · 2023-01-13T19:29:41Z

lib/compress/zstd_compress.c

 {
    size_t const windowSize = (size_t) BOUNDED(1ULL, 1ULL << cParams->windowLog, pledgedSrcSize);
-    size_t const blockSize = MIN(ZSTD_BLOCKSIZE_MAX, windowSize);
+    size_t const blockSize = MIN(maxBlockSize, windowSize);


I think maxBlockSize could be equal to zero here, you have to resolve it.

daniellerozenblit · 2023-01-17T20:14:34Z

I've added some tests for ZSTD_estimateCCtxSize_usingCCtxParams() and ZSTD_estimateCStreamSize_usingCCtxParams() and have confirmed that they fail before resolving maxBlockSize in the respective methods.

Cyan4973 · 2023-01-17T21:14:45Z

tests/zstreamtest.c

+            CHECK_Z(ZSTD_decompress(checkBuf, srcSize, dst1, size1));
+            CHECK(memcmp(src, checkBuf, srcSize) != 0, "Corruption!");
+
+            /* maxBlockSize = 3KB, windowLog = 10 */


terrelln

Awesome!

daniellerozenblit and others added 3 commits January 4, 2023 13:01

initial commit

908e812

Merge branch 'dev' into fuzz-max-block-size

d913417

resolve max block value in cctx and use when calculating the max bloc…

fe08137

…k size

daniellerozenblit requested a review from embg January 11, 2023 18:46

facebook-github-bot added the CLA Signed label Jan 11, 2023

update minimum threshold for max block size

1fffcfe

daniellerozenblit force-pushed the fuzz-max-block-size branch from 887298f to 1fffcfe Compare January 11, 2023 19:10

daniellerozenblit marked this pull request as ready for review January 11, 2023 20:07

daniellerozenblit requested a review from Cyan4973 January 11, 2023 20:16

Cyan4973 reviewed Jan 12, 2023

View reviewed changes

lib/compress/zstd_compress.c Show resolved Hide resolved

add simple test for maxBlockSize expected functionality

53eb5a7

daniellerozenblit force-pushed the fuzz-max-block-size branch from 523e0a7 to 53eb5a7 Compare January 12, 2023 16:55

Cyan4973 reviewed Jan 12, 2023

View reviewed changes

terrelln reviewed Jan 12, 2023

View reviewed changes

additional tests and documentation updates + allow maxBlockSize to be…

06b096d

… set to 0 (goes to default)

terrelln reviewed Jan 12, 2023

View reviewed changes

move ZSTD_BLOCKSIZE_MAX_MIN to static linking only section

14b8def

terrelln requested changes Jan 13, 2023

View reviewed changes

fix maxBlockSize resolution + add test cases

8353a4b

daniellerozenblit force-pushed the fuzz-max-block-size branch from dc01fad to 8353a4b Compare January 17, 2023 20:24

Cyan4973 reviewed Jan 17, 2023

View reviewed changes

terrelln approved these changes Jan 19, 2023

View reviewed changes

daniellerozenblit merged commit dc1c6cc into facebook:dev Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fuzz on maxBlockSize #3418

Fuzz on maxBlockSize #3418

daniellerozenblit commented Jan 11, 2023

Cyan4973 commented Jan 12, 2023

Cyan4973 Jan 12, 2023

terrelln Jan 12, 2023

terrelln Jan 12, 2023

terrelln left a comment

terrelln Jan 12, 2023

terrelln left a comment

terrelln Jan 13, 2023

terrelln Jan 13, 2023

terrelln Jan 13, 2023

daniellerozenblit commented Jan 17, 2023

Cyan4973 Jan 17, 2023

terrelln left a comment

Fuzz on maxBlockSize #3418

Fuzz on maxBlockSize #3418

Conversation

daniellerozenblit commented Jan 11, 2023

Cyan4973 commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrelln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrelln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniellerozenblit commented Jan 17, 2023

Choose a reason for hiding this comment

terrelln left a comment

Choose a reason for hiding this comment