From b1ec94e63cfb4ba75a7268d15ec706ed0dcb8ce2 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Mon, 21 Oct 2019 19:42:14 -0700 Subject: [PATCH] Fix ZSTD_f_zstd1_magicless for small data * Fix `ZSTD_FRAMEHEADERSIZE_PREFIX` and `ZSTD_FRAMEHEADERSIZE_MIN` to take a `format` parameter, so it is impossible to get the wrong size. * Fix the places that called `ZSTD_FRAMEHEADERSIZE_PREFIX` without taking the format into account, which is now impossible by design. * Call `ZSTD_frameHeaderSize_internal()` with `dctx->format`. * The added tests catch both bugs in `ZSTD_decompressFrame()`. Fixes #1813. --- lib/decompress/zstd_decompress.c | 26 ++++++++++++-------------- lib/zstd.h | 4 ++-- programs/fileio.c | 2 +- tests/fullbench.c | 6 +++--- tests/fuzzer.c | 30 ++++++++++++++++++++++++++++++ zlibWrapper/zstd_zlibwrapper.c | 4 ++-- 6 files changed, 50 insertions(+), 22 deletions(-) diff --git a/lib/decompress/zstd_decompress.c b/lib/decompress/zstd_decompress.c index 751060b2c..ca47a6678 100644 --- a/lib/decompress/zstd_decompress.c +++ b/lib/decompress/zstd_decompress.c @@ -88,10 +88,7 @@ size_t ZSTD_estimateDCtxSize(void) { return sizeof(ZSTD_DCtx); } static size_t ZSTD_startingInputLength(ZSTD_format_e format) { - size_t const startingInputLength = (format==ZSTD_f_zstd1_magicless) ? - ZSTD_FRAMEHEADERSIZE_PREFIX - ZSTD_FRAMEIDSIZE : - ZSTD_FRAMEHEADERSIZE_PREFIX; - ZSTD_STATIC_ASSERT(ZSTD_FRAMEHEADERSIZE_PREFIX >= ZSTD_FRAMEIDSIZE); + size_t const startingInputLength = ZSTD_FRAMEHEADERSIZE_PREFIX(format); /* only supports formats ZSTD_f_zstd1 and ZSTD_f_zstd1_magicless */ assert( (format == ZSTD_f_zstd1) || (format == ZSTD_f_zstd1_magicless) ); return startingInputLength; @@ -376,7 +373,7 @@ unsigned long long ZSTD_findDecompressedSize(const void* src, size_t srcSize) { unsigned long long totalDstSize = 0; - while (srcSize >= ZSTD_FRAMEHEADERSIZE_PREFIX) { + while (srcSize >= ZSTD_startingInputLength(ZSTD_f_zstd1)) { U32 const magicNumber = MEM_readLE32(src); if ((magicNumber & ZSTD_MAGIC_SKIPPABLE_MASK) == ZSTD_MAGIC_SKIPPABLE_START) { @@ -629,11 +626,12 @@ static size_t ZSTD_decompressFrame(ZSTD_DCtx* dctx, /* check */ RETURN_ERROR_IF( - remainingSrcSize < ZSTD_FRAMEHEADERSIZE_MIN+ZSTD_blockHeaderSize, + remainingSrcSize < ZSTD_FRAMEHEADERSIZE_MIN(dctx->format)+ZSTD_blockHeaderSize, srcSize_wrong); /* Frame Header */ - { size_t const frameHeaderSize = ZSTD_frameHeaderSize(ip, ZSTD_FRAMEHEADERSIZE_PREFIX); + { size_t const frameHeaderSize = ZSTD_frameHeaderSize_internal( + ip, ZSTD_FRAMEHEADERSIZE_PREFIX(dctx->format), dctx->format); if (ZSTD_isError(frameHeaderSize)) return frameHeaderSize; RETURN_ERROR_IF(remainingSrcSize < frameHeaderSize+ZSTD_blockHeaderSize, srcSize_wrong); @@ -714,7 +712,7 @@ static size_t ZSTD_decompressMultiFrame(ZSTD_DCtx* dctx, dictSize = ZSTD_DDict_dictSize(ddict); } - while (srcSize >= ZSTD_FRAMEHEADERSIZE_PREFIX) { + while (srcSize >= ZSTD_startingInputLength(dctx->format)) { #if defined(ZSTD_LEGACY_SUPPORT) && (ZSTD_LEGACY_SUPPORT >= 1) if (ZSTD_isLegacy(src, srcSize)) { @@ -1300,14 +1298,14 @@ size_t ZSTD_DCtx_refPrefix(ZSTD_DCtx* dctx, const void* prefix, size_t prefixSiz /* ZSTD_initDStream_usingDict() : - * return : expected size, aka ZSTD_FRAMEHEADERSIZE_PREFIX. + * return : expected size, aka ZSTD_startingInputLength(). * this function cannot fail */ size_t ZSTD_initDStream_usingDict(ZSTD_DStream* zds, const void* dict, size_t dictSize) { DEBUGLOG(4, "ZSTD_initDStream_usingDict"); FORWARD_IF_ERROR( ZSTD_DCtx_reset(zds, ZSTD_reset_session_only) ); FORWARD_IF_ERROR( ZSTD_DCtx_loadDictionary(zds, dict, dictSize) ); - return ZSTD_FRAMEHEADERSIZE_PREFIX; + return ZSTD_startingInputLength(zds->format); } /* note : this variant can't fail */ @@ -1324,16 +1322,16 @@ size_t ZSTD_initDStream_usingDDict(ZSTD_DStream* dctx, const ZSTD_DDict* ddict) { FORWARD_IF_ERROR( ZSTD_DCtx_reset(dctx, ZSTD_reset_session_only) ); FORWARD_IF_ERROR( ZSTD_DCtx_refDDict(dctx, ddict) ); - return ZSTD_FRAMEHEADERSIZE_PREFIX; + return ZSTD_startingInputLength(dctx->format); } /* ZSTD_resetDStream() : - * return : expected size, aka ZSTD_FRAMEHEADERSIZE_PREFIX. + * return : expected size, aka ZSTD_startingInputLength(). * this function cannot fail */ size_t ZSTD_resetDStream(ZSTD_DStream* dctx) { FORWARD_IF_ERROR(ZSTD_DCtx_reset(dctx, ZSTD_reset_session_only)); - return ZSTD_FRAMEHEADERSIZE_PREFIX; + return ZSTD_startingInputLength(dctx->format); } @@ -1564,7 +1562,7 @@ size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* output, ZSTD_inB zds->lhSize += remainingInput; } input->pos = input->size; - return (MAX(ZSTD_FRAMEHEADERSIZE_MIN, hSize) - zds->lhSize) + ZSTD_blockHeaderSize; /* remaining header bytes + next block header */ + return (MAX((size_t)ZSTD_FRAMEHEADERSIZE_MIN(zds->format), hSize) - zds->lhSize) + ZSTD_blockHeaderSize; /* remaining header bytes + next block header */ } assert(ip != NULL); memcpy(zds->headerBuffer + zds->lhSize, ip, toLoad); zds->lhSize = hSize; ip += toLoad; diff --git a/lib/zstd.h b/lib/zstd.h index c57f2debc..22711d17b 100644 --- a/lib/zstd.h +++ b/lib/zstd.h @@ -1017,8 +1017,8 @@ ZSTDLIB_API size_t ZSTD_sizeof_DDict(const ZSTD_DDict* ddict); * Some of them might be removed in the future (especially when redundant with existing stable functions) * ***************************************************************************************/ -#define ZSTD_FRAMEHEADERSIZE_PREFIX 5 /* minimum input size required to query frame header size */ -#define ZSTD_FRAMEHEADERSIZE_MIN 6 +#define ZSTD_FRAMEHEADERSIZE_PREFIX(format) ((format) == ZSTD_f_zstd1 ? 5 : 1) /* minimum input size required to query frame header size */ +#define ZSTD_FRAMEHEADERSIZE_MIN(format) ((format) == ZSTD_f_zstd1 ? 6 : 2) #define ZSTD_FRAMEHEADERSIZE_MAX 18 /* can be useful for static allocation */ #define ZSTD_SKIPPABLEHEADERSIZE 8 diff --git a/programs/fileio.c b/programs/fileio.c index f4384484c..16d06287d 100644 --- a/programs/fileio.c +++ b/programs/fileio.c @@ -2443,7 +2443,7 @@ FIO_analyzeFrames(fileInfo_t* info, FILE* const srcFile) for ( ; ; ) { BYTE headerBuffer[ZSTD_FRAMEHEADERSIZE_MAX]; size_t const numBytesRead = fread(headerBuffer, 1, sizeof(headerBuffer), srcFile); - if (numBytesRead < ZSTD_FRAMEHEADERSIZE_MIN) { + if (numBytesRead < ZSTD_FRAMEHEADERSIZE_MIN(ZSTD_f_zstd1)) { if ( feof(srcFile) && (numBytesRead == 0) && (info->compressedSize > 0) diff --git a/tests/fullbench.c b/tests/fullbench.c index f750ee0d7..0e2761e11 100644 --- a/tests/fullbench.c +++ b/tests/fullbench.c @@ -450,7 +450,7 @@ static int benchMem(unsigned benchNb, case 31: /* ZSTD_decodeLiteralsBlock : starts literals block in dstBuff2 */ { size_t frameHeaderSize; g_cSize = ZSTD_compress(dstBuff, dstBuffSize, src, srcSize, cLevel); - frameHeaderSize = ZSTD_frameHeaderSize(dstBuff, ZSTD_FRAMEHEADERSIZE_PREFIX); + frameHeaderSize = ZSTD_frameHeaderSize(dstBuff, ZSTD_FRAMEHEADERSIZE_PREFIX(ZSTD_f_zstd1)); CONTROL(!ZSTD_isError(frameHeaderSize)); /* check block is compressible, hence contains a literals section */ { blockProperties_t bp; @@ -471,10 +471,10 @@ static int benchMem(unsigned benchNb, const BYTE* ip = dstBuff; const BYTE* iend; { size_t const cSize = ZSTD_compress(dstBuff, dstBuffSize, src, srcSize, cLevel); - CONTROL(cSize > ZSTD_FRAMEHEADERSIZE_PREFIX); + CONTROL(cSize > ZSTD_FRAMEHEADERSIZE_PREFIX(ZSTD_f_zstd1)); } /* Skip frame Header */ - { size_t const frameHeaderSize = ZSTD_frameHeaderSize(dstBuff, ZSTD_FRAMEHEADERSIZE_PREFIX); + { size_t const frameHeaderSize = ZSTD_frameHeaderSize(dstBuff, ZSTD_FRAMEHEADERSIZE_PREFIX(ZSTD_f_zstd1)); CONTROL(!ZSTD_isError(frameHeaderSize)); ip += frameHeaderSize; } diff --git a/tests/fuzzer.c b/tests/fuzzer.c index a109a440d..88f3b83f8 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -1886,6 +1886,36 @@ static int basicUnitTests(U32 const seed, double compressibility) DISPLAYLEVEL(3, "streaming OK : regenerated %u bytes \n", (unsigned)out.pos); } + /* basic block compression */ + DISPLAYLEVEL(3, "test%3i : empty magic-less format test : ", testNb++); + CHECK( ZSTD_CCtx_setParameter(cctx, ZSTD_c_format, ZSTD_f_zstd1_magicless) ); + { ZSTD_inBuffer in = { CNBuffer, 0, 0 }; + ZSTD_outBuffer out = { compressedBuffer, ZSTD_compressBound(0), 0 }; + size_t const result = ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end); + if (result != 0) goto _output_error; + if (in.pos != in.size) goto _output_error; + cSize = out.pos; + } + DISPLAYLEVEL(3, "OK (compress : %u -> %u bytes)\n", (unsigned)0, (unsigned)cSize); + + DISPLAYLEVEL(3, "test%3i : decompress of empty magic-less frame : ", testNb++); + ZSTD_DCtx_reset(dctx, ZSTD_reset_session_and_parameters); + CHECK( ZSTD_DCtx_setParameter(dctx, ZSTD_d_format, ZSTD_f_zstd1_magicless) ); + /* one shot */ + { size_t const result = ZSTD_decompressDCtx(dctx, decodedBuffer, CNBuffSize, compressedBuffer, cSize); + if (result != 0) goto _output_error; + DISPLAYLEVEL(3, "one-shot OK, "); + } + /* streaming */ + { ZSTD_inBuffer in = { compressedBuffer, cSize, 0 }; + ZSTD_outBuffer out = { decodedBuffer, CNBuffSize, 0 }; + size_t const result = ZSTD_decompressStream(dctx, &out, &in); + if (result != 0) goto _output_error; + if (in.pos != in.size) goto _output_error; + if (out.pos != 0) goto _output_error; + DISPLAYLEVEL(3, "streaming OK : regenerated %u bytes \n", (unsigned)out.pos); + } + ZSTD_freeCCtx(cctx); ZSTD_freeDCtx(dctx); } diff --git a/zlibWrapper/zstd_zlibwrapper.c b/zlibWrapper/zstd_zlibwrapper.c index cb6afa786..3fa442106 100644 --- a/zlibWrapper/zstd_zlibwrapper.c +++ b/zlibWrapper/zstd_zlibwrapper.c @@ -31,7 +31,7 @@ /* === Constants === */ #define Z_INFLATE_SYNC 8 #define ZLIB_HEADERSIZE 4 -#define ZSTD_HEADERSIZE ZSTD_FRAMEHEADERSIZE_MIN +#define ZSTD_HEADERSIZE ZSTD_FRAMEHEADERSIZE_MIN(ZSTD_f_zstd1) #define ZWRAP_DEFAULT_CLEVEL 3 /* Z_DEFAULT_COMPRESSION is translated to ZWRAP_DEFAULT_CLEVEL for zstd */ @@ -457,7 +457,7 @@ static void ZWRAP_initDCtx(ZWRAP_DCtx* zwd) static ZWRAP_DCtx* ZWRAP_createDCtx(z_streamp strm) { ZWRAP_DCtx* zwd; - MEM_STATIC_ASSERT(sizeof(zwd->headerBuf) >= ZSTD_FRAMEHEADERSIZE_MIN); /* check static buffer size condition */ + MEM_STATIC_ASSERT(sizeof(zwd->headerBuf) >= ZSTD_HEADERSIZE); /* check static buffer size condition */ if (strm->zalloc && strm->zfree) { zwd = (ZWRAP_DCtx*)strm->zalloc(strm->opaque, 1, sizeof(ZWRAP_DCtx));