Merge pull request #10563 from keszybz/lz4-quickfix-quickfix

journal: adapt for new improved LZ4_decompress_safe_partial()

README

@@ -148,7 +148,7 @@ REQUIREMENTS:
         libacl (optional)
         libselinux (optional)
         liblzma (optional)
-        liblz4 >= 119 (optional)
+        liblz4 >= 1.3.0 / 130 (optional)
         libgcrypt (optional)
         libqrencode (optional)
         libmicrohttpd (optional)

meson.build

@@ -1092,6 +1092,7 @@ conf.set10('HAVE_XZ', have)
 want_lz4 = get_option('lz4')
 if want_lz4 != 'false' and not fuzzer_build
         liblz4 = dependency('liblz4',
+                            version : '>= 1.3.0',
                             required : want_lz4 == 'true')
         have = liblz4.found()
 else

src/fuzz/fuzz-compress.c

@@ -0,0 +1,80 @@
+/* SPDX-License-Identifier: LGPL-2.1+ */
+
+#include <errno.h>
+
+#include "alloc-util.h"
+#include "compress.h"
+#include "fuzz.h"
+
+static int compress(int alg,
+                    const void *src, uint64_t src_size,
+                    void *dst, size_t dst_alloc_size, size_t *dst_size) {
+
+        if (alg == OBJECT_COMPRESSED_LZ4)
+                return compress_blob_lz4(src, src_size, dst, dst_alloc_size, dst_size);
+        if (alg == OBJECT_COMPRESSED_XZ)
+                return compress_blob_xz(src, src_size, dst, dst_alloc_size, dst_size);
+        return -EOPNOTSUPP;
+}
+
+typedef struct header {
+        uint32_t alg:2; /* We have only two compression algorithms so far, but we might add
+                         * more in the future. Let's make this a bit wider so our fuzzer
+                         * cases remain stable in the future. */
+        uint32_t sw_len;
+        uint32_t sw_alloc;
+        uint32_t reserved[3]; /* Extra space to keep fuzz cases stable in case we need to
+                               * add stuff in the future. */
+        uint8_t data[];
+} header;
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
+        _cleanup_free_ void *buf = NULL, *buf2 = NULL;
+        int r;
+
+        if (size < offsetof(header, data) + 1)
+                return 0;
+
+        const header *h = (struct header*) data;
+        const size_t data_len = size - offsetof(header, data);
+
+        int alg = h->alg;
+
+        /* We don't want to fill the logs with messages about parse errors.
+         * Disable most logging if not running standalone */
+        if (!getenv("SYSTEMD_LOG_LEVEL"))
+                log_set_max_level(LOG_CRIT);
+
+        log_info("Using compression %s, data size=%zu",
+                 object_compressed_to_string(alg) ?: "(none)",
+                 data_len);
+
+        buf = malloc(MAX(size, 128u)); /* Make the buffer a bit larger for very small data */
+        if (!buf) {
+                log_oom();
+                return 0;
+        }
+
+        size_t csize;
+        r = compress(alg, h->data, data_len, buf, size, &csize);
+        if (r < 0) {
+                log_error_errno(r, "Compression failed: %m");
+                return 0;
+        }
+
+        log_debug("Compressed %zu bytes to → %zu bytes", data_len, csize);
+
+        size_t sw_alloc = MAX(h->sw_alloc, 1u);
+        buf2 = malloc(sw_alloc);
+        if (!buf) {
+                log_oom();
+                return 0;
+        }
+
+        size_t sw_len = MIN(data_len - 1, h->sw_len);
+
+        r = decompress_startswith(alg, buf, csize, &buf2, &sw_alloc, h->data, sw_len, h->data[sw_len]);
+        assert_se(r > 0);
+
+        return 0;
+}

src/fuzz/meson.build

@@ -67,4 +67,8 @@ fuzzers += [
          [libsystemd_journal_remote,
           libshared],
          []],
+
+        [['src/fuzz/fuzz-compress.c'],
+         [libshared],
+         []],
 ]

src/journal/compress.c

@@ -95,11 +95,7 @@ int compress_blob_lz4(const void *src, uint64_t src_size,
         if (src_size < 9)
                 return -ENOBUFS;
 
-#if LZ4_VERSION_NUMBER >= 10700
         r = LZ4_compress_default(src, (char*)dst + 8, src_size, (int) dst_alloc_size - 8);
-#else
-        r = LZ4_compress_limitedOutput(src, (char*)dst + 8, src_size, (int) dst_alloc_size - 8);
-#endif
         if (r <= 0)
                 return -ENOBUFS;
 
@@ -294,7 +290,6 @@ int decompress_startswith_lz4(const void *src, uint64_t src_size,
          * prefix */
 
         int r;
-        size_t size;
 
         assert(src);
         assert(src_size > 0);
@@ -311,23 +306,37 @@ int decompress_startswith_lz4(const void *src, uint64_t src_size,
 
         r = LZ4_decompress_safe_partial((char*)src + 8, *buffer, src_size - 8,
                                         prefix_len + 1, *buffer_size);
-        if (r >= 0)
-                size = (unsigned) r;
-        else {
-                /* lz4 always tries to decode full "sequence", so in
-                 * pathological cases might need to decompress the
-                 * full field. */
+        /* One lz4 < 1.8.3, we might get "failure" (r < 0), or "success" where
+         * just a part of the buffer is decompressed. But if we get a smaller
+         * amount of bytes than requested, we don't know whether there isn't enough
+         * data to fill the requested size or whether we just got a partial answer.
+         */
+        if (r < 0 || (size_t) r < prefix_len + 1) {
+                size_t size;
+
+                if (LZ4_versionNumber() >= 10803)
+                        /* We trust that the newer lz4 decompresses the number of bytes we
+                         * requested if available in the compressed string. */
+                        return 0;
+
+                if (r > 0)
+                        /* Compare what we have first, in case of mismatch we can
+                         * shortcut the full comparison. */
+                        if (memcmp(*buffer, prefix, r) != 0)
+                                return 0;
+
+                /* Before version 1.8.3, lz4 always tries to decode full a "sequence",
+                 * so in pathological cases might need to decompress the full field. */
                 r = decompress_blob_lz4(src, src_size, buffer, buffer_size, &size, 0);
                 if (r < 0)
                         return r;
+
+                if (size < prefix_len + 1)
+                        return 0;
         }
 
-        if (size >= prefix_len + 1)
-                return memcmp(*buffer, prefix, prefix_len) == 0 &&
-                        ((const uint8_t*) *buffer)[prefix_len] == extra;
-        else
-                return 0;
-
+        return memcmp(*buffer, prefix, prefix_len) == 0 &&
+                ((const uint8_t*) *buffer)[prefix_len] == extra;
 #else
         return -EPROTONOSUPPORT;
 #endif

src/journal/test-compress.c

@@ -132,6 +132,32 @@ static void test_decompress_startswith(int compression,
         assert_se(r > 0);
 }
 
+static void test_decompress_startswith_short(int compression,
+                                             compress_blob_t compress,
+                                             decompress_sw_t decompress_sw) {
+
+#define TEXT "HUGE=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+
+        char buf[1024];
+        size_t i, csize;
+        int r;
+
+        log_info("/* %s with %s */", __func__, object_compressed_to_string(compression));
+
+        r = compress(TEXT, sizeof TEXT, buf, sizeof buf, &csize);
+        assert_se(r == 0);
+
+        for (i = 1; i < strlen(TEXT); i++) {
+                size_t alloc_size = i;
+                _cleanup_free_ void *buf2 = NULL;
+
+                assert_se(buf2 = malloc(i));
+
+                assert_se(decompress_sw(buf, csize, &buf2, &alloc_size, TEXT, i, TEXT[i]) == 1);
+                assert_se(decompress_sw(buf, csize, &buf2, &alloc_size, TEXT, i, 'y') == 0);
+        }
+}
+
 static void test_compress_stream(int compression,
                                  const char* cat,
                                  compress_stream_t compress,
@@ -198,21 +224,17 @@ static void test_compress_stream(int compression,
 
 #if HAVE_LZ4
 static void test_lz4_decompress_partial(void) {
-        char buf[20000];
+        char buf[20000], buf2[100];
         size_t buf_size = sizeof(buf), compressed;
         int r;
         _cleanup_free_ char *huge = NULL;
 
 #define HUGE_SIZE (4096*1024)
-        huge = malloc(HUGE_SIZE);
+        assert_se(huge = malloc(HUGE_SIZE));
         memset(huge, 'x', HUGE_SIZE);
         memcpy(huge, "HUGE=", 5);
 
-#if LZ4_VERSION_NUMBER >= 10700
         r = LZ4_compress_default(huge, buf, HUGE_SIZE, buf_size);
-#else
-        r = LZ4_compress_limitedOutput(huge, buf, HUGE_SIZE, buf_size);
-#endif
         assert_se(r >= 0);
         compressed = r;
         log_info("Compressed %i → %zu", HUGE_SIZE, compressed);
@@ -227,14 +249,15 @@ static void test_lz4_decompress_partial(void) {
         assert_se(r >= 0);
         log_info("Decompressed partial %i/%i → %i", 12, HUGE_SIZE, r);
 
-        /* We expect this to fail, because that's how current lz4 works. If this
-         * call succeeds, then lz4 has been fixed, and we need to change our code.
-         */
-        r = LZ4_decompress_safe_partial(buf, huge,
-                                        compressed,
-                                        12, HUGE_SIZE-1);
-        assert_se(r < 0);
-        log_info("Decompressed partial %i/%i → %i", 12, HUGE_SIZE-1, r);
+        for (size_t size = 1; size < sizeof(buf2); size++) {
+                /* This failed in older lz4s but works in newer ones. */
+                r = LZ4_decompress_safe_partial(buf, buf2, compressed, size, size);
+                log_info("Decompressed partial %zu/%zu → %i (%s)", size, size, r,
+                                                                   r < 0 ? "bad" : "good");
+                if (r >= 0 && LZ4_versionNumber() >= 10803)
+                        /* lz4 <= 1.8.2 should fail that test, let's only check for newer ones */
+                        assert_se(memcmp(buf2, huge, r) == 0);
+        }
 }
 #endif
 
@@ -276,6 +299,9 @@ int main(int argc, char *argv[]) {
 
         test_compress_stream(OBJECT_COMPRESSED_XZ, "xzcat",
                              compress_stream_xz, decompress_stream_xz, srcfile);
+
+        test_decompress_startswith_short(OBJECT_COMPRESSED_XZ, compress_blob_xz, decompress_startswith_xz);
+
 #else
         log_info("/* XZ test skipped */");
 #endif
@@ -300,6 +326,9 @@ int main(int argc, char *argv[]) {
                              compress_stream_lz4, decompress_stream_lz4, srcfile);
 
         test_lz4_decompress_partial();
+
+        test_decompress_startswith_short(OBJECT_COMPRESSED_LZ4, compress_blob_lz4, decompress_startswith_lz4);
+
 #else
         log_info("/* LZ4 test skipped */");
 #endif