From 0012332ce0acf589da4831e9f9b2e79ce12eed9d Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Fri, 9 Dec 2016 17:15:33 -0800 Subject: [PATCH 1/5] Fix compression segfault When the overflow protection kicks in, it makes sure that ip - ctx->base isn't too large. However, it didn't ensure that saved offsets are still valid. This change ensures that any valid offsets (<= windowLog) are still representable after the update. The bug would shop up on line 1056, when `offset_1 > current + 1`, which causes an underflow. This in turn, would cause a segfault on line 1063. The input must necessarily be longer than 1 GB for this issue to occur. Even then, it only occurs if one of the last 3 matches is larger than the chain size and block size. --- lib/compress/zstd_compress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index 665d09c0f..a575534c3 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2277,7 +2277,7 @@ static size_t ZSTD_compress_generic (ZSTD_CCtx* cctx, if (cctx->lowLimit > (1<<30)) { U32 const btplus = (cctx->params.cParams.strategy == ZSTD_btlazy2) | (cctx->params.cParams.strategy == ZSTD_btopt) | (cctx->params.cParams.strategy == ZSTD_btopt2); U32 const chainMask = (1 << (cctx->params.cParams.chainLog - btplus)) - 1; - U32 const supLog = MAX(cctx->params.cParams.chainLog, 17 /* blockSize */); + U32 const supLog = MAX(MAX(cctx->params.cParams.chainLog, 17 /* blockSize */), cctx->params.cParams.windowLog); U32 const newLowLimit = (cctx->lowLimit & chainMask) + (1 << supLog); /* preserve position % chainSize, ensure current-repcode doesn't underflow */ U32 const correction = cctx->lowLimit - newLowLimit; ZSTD_reduceIndex(cctx, correction); From 3826207a70779477703fcfe11f652d5cea9cff8c Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Sat, 10 Dec 2016 18:46:55 -0800 Subject: [PATCH 2/5] Simplify segfault fix Take advantage of the fact that `chainLog <= windowLog`. --- lib/compress/zstd_compress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/compress/zstd_compress.c b/lib/compress/zstd_compress.c index a575534c3..aef769fc0 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2277,7 +2277,7 @@ static size_t ZSTD_compress_generic (ZSTD_CCtx* cctx, if (cctx->lowLimit > (1<<30)) { U32 const btplus = (cctx->params.cParams.strategy == ZSTD_btlazy2) | (cctx->params.cParams.strategy == ZSTD_btopt) | (cctx->params.cParams.strategy == ZSTD_btopt2); U32 const chainMask = (1 << (cctx->params.cParams.chainLog - btplus)) - 1; - U32 const supLog = MAX(MAX(cctx->params.cParams.chainLog, 17 /* blockSize */), cctx->params.cParams.windowLog); + U32 const supLog = MAX(cctx->params.cParams.windowLog, 17 /* blockSize */); U32 const newLowLimit = (cctx->lowLimit & chainMask) + (1 << supLog); /* preserve position % chainSize, ensure current-repcode doesn't underflow */ U32 const correction = cctx->lowLimit - newLowLimit; ZSTD_reduceIndex(cctx, correction); From 0acae734f17e43ccceb8bd6bd3f1eee0d179ca9f Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Sat, 10 Dec 2016 19:12:13 -0800 Subject: [PATCH 3/5] Add exposing test case --- tests/.gitignore | 1 + tests/Makefile | 10 +++++-- tests/longmatch.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 tests/longmatch.c diff --git a/tests/.gitignore b/tests/.gitignore index c8f9ae79e..b558ac258 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -11,6 +11,7 @@ datagen paramgrill paramgrill32 roundTripCrash +longmatch # Tmp test directory zstdtest diff --git a/tests/Makefile b/tests/Makefile index fbee21448..6110465f6 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -124,6 +124,9 @@ datagen : $(PRGDIR)/datagen.c datagencli.c roundTripCrash : $(ZSTD_FILES) roundTripCrash.c $(CC) $(FLAGS) $^ -o $@$(EXT) +longmatch : $(ZSTD_FILES) longmatch.c + $(CC) $(FLAGS) $^ -o $@$(EXT) + namespaceTest: if $(CC) namespaceTest.c ../lib/common/xxhash.c -o $@ ; then echo compilation should fail; exit 1 ; fi $(RM) $@ @@ -140,7 +143,7 @@ clean: fullbench-lib$(EXT) fullbench-dll$(EXT) \ fuzzer$(EXT) fuzzer32$(EXT) zbufftest$(EXT) zbufftest32$(EXT) \ zstreamtest$(EXT) zstreamtest32$(EXT) \ - datagen$(EXT) paramgrill$(EXT) roundTripCrash$(EXT) + datagen$(EXT) paramgrill$(EXT) roundTripCrash$(EXT) longmatch$(EXT) @echo Cleaning completed @@ -180,7 +183,7 @@ zstd-playTests: datagen file $(ZSTD) ZSTD="$(QEMU_SYS) $(ZSTD)" ./playTests.sh $(ZSTDRTTEST) -test: test-zstd test-fullbench test-fuzzer test-zstream +test: test-zstd test-fullbench test-fuzzer test-zstream test-longmatch test32: test-zstd32 test-fullbench32 test-fuzzer32 test-zstream32 @@ -237,4 +240,7 @@ test-zstream: zstreamtest test-zstream32: zstreamtest32 $(QEMU_SYS) ./zstreamtest32 $(ZSTREAM_TESTTIME) +test-longmatch: longmatch + $(QEMU_SYS) ./longmatch + endif diff --git a/tests/longmatch.c b/tests/longmatch.c new file mode 100644 index 000000000..b99dccc6f --- /dev/null +++ b/tests/longmatch.c @@ -0,0 +1,72 @@ +#define ZSTD_STATIC_LINKING_ONLY +#include +#include +#include +#include + +void compress(ZSTD_CStream *ctx, ZSTD_outBuffer out, const void *data, size_t size) { + ZSTD_inBuffer in = { data, size, 0 }; + while (in.pos < in.size) { + ZSTD_outBuffer tmp = out; + const size_t rc = ZSTD_compressStream(ctx, &tmp, &in); + if (ZSTD_isError(rc)) { + exit(5); + } + } + ZSTD_outBuffer tmp = out; + const size_t rc = ZSTD_flushStream(ctx, &tmp); + if (rc != 0) { exit(6); } +} + +int main() { + ZSTD_CStream *ctx; + ZSTD_parameters params = {}; + size_t rc; + unsigned windowLog; + /* Create stream */ + ctx = ZSTD_createCStream(); + if (!ctx) { return 1; } + /* Set parameters */ + params.cParams.windowLog = 18; + params.cParams.chainLog = 13; + params.cParams.hashLog = 14; + params.cParams.searchLog = 1; + params.cParams.searchLength = 7; + params.cParams.targetLength = 16; + params.cParams.strategy = ZSTD_fast; + windowLog = params.cParams.windowLog; + /* Initialize stream */ + rc = ZSTD_initCStream_advanced(ctx, NULL, 0, params, 0); + if (ZSTD_isError(rc)) { return 2; } + { + uint64_t compressed = 0; + const uint64_t toCompress = ((uint64_t)1) << 33; + const size_t size = 1 << windowLog; + size_t pos = 0; + char *srcBuffer = (char*) malloc(1 << windowLog); + char *dstBuffer = (char*) malloc(ZSTD_compressBound(1 << windowLog)); + ZSTD_outBuffer out = { dstBuffer, ZSTD_compressBound(1 << windowLog), 0 }; + const char match[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"; + const size_t randomData = (1 << windowLog) - 2*sizeof(match); + for (size_t i = 0; i < sizeof(match); ++i) { + srcBuffer[i] = match[i]; + } + for (size_t i = 0; i < randomData; ++i) { + srcBuffer[sizeof(match) + i] = (char)(rand() & 0xFF); + } + for (size_t i = 0; i < sizeof(match); ++i) { + srcBuffer[sizeof(match) + randomData + i] = match[i]; + } + compress(ctx, out, srcBuffer, size); + compressed += size; + while (compressed < toCompress) { + const size_t block = rand() % (size - pos + 1); + if (pos == size) { pos = 0; } + compress(ctx, out, srcBuffer + pos, block); + pos += block; + compressed += block; + } + free(srcBuffer); + free(dstBuffer); + } +} From 5cc85cf18398cde78cef206a4f0ef381c0d91846 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Sat, 10 Dec 2016 19:31:55 -0800 Subject: [PATCH 4/5] Switch uint64_t to U64 --- tests/longmatch.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/longmatch.c b/tests/longmatch.c index b99dccc6f..46dcbc77a 100644 --- a/tests/longmatch.c +++ b/tests/longmatch.c @@ -1,8 +1,10 @@ #define ZSTD_STATIC_LINKING_ONLY -#include +#include "zstd.h" +#include "mem.h" #include #include #include +#include void compress(ZSTD_CStream *ctx, ZSTD_outBuffer out, const void *data, size_t size) { ZSTD_inBuffer in = { data, size, 0 }; @@ -39,8 +41,8 @@ int main() { rc = ZSTD_initCStream_advanced(ctx, NULL, 0, params, 0); if (ZSTD_isError(rc)) { return 2; } { - uint64_t compressed = 0; - const uint64_t toCompress = ((uint64_t)1) << 33; + U64 compressed = 0; + const U64 toCompress = ((U64)1) << 33; const size_t size = 1 << windowLog; size_t pos = 0; char *srcBuffer = (char*) malloc(1 << windowLog); From b547d212a191c6cd7c2b0b4b942833ac8c360c33 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Sat, 10 Dec 2016 23:17:36 -0800 Subject: [PATCH 5/5] Fix longmatch test build errors. --- tests/longmatch.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/tests/longmatch.c b/tests/longmatch.c index 46dcbc77a..c75c0d18e 100644 --- a/tests/longmatch.c +++ b/tests/longmatch.c @@ -6,29 +6,35 @@ #include #include -void compress(ZSTD_CStream *ctx, ZSTD_outBuffer out, const void *data, size_t size) { +int compress(ZSTD_CStream *ctx, ZSTD_outBuffer out, const void *data, size_t size) { ZSTD_inBuffer in = { data, size, 0 }; while (in.pos < in.size) { ZSTD_outBuffer tmp = out; const size_t rc = ZSTD_compressStream(ctx, &tmp, &in); if (ZSTD_isError(rc)) { - exit(5); + return 1; } } - ZSTD_outBuffer tmp = out; - const size_t rc = ZSTD_flushStream(ctx, &tmp); - if (rc != 0) { exit(6); } + { + ZSTD_outBuffer tmp = out; + const size_t rc = ZSTD_flushStream(ctx, &tmp); + if (rc != 0) { return 1; } + } + return 0; } -int main() { +int main(int argc, const char** argv) { ZSTD_CStream *ctx; - ZSTD_parameters params = {}; + ZSTD_parameters params; size_t rc; unsigned windowLog; + (void)argc; + (void)argv; /* Create stream */ ctx = ZSTD_createCStream(); if (!ctx) { return 1; } /* Set parameters */ + memset(¶ms, 0, sizeof(params)); params.cParams.windowLog = 18; params.cParams.chainLog = 13; params.cParams.hashLog = 14; @@ -50,25 +56,31 @@ int main() { ZSTD_outBuffer out = { dstBuffer, ZSTD_compressBound(1 << windowLog), 0 }; const char match[] = "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"; const size_t randomData = (1 << windowLog) - 2*sizeof(match); - for (size_t i = 0; i < sizeof(match); ++i) { + size_t i; + for (i = 0; i < sizeof(match); ++i) { srcBuffer[i] = match[i]; } - for (size_t i = 0; i < randomData; ++i) { + for (i = 0; i < randomData; ++i) { srcBuffer[sizeof(match) + i] = (char)(rand() & 0xFF); } - for (size_t i = 0; i < sizeof(match); ++i) { + for (i = 0; i < sizeof(match); ++i) { srcBuffer[sizeof(match) + randomData + i] = match[i]; } - compress(ctx, out, srcBuffer, size); + if (compress(ctx, out, srcBuffer, size)) { + return 1; + } compressed += size; while (compressed < toCompress) { const size_t block = rand() % (size - pos + 1); if (pos == size) { pos = 0; } - compress(ctx, out, srcBuffer + pos, block); + if (compress(ctx, out, srcBuffer + pos, block)) { + return 1; + } pos += block; compressed += block; } free(srcBuffer); free(dstBuffer); } + return 0; }