From e3cd2785e2caaf51d815d750a789215a850ddbda Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Fri, 13 Dec 2019 15:31:29 -0800 Subject: [PATCH 1/7] Add test to catch too many noCompress superblocks on streaming --- tests/fuzzer.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 56936e9f0..89bac525c 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -489,6 +489,53 @@ static int basicUnitTests(U32 const seed, double compressibility) } DISPLAYLEVEL(3, "OK \n"); + DISPLAYLEVEL(3, "test%3d: superblock uncompressable data, too many nocompress superblocks : ", testNb++) + { + ZSTD_CCtx* cctx = ZSTD_createCCtx(); + void* src = CNBuffer; void* dst = compressedBuffer; + size_t srcSize = 321656; size_t dstCapacity = ZSTD_compressBound(srcSize); + + /* This is the number of bytes to stream before ending. This value + * was obtained by trial and error :/. */ + + const size_t streamCompressThreshold = 161792; + const size_t streamCompressDelta = 1024; + + /* The first 1/3 of the buffer is compressable and the last 2/3 is + * uncompressable. This is an approximation of the type of data + * the fuzzer generated to catch this bug. Streams like this were making + * zstd generate noCompress superblocks (which are larger than the src + * they come from). Do this enough times, and we'll run out of room + * and throw a dstSize_tooSmall error. */ + + const size_t compressablePartSize = srcSize/3; + const size_t uncompressablePartSize = srcSize-compressablePartSize; + RDG_genBuffer(CNBuffer, compressablePartSize, 0.5, 0.5, seed); + RDG_genBuffer(CNBuffer+compressablePartSize, uncompressablePartSize, 0, 0, seed); + + /* Setting target block size so that superblock is used */ + + ZSTD_CCtx_setParameter(cctx, ZSTD_c_targetCBlockSize, 81); + + { size_t read; + for (read = 0; read < streamCompressThreshold; read += streamCompressDelta) { + ZSTD_inBuffer in = {src, streamCompressDelta, 0}; + ZSTD_outBuffer out = {dst, dstCapacity, 0}; + assert(!ZSTD_isError(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_continue))); + assert(!ZSTD_isError(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end))); + src += streamCompressDelta; srcSize -= streamCompressDelta; + dst += out.pos; dstCapacity -= out.pos;}} + + /* This is trying to catch a dstSize_tooSmall error */ + + { ZSTD_inBuffer in = {src, srcSize, 0}; + ZSTD_outBuffer out = {dst, dstCapacity, 0}; + assert(!ZSTD_isError(ZSTD_compressStream2(cctx, &out, &in, ZSTD_e_end)));} + ZSTD_freeCCtx(cctx); + } + DISPLAYLEVEL(3, "OK \n"); + + RDG_genBuffer(CNBuffer, CNBuffSize, compressibility, 0. /*auto*/, seed); DISPLAYLEVEL(3, "test%3d: superblock enough room for checksum : ", testNb++) { /* This tests whether or not we leave enough room for the checksum at the end @@ -501,7 +548,7 @@ static int basicUnitTests(U32 const seed, double compressibility) ZSTD_freeCCtx(cctx); } DISPLAYLEVEL(3, "OK \n"); - + DISPLAYLEVEL(3, "test%3i : compress a NULL input with each level : ", testNb++); { int level = -1; ZSTD_CCtx* cctx = ZSTD_createCCtx(); From 5225dcfc0f8c03ea718417d58c81cd0c799abf22 Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Fri, 13 Dec 2019 15:47:28 -0800 Subject: [PATCH 2/7] Adding bool to check if enough room left for noCompress superblocks --- lib/compress/zstd_compress.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index b624e3c18..338f8dd56 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2562,6 +2562,14 @@ static size_t ZSTD_compress_frameChunk (ZSTD_CCtx* cctx, BYTE* const ostart = (BYTE*)dst; BYTE* op = ostart; U32 const maxDist = (U32)1 << cctx->appliedParams.cParams.windowLog; + + /* This bool is set if there is enough room to output all noCompress superblocks. + * Just checks if the number of compressed blocks we can fit in dstCapacity is + * greater than the optimistic number of blocks we still have remaining. + * This might be UNset when data is uncompressable and we're streaming. */ + + int enoughDstCapacityForNoCompressSuperBlocks = + (dstCapacity / (blockSize + 7 /* header + checksum */)) > (srcSize / blockSize); assert(cctx->appliedParams.cParams.windowLog <= ZSTD_WINDOWLOG_MAX); DEBUGLOG(5, "ZSTD_compress_frameChunk (blockSize=%u)", (unsigned)blockSize); @@ -2586,7 +2594,7 @@ static size_t ZSTD_compress_frameChunk (ZSTD_CCtx* cctx, { size_t cSize; int useTargetCBlockSize = ZSTD_useTargetCBlockSize(&cctx->appliedParams); - if (useTargetCBlockSize) { + if (useTargetCBlockSize && enoughDstCapacityForNoCompressSuperBlocks) { cSize = ZSTD_compressBlock_targetCBlockSize(cctx, op, dstCapacity, ip, blockSize, lastBlock); FORWARD_IF_ERROR(cSize); } else { From 49b2bf7106a9080b9dd121bea9359473fa5c1321 Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Fri, 13 Dec 2019 16:06:57 -0800 Subject: [PATCH 3/7] 'void* size issue' fix --- tests/fuzzer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 89bac525c..c39969a8d 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -511,7 +511,7 @@ static int basicUnitTests(U32 const seed, double compressibility) const size_t compressablePartSize = srcSize/3; const size_t uncompressablePartSize = srcSize-compressablePartSize; RDG_genBuffer(CNBuffer, compressablePartSize, 0.5, 0.5, seed); - RDG_genBuffer(CNBuffer+compressablePartSize, uncompressablePartSize, 0, 0, seed); + RDG_genBuffer((void*)((char*)CNBuffer+compressablePartSize), uncompressablePartSize, 0, 0, seed); /* Setting target block size so that superblock is used */ From db5124ef6e6d5ca97df957280930a3591021b1bb Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Fri, 13 Dec 2019 16:24:49 -0800 Subject: [PATCH 4/7] More void* issues. Just replacing with BYTE* --- tests/fuzzer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index c39969a8d..d413be040 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -492,7 +492,7 @@ static int basicUnitTests(U32 const seed, double compressibility) DISPLAYLEVEL(3, "test%3d: superblock uncompressable data, too many nocompress superblocks : ", testNb++) { ZSTD_CCtx* cctx = ZSTD_createCCtx(); - void* src = CNBuffer; void* dst = compressedBuffer; + BYTE* src = CNBuffer; BYTE* dst = compressedBuffer; size_t srcSize = 321656; size_t dstCapacity = ZSTD_compressBound(srcSize); /* This is the number of bytes to stream before ending. This value @@ -511,7 +511,7 @@ static int basicUnitTests(U32 const seed, double compressibility) const size_t compressablePartSize = srcSize/3; const size_t uncompressablePartSize = srcSize-compressablePartSize; RDG_genBuffer(CNBuffer, compressablePartSize, 0.5, 0.5, seed); - RDG_genBuffer((void*)((char*)CNBuffer+compressablePartSize), uncompressablePartSize, 0, 0, seed); + RDG_genBuffer(CNBuffer+compressablePartSize, uncompressablePartSize, 0, 0, seed); /* Setting target block size so that superblock is used */ From 4399eed42e06ebcf0cfc8ef60350e0e98fc149a2 Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Fri, 13 Dec 2019 16:38:11 -0800 Subject: [PATCH 5/7] Adding explict cast to satisfy appveyor ci --- tests/fuzzer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index d413be040..47d0e012a 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -492,7 +492,7 @@ static int basicUnitTests(U32 const seed, double compressibility) DISPLAYLEVEL(3, "test%3d: superblock uncompressable data, too many nocompress superblocks : ", testNb++) { ZSTD_CCtx* cctx = ZSTD_createCCtx(); - BYTE* src = CNBuffer; BYTE* dst = compressedBuffer; + BYTE* src = (BYTE*)CNBuffer; BYTE* dst = (BYTE*)compressedBuffer; size_t srcSize = 321656; size_t dstCapacity = ZSTD_compressBound(srcSize); /* This is the number of bytes to stream before ending. This value From 989ce13e190e446996379a96758b70562540ba26 Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Fri, 13 Dec 2019 16:50:21 -0800 Subject: [PATCH 6/7] One more type conversion --- tests/fuzzer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 47d0e012a..7880c5d43 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -511,7 +511,7 @@ static int basicUnitTests(U32 const seed, double compressibility) const size_t compressablePartSize = srcSize/3; const size_t uncompressablePartSize = srcSize-compressablePartSize; RDG_genBuffer(CNBuffer, compressablePartSize, 0.5, 0.5, seed); - RDG_genBuffer(CNBuffer+compressablePartSize, uncompressablePartSize, 0, 0, seed); + RDG_genBuffer((BYTE*)CNBuffer+compressablePartSize, uncompressablePartSize, 0, 0, seed); /* Setting target block size so that superblock is used */ From 56415efc76b8407df39e872b2f5bf65c6badf507 Mon Sep 17 00:00:00 2001 From: Bimba Shrestha Date: Tue, 17 Dec 2019 17:16:51 -0800 Subject: [PATCH 7/7] Constifying, malloc check and naming nit --- lib/compress/zstd_compress.c | 2 +- tests/fuzzer.c | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 338f8dd56..a674cea40 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2568,7 +2568,7 @@ static size_t ZSTD_compress_frameChunk (ZSTD_CCtx* cctx, * greater than the optimistic number of blocks we still have remaining. * This might be UNset when data is uncompressable and we're streaming. */ - int enoughDstCapacityForNoCompressSuperBlocks = + const int enoughDstCapacityForNoCompressSuperBlocks = (dstCapacity / (blockSize + 7 /* header + checksum */)) > (srcSize / blockSize); assert(cctx->appliedParams.cParams.windowLog <= ZSTD_WINDOWLOG_MAX); diff --git a/tests/fuzzer.c b/tests/fuzzer.c index 7880c5d43..337455d9f 100644 --- a/tests/fuzzer.c +++ b/tests/fuzzer.c @@ -489,10 +489,10 @@ static int basicUnitTests(U32 const seed, double compressibility) } DISPLAYLEVEL(3, "OK \n"); - DISPLAYLEVEL(3, "test%3d: superblock uncompressable data, too many nocompress superblocks : ", testNb++) + DISPLAYLEVEL(3, "test%3d: superblock uncompressible data, too many nocompress superblocks : ", testNb++) { - ZSTD_CCtx* cctx = ZSTD_createCCtx(); - BYTE* src = (BYTE*)CNBuffer; BYTE* dst = (BYTE*)compressedBuffer; + ZSTD_CCtx* const cctx = ZSTD_createCCtx(); + const BYTE* src = (BYTE*)CNBuffer; BYTE* dst = (BYTE*)compressedBuffer; size_t srcSize = 321656; size_t dstCapacity = ZSTD_compressBound(srcSize); /* This is the number of bytes to stream before ending. This value @@ -501,20 +501,21 @@ static int basicUnitTests(U32 const seed, double compressibility) const size_t streamCompressThreshold = 161792; const size_t streamCompressDelta = 1024; - /* The first 1/3 of the buffer is compressable and the last 2/3 is - * uncompressable. This is an approximation of the type of data + /* The first 1/3 of the buffer is compressible and the last 2/3 is + * uncompressible. This is an approximation of the type of data * the fuzzer generated to catch this bug. Streams like this were making * zstd generate noCompress superblocks (which are larger than the src * they come from). Do this enough times, and we'll run out of room * and throw a dstSize_tooSmall error. */ - const size_t compressablePartSize = srcSize/3; - const size_t uncompressablePartSize = srcSize-compressablePartSize; - RDG_genBuffer(CNBuffer, compressablePartSize, 0.5, 0.5, seed); - RDG_genBuffer((BYTE*)CNBuffer+compressablePartSize, uncompressablePartSize, 0, 0, seed); + const size_t compressiblePartSize = srcSize/3; + const size_t uncompressiblePartSize = srcSize-compressiblePartSize; + RDG_genBuffer(CNBuffer, compressiblePartSize, 0.5, 0.5, seed); + RDG_genBuffer((BYTE*)CNBuffer+compressiblePartSize, uncompressiblePartSize, 0, 0, seed); /* Setting target block size so that superblock is used */ + assert(cctx != NULL); ZSTD_CCtx_setParameter(cctx, ZSTD_c_targetCBlockSize, 81); { size_t read;