From 31769ce7029fee2409ad52155d143b89aed5b692 Mon Sep 17 00:00:00 2001 From: Yann Collet Date: Fri, 22 Jun 2018 17:58:21 -0700 Subject: [PATCH] error on no forward progress streaming decoders, such as ZSTD_decompressStream() or ZSTD_decompress_generic(), may end up making no forward progress, (aka no byte read from input __and__ no byte written to output), due to unusual parameters conditions, such as providing an output buffer already full. In such case, the caller may be caught in an infinite loop, calling the streaming decompression function again and again, without making any progress. This version detects such situation, and generates an error instead : ZSTD_error_dstSize_tooSmall when output buffer is full, ZSTD_error_srcSize_wrong when input buffer is empty. The detection tolerates a number of attempts before triggering an error, controlled by ZSTD_NO_FORWARD_PROGRESS_MAX macro constant, which is set to 16 by default, and can be re-defined at compilation time. This behavior tolerates potentially existing implementations where such cases happen sporadically, like once or twice, which is not dangerous (only infinite loops are), without generating an error, hence without breaking these implementations. --- lib/decompress/zstd_decompress.c | 28 ++++++++++++++++++++++++-- tests/fuzzer.c | 4 ++++ tests/zstreamtest.c | 34 +++++++++++++++++++++++++++++--- 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/lib/decompress/zstd_decompress.c b/lib/decompress/zstd_decompress.c index 4de98a8e6..8f4589d13 100644 --- a/lib/decompress/zstd_decompress.c +++ b/lib/decompress/zstd_decompress.c @@ -41,6 +41,17 @@ #endif +/*! + * NO_FORWARD_PROGRESS_MAX : + * maximum allowed nb of calls to ZSTD_decompressStream() and ZSTD_decompress_generic() + * without any forward progress + * (defined as: no byte read from input, and no byte flushed to output) + * before triggering an error. + */ +#ifndef ZSTD_NO_FORWARD_PROGRESS_MAX +# define ZSTD_NO_FORWARD_PROGRESS_MAX 16 +#endif + /*-******************************************************* * Dependencies *********************************************************/ @@ -153,6 +164,7 @@ struct ZSTD_DCtx_s U32 previousLegacyVersion; U32 legacyVersion; U32 hostageByte; + int noForwardProgress; /* workspace */ BYTE litBuffer[ZSTD_BLOCKSIZE_MAX + WILDCOPY_OVERLENGTH]; @@ -194,6 +206,7 @@ static void ZSTD_initDCtx_internal(ZSTD_DCtx* dctx) dctx->streamStage = zdss_init; dctx->legacyContext = NULL; dctx->previousLegacyVersion = 0; + dctx->noForwardProgress = 0; dctx->bmi2 = ZSTD_cpuid_bmi2(ZSTD_cpuid()); } @@ -2620,6 +2633,7 @@ size_t ZSTD_initDStream_usingDict(ZSTD_DStream* zds, const void* dict, size_t di { DEBUGLOG(4, "ZSTD_initDStream_usingDict"); zds->streamStage = zdss_init; + zds->noForwardProgress = 0; CHECK_F( ZSTD_DCtx_loadDictionary(zds, dict, dictSize) ); return ZSTD_frameHeaderSize_prefix; } @@ -2960,8 +2974,18 @@ size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* output, ZSTD_inB } } /* result */ - input->pos += (size_t)(ip-istart); - output->pos += (size_t)(op-ostart); + input->pos = (size_t)(ip - (const char*)(input->src)); + output->pos = (size_t)(op - (char*)(output->dst)); + if ((ip==istart) && (op==ostart)) { /* no forward progress */ + zds->noForwardProgress ++; + if (zds->noForwardProgress >= ZSTD_NO_FORWARD_PROGRESS_MAX) { + if (op==oend) return ERROR(dstSize_tooSmall); + if (ip==iend) return ERROR(srcSize_wrong); + assert(0); + } + } else { + zds->noForwardProgress = 0; + } { size_t nextSrcSizeHint = ZSTD_nextSrcSizeToDecompress(zds); if (!nextSrcSizeHint) { /* frame fully decoded */ if (zds->outEnd == zds->outStart) { /* output fully flushed */ diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 9b4cd58d5..f7bacd5ca 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -66,6 +66,9 @@ static UTIL_time_t g_displayClock = UTIL_TIME_INITIALIZER; if (g_displayLevel>=4) fflush(stderr); } } +/*-******************************************************* +* Compile time test +*********************************************************/ #undef MIN #undef MAX void FUZ_bug976(void) @@ -74,6 +77,7 @@ void FUZ_bug976(void) assert(ZSTD_CHAINLOG_MAX < 31); } + /*-******************************************************* * Internal functions *********************************************************/ diff --git a/tests/zstreamtest.c b/tests/zstreamtest.c index c76622701..70da29728 100644 --- a/tests/zstreamtest.c +++ b/tests/zstreamtest.c @@ -433,11 +433,12 @@ static int basicUnitTests(U32 seed, double compressibility) inBuff.pos = 0; outBuff.pos = 0; while (r) { /* skippable frame */ - size_t const inSize = FUZ_rand(&coreSeed) & 15; - size_t const outSize = FUZ_rand(&coreSeed) & 15; + size_t const inSize = (FUZ_rand(&coreSeed) & 15) + 1; + size_t const outSize = (FUZ_rand(&coreSeed) & 15) + 1; inBuff.size = inBuff.pos + inSize; outBuff.size = outBuff.pos + outSize; r = ZSTD_decompressStream(zd, &outBuff, &inBuff); + if (ZSTD_isError(r)) DISPLAYLEVEL(4, "ZSTD_decompressStream on skippable frame error : %s \n", ZSTD_getErrorName(r)); if (ZSTD_isError(r)) goto _output_error; } /* normal frame */ @@ -445,14 +446,17 @@ static int basicUnitTests(U32 seed, double compressibility) r=1; while (r) { size_t const inSize = FUZ_rand(&coreSeed) & 15; - size_t const outSize = FUZ_rand(&coreSeed) & 15; + size_t const outSize = (FUZ_rand(&coreSeed) & 15) + (!inSize); /* avoid having both sizes at 0 => would trigger a no_forward_progress error */ inBuff.size = inBuff.pos + inSize; outBuff.size = outBuff.pos + outSize; r = ZSTD_decompressStream(zd, &outBuff, &inBuff); + if (ZSTD_isError(r)) DISPLAYLEVEL(4, "ZSTD_decompressStream error : %s \n", ZSTD_getErrorName(r)); if (ZSTD_isError(r)) goto _output_error; } } + if (outBuff.pos != CNBufferSize) DISPLAYLEVEL(4, "outBuff.pos != CNBufferSize : should have regenerated same amount ! \n"); if (outBuff.pos != CNBufferSize) goto _output_error; /* should regenerate the same amount */ + if (inBuff.pos != cSize) DISPLAYLEVEL(4, "inBuff.pos != cSize : should have real all input ! \n"); if (inBuff.pos != cSize) goto _output_error; /* should have read the entire frame */ DISPLAYLEVEL(3, "OK \n"); @@ -464,6 +468,30 @@ static int basicUnitTests(U32 seed, double compressibility) } } DISPLAYLEVEL(3, "OK \n"); + /* Decompression forward progress */ + DISPLAYLEVEL(3, "test%3i : generate error when ZSTD_decompressStream() doesn't progress : ", testNb++); + { /* skippable frame */ + size_t r = 0; + int decNb = 0; + int const maxDec = 100; + inBuff.src = compressedBuffer; + inBuff.size = cSize; + inBuff.pos = 0; + + outBuff.dst = decodedBuffer; + outBuff.pos = 0; + outBuff.size = CNBufferSize-1; /* 1 byte missing */ + + for (decNb=0; decNb