From ecf90ca24b7031444133bc236a51683bd059dfc7 Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Mon, 13 Feb 2017 18:27:34 -0800 Subject: [PATCH 1/2] [zstdmt] Fix MSAN failure with ZSTD_p_forceWindow Reproduction steps: ``` make zstreamtest CC=clang CFLAGS="-O3 -g -fsanitize=memory -fsanitize-memory-track-origins" ./zstreamtest -vv -t4178 -i4178 -s4531 ``` How to get to the error in gdb (may be a more efficient way): * 2 breaks at zstd_compress.c:2418 -- in ZSTD_compressContinue_internal() * 2 breaks at zstd_compress.c:2276 -- in ZSTD_compressBlock_internal() * 1 break at zstd_compress.c:1547 Why the error occurred: When `zc->forceWindow == 1`, after calling `ZSTD_loadDictionaryContent()` we have `zc->loadedDictEnd == zc->nextToUpdate == 0`. But, we've really loaded up to `iend` into the dictionary. Then in `ZSTD_compressBlock_internal()` we see that `current > zc->nextToUpdate + 384`, so we load the last 192 bytes a second time. In this case the bytes we are loading are a block of all 0s, starting in the previous block. So when we are loading the last 192 bytes, we find a `match` in the future, 183 bytes beyond `ip`. Since the block is all 0s, the match extends to the end of the block. But in `ZSTD_count()` we only check that `pIn < pInLoopLimit`, but since `pMatch > pIn`, `pMatch` eventually points past the end of the buffer, causing the MSAN failure. The fix: The line changed sets sets `zc->nextToUpdate` to the end of the dictionary. This is the behavior that existed before `ZSTD_p_forceWindow` was introduced. This fixes the exposing test case. Since the code doesn't fail without `zc->forceWindow`, it makes sense that this works. I've run the command `./zstreamtest -T2mn` 64 times without failures. CI should also verify nothing obvious broke. --- 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 765c8e34d..91e81d9c2 100644 --- a/lib/compress/zstd_compress.c +++ b/lib/compress/zstd_compress.c @@ -2512,7 +2512,7 @@ static size_t ZSTD_loadDictionaryContent(ZSTD_CCtx* zc, const void* src, size_t return ERROR(GENERIC); /* strategy doesn't exist; impossible */ } - zc->nextToUpdate = zc->loadedDictEnd; + zc->nextToUpdate = (U32)(iend - zc->base); return 0; } From 74b81ada256f45b0ea69f50c3b9b3918faacc91a Mon Sep 17 00:00:00 2001 From: Nick Terrell Date: Tue, 14 Feb 2017 10:08:14 -0800 Subject: [PATCH 2/2] Don't run test-pool with QEMU > make test -n ... ./pool > make test -n QEMU_SYS=valgrind ... ./legacy # ./pool not run --- tests/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile b/tests/Makefile index f64be1695..2b58c949c 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -231,7 +231,7 @@ zstd-playTests: datagen ZSTD="$(QEMU_SYS) $(ZSTD)" ./playTests.sh $(ZSTDRTTEST) test: test-zstd test-fullbench test-fuzzer test-zstream test-invalidDictionaries test-legacy -ifneq ($(QEMU_SYS),qemu-ppc64-static) +ifeq ($(QEMU_SYS),) test: test-pool endif