Merge pull request #3258 from daniellerozenblit/null-buffer-decompress

Fix undefined behavior in ZSTD_decompressStream()

.github/workflows/dev-long-tests.yml

@@ -50,6 +50,13 @@ jobs:
     - name: thread sanitizer zstreamtest
       run: CC=clang ZSTREAM_TESTTIME=-T3mn make tsan-test-zstream
 
+  ubsan-zstreamtest:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: undefined behavior sanitizer zstreamtest
+      run: CC=clang make uasan-test-zstream
+
   # lasts ~15mn
   tsan-fuzztest:
     runs-on: ubuntu-latest
@@ -69,6 +76,13 @@ jobs:
         make gcc8install
         CC=gcc-8 make -j uasan-test-zstd </dev/null V=1
 
+  clang-asan-ubsan-testzstd:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: clang + ASan + UBSan + Test Zstd
+      run: CC=clang make -j uasan-test-zstd </dev/null V=1
+
   gcc-asan-ubsan-testzstd-32bit:
     runs-on: ubuntu-latest
     steps:
@@ -93,6 +107,13 @@ jobs:
         make gcc8install
         CC=gcc-8 FUZZER_FLAGS="--long-tests" make clean uasan-fuzztest
 
+  clang-asan-ubsan-fuzz:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: clang + ASan + UBSan + Fuzz Test
+      run: CC=clang FUZZER_FLAGS="--long-tests" make clean uasan-fuzztest
+
   gcc-asan-ubsan-fuzz32:
     runs-on: ubuntu-latest
     steps:
@@ -103,6 +124,16 @@ jobs:
         make libc6install
         CFLAGS="-O3 -m32" FUZZER_FLAGS="--long-tests" make uasan-fuzztest
 
+  clang-asan-ubsan-fuzz32:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: clang + ASan + UBSan + Fuzz Test 32bit
+      run: |
+        sudo apt-get -qqq update
+        make libc6install
+        CC=clang CFLAGS="-O3 -m32" FUZZER_FLAGS="--long-tests" make uasan-fuzztest
+
   asan-ubsan-regression:
     runs-on: ubuntu-latest
     steps:
@@ -110,6 +141,13 @@ jobs:
     - name: ASan + UBSan + Regression Test
       run: make -j uasanregressiontest
 
+  clang-ubsan-regression:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: clang + ASan + UBSan + Regression Test
+      run: CC=clang make -j uasanregressiontest
+
   msan-regression:
     runs-on: ubuntu-latest
     steps:

lib/decompress/zstd_decompress.c

@@ -2059,7 +2059,7 @@ size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* output, ZSTD_inB
                     if (ZSTD_isError(decompressedSize)) return decompressedSize;
                     DEBUGLOG(4, "shortcut to single-pass ZSTD_decompress_usingDDict()")
                     ip = istart + cSize;
-                    op += decompressedSize;
+                    op = op ? op + decompressedSize : op; /* can occur if frameContentSize = 0 (empty frame) */
                     zds->expected = 0;
                     zds->streamStage = zdss_init;
                     someMoreWork = 0;
@@ -2177,14 +2177,17 @@ size_t ZSTD_decompressStream(ZSTD_DStream* zds, ZSTD_outBuffer* output, ZSTD_inB
                 break;
             }
         case zdss_flush:
-            {   size_t const toFlushSize = zds->outEnd - zds->outStart;
+            if (op != NULL) {
+                size_t const toFlushSize = zds->outEnd - zds->outStart;
                 size_t const flushedSize = ZSTD_limitCopy(op, (size_t)(oend-op), zds->outBuff + zds->outStart, toFlushSize);
+
                 op += flushedSize;
+
                 zds->outStart += flushedSize;
                 if (flushedSize == toFlushSize) {  /* flush completed */
                     zds->streamStage = zdss_read;
                     if ( (zds->outBuffSize < zds->fParams.frameContentSize)
-                      && (zds->outStart + zds->fParams.blockSizeMax > zds->outBuffSize) ) {
+                    && (zds->outStart + zds->fParams.blockSizeMax > zds->outBuffSize) ) {
                         DEBUGLOG(5, "restart filling outBuff from beginning (left:%i, needed:%u)",
                                 (int)(zds->outBuffSize - zds->outStart),
                                 (U32)zds->fParams.blockSizeMax);

tests/.gitignore

@@ -12,6 +12,7 @@ zstreamtest
 zstreamtest32
 zstreamtest_asan
 zstreamtest_tsan
+zstreamtest_ubsan
 zstreamtest-dll
 datagen
 paramgrill

tests/Makefile

@@ -183,6 +183,10 @@ zstreamtest_tsan : CFLAGS += -fsanitize=thread
 zstreamtest_tsan : $(ZSTREAMFILES)
 	$(LINK.c) $(MULTITHREAD) $^ -o $@$(EXT)
 
+zstreamtest_ubsan : CFLAGS += -fsanitize=undefined
+zstreamtest_ubsan : $(ZSTREAMFILES)
+	$(LINK.c) $(MULTITHREAD) $^ -o $@$(EXT)
+
 # note : broken : requires symbols unavailable from dynamic library
 zstreamtest-dll : $(ZSTDDIR)/common/xxhash.c  # xxh symbols not exposed from dll
 zstreamtest-dll : $(ZSTREAM_LOCAL_FILES)
@@ -238,6 +242,7 @@ clean:
         fuzzer$(EXT) fuzzer32$(EXT) \
         fuzzer-dll$(EXT) zstreamtest-dll$(EXT) \
         zstreamtest$(EXT) zstreamtest32$(EXT) \
+		zstreamtest_ubsan$(EXT) zstreamtest_asan$(EXT) zstreamtest_tsan$(EXT) \
         datagen$(EXT) paramgrill$(EXT) roundTripCrash$(EXT) longmatch$(EXT) \
         symbols$(EXT) invalidDictionaries$(EXT) legacy$(EXT) poolTests$(EXT) \
         decodecorpus$(EXT) checkTag$(EXT) bigdict$(EXT)

tests/zstreamtest.c

@@ -522,7 +522,7 @@ static int basicUnitTests(U32 seed, double compressibility)
     }
     DISPLAYLEVEL(3, "OK \n");
 
-    DISPLAYLEVEL(3, "test%3i : NULL buffers : ", testNb++);
+    DISPLAYLEVEL(3, "test%3i : NULL output and NULL input : ", testNb++);
     inBuff.src = NULL;
     inBuff.size = 0;
     inBuff.pos = 0;
@@ -548,6 +548,36 @@ static int basicUnitTests(U32 seed, double compressibility)
     {   size_t const ret = ZSTD_decompressStream(zd, &outBuff, &inBuff);
         if (ret != 0) goto _output_error;
     }
+    DISPLAYLEVEL(3, "OK\n");
+
+    DISPLAYLEVEL(3, "test%3i : NULL output buffer with non-NULL input : ", testNb++);
+    {
+        const char* test = "aa";
+        inBuff.src = test;
+        inBuff.size = 2;
+        inBuff.pos = 0;
+        outBuff.dst = NULL;
+        outBuff.size = 0;
+        outBuff.pos = 0;
+        CHECK_Z( ZSTD_compressStream(zc, &outBuff, &inBuff) );
+        CHECK(inBuff.pos != inBuff.size, "Entire input should be consumed");
+        CHECK_Z( ZSTD_endStream(zc, &outBuff) );
+        outBuff.dst = (char*)(compressedBuffer);
+        outBuff.size = compressedBufferSize;
+        outBuff.pos = 0;
+        {   size_t const r = ZSTD_endStream(zc, &outBuff);
+            CHECK(r != 0, "Error or some data not flushed (ret=%zu)", r);
+        }
+        inBuff.src = outBuff.dst;
+        inBuff.size = outBuff.pos;
+        inBuff.pos = 0;
+        outBuff.dst = NULL;
+        outBuff.size = 0;
+        outBuff.pos = 0;
+        CHECK_Z( ZSTD_initDStream(zd) );
+        CHECK_Z(ZSTD_decompressStream(zd, &outBuff, &inBuff));
+    }
+
     DISPLAYLEVEL(3, "OK\n");
     /* _srcSize compression test */
     DISPLAYLEVEL(3, "test%3i : compress_srcSize %u bytes : ", testNb++, COMPRESSIBLE_NOISE_LENGTH);