Merge patch series "gunzip: Fix spurious Z_BUF_ERROR in chunked gzwrite decompression"

Aristo Chen <[email protected]> says:

The dm_test_cmd_zip_gzwrite sandbox test occasionally fails in CI
with:

  12582912/16777216
  Error: inflate() returned -5

The chunked decompression loop added in commit 58e523fedf48 ("gunzip:
Implement chunked decompression") treats Z_BUF_ERROR from inflate()
as fatal. When an input chunk is exhausted at exactly the same time
as the write buffer fills up, the next inflate() call is made with
avail_in == 0, cannot make progress, and returns Z_BUF_ERROR. Per the
zlib documentation this only means "no progress was possible" and the
call should be repeated with more input, which is what the reference
implementation in zlib examples/zpipe.c does.

The failure needs the consumed/produced byte counts to line up with
both the chunk size and the write buffer size at once, with no
buffered output on the inflate side, which is why only certain random
payloads trigger it. Note that the failure offset above is a multiple
of the 1 MiB write buffer while gzwrite_chunk was SZ_1M + 1.

Patch 1 makes gzwrite() refill the input chunk in this situation.
Patch 2 adds a deterministic regression test which builds a gzip file
from two stored deflate blocks by hand and aligns the chunk boundary
with the write buffer boundary exactly, failing reliably without
patch 1.

Verified on sandbox and sandbox64:
- dm_test_cmd_gzwrite_chunk_boundary fails with -5 in 20 out of 20
  runs before the fix, passes 100 out of 100 runs after
- dm_test_cmd_zip_gzwrite fails 17 out of 2000 runs (about 1%)
  before the fix, every time with the same signature as the CI
  flake, and passes 2000 out of 2000 runs after
- dm_test_cmd_zip_unzip keeps passing

Link: https://lore.kernel.org/r/[email protected]

2 files changed, 126 insertions, 2 deletions

diff --git a/lib/gunzip.c b/lib/gunzip.c
index 20cc14f9688..b30cbfc34ef 100644
--- a/lib/gunzip.c
+++ b/lib/gunzip.c
@@ -246,6 +246,16 @@ int gzwrite(unsigned char *src, size_t len, struct blk_desc *dev,
 				s.next_out = writebuf;
 			}
 			r = inflate(&s, Z_SYNC_FLUSH);
+			if (r == Z_BUF_ERROR && !s.avail_in && payload_size) {
+				/*
+				 * The input chunk was exhausted at exactly
+				 * the same time as the write buffer filled
+				 * up, so no progress was possible. This is
+				 * not fatal, let the outer loop refill the
+				 * input chunk.
+				 */
+				break;
+			}
 			if ((r != Z_OK) &&
 			    (r != Z_STREAM_END)) {
 				printf("Error: inflate() returned %d\n", r);
diff --git a/test/cmd/unzip.c b/test/cmd/unzip.c
index 623a2785884..e33b6c3fb3a 100644
--- a/test/cmd/unzip.c
+++ b/test/cmd/unzip.c
@@ -101,11 +101,10 @@ static int dm_test_cmd_zip_unzip(struct unit_test_state *uts)
 }
 DM_TEST(dm_test_cmd_zip_unzip, UTF_CONSOLE);
 
-static int dm_test_cmd_zip_gzwrite(struct unit_test_state *uts)
+static int bind_mmc9(struct unit_test_state *uts)
 {
 	struct udevice *dev;
 	ofnode root, node;
-	int i, j, ret;
 
 	/* Enable the mmc9 node for this test */
 	root = oftree_root(oftree_default());
@@ -113,6 +112,15 @@ static int dm_test_cmd_zip_gzwrite(struct unit_test_state *uts)
 	ut_assert(ofnode_valid(node));
 	ut_assertok(lists_bind_fdt(gd->dm_root, node, &dev, NULL, false));
 
+	return 0;
+}
+
+static int dm_test_cmd_zip_gzwrite(struct unit_test_state *uts)
+{
+	int i, j, ret;
+
+	ut_assertok(bind_mmc9(uts));
+
 	for (i = 0; i < ARRAY_SIZE(sizes); i++) {
 		ret = do_test_cmd_zip_unzip(uts, sizes[i], true);
 		if (ret)
@@ -132,3 +140,109 @@ static int dm_test_cmd_zip_gzwrite(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_cmd_zip_gzwrite, UTF_CONSOLE);
+
+/*
+ * Regression test for the case where a decompression input chunk is
+ * exhausted at exactly the same time as the write buffer fills up, in
+ * which case gzwrite() used to call inflate() again with no input,
+ * receive Z_BUF_ERROR back and treat it as a fatal error.
+ *
+ * Craft a gzip file by hand from two stored (uncompressed) deflate
+ * blocks of 1 KiB each, and pick a chunk size that covers exactly the
+ * 5 byte header plus payload of the first stored block, so that with a
+ * 1 KiB write buffer the first chunk runs out precisely when the write
+ * buffer is full.
+ */
+#define STORED_BLK_HDR_LEN	5	/* deflate stored block header size */
+#define STORED_BLK_LEN		SZ_1K	/* payload bytes per stored block */
+
+static int gzwrite_chunk_boundary(struct unit_test_state *uts)
+{
+	static const u8 gzip_hdr[10] = {
+		0x1f, 0x8b,		/* magic */
+		0x08,			/* deflate */
+		0x00,			/* no flags */
+		0x00, 0x00, 0x00, 0x00,	/* mtime */
+		0x00,			/* extra flags */
+		0x03,			/* OS: unix */
+	};
+	unsigned long loadaddr = env_get_ulong("loadaddr", 16, 0);
+	unsigned long decaddr = loadaddr + SZ_1M;
+	u8 raw[2 * STORED_BLK_LEN];
+	const size_t rawsize = sizeof(raw);
+	unsigned char *gzmap = map_sysmem(loadaddr, sizeof(gzip_hdr) +
+					  2 * (STORED_BLK_HDR_LEN +
+					       STORED_BLK_LEN) + 8);
+	unsigned char *decmap = map_sysmem(decaddr, rawsize);
+	struct blk_desc *mmc_dev_desc;
+	const u16 len = STORED_BLK_LEN;
+	const u16 nlen = ~STORED_BLK_LEN & 0xffff;
+	size_t gzlen, cnt;
+	u8 *p = gzmap;
+	u32 crc;
+	int i;
+
+	ut_assertok(bind_mmc9(uts));
+
+	for (i = 0; i < rawsize; i++)
+		raw[i] = (i * 251) & 0xff;
+	crc = crc32(0, raw, rawsize);
+
+	memcpy(p, gzip_hdr, sizeof(gzip_hdr));
+	p += sizeof(gzip_hdr);
+	for (i = 0; i < 2; i++) {
+		*p++ = (i == 1) ? 0x01 : 0x00;	/* BFINAL on last block */
+		*p++ = len & 0xff;		/* LEN */
+		*p++ = len >> 8;
+		*p++ = nlen & 0xff;		/* NLEN */
+		*p++ = nlen >> 8;
+		memcpy(p, raw + i * STORED_BLK_LEN, STORED_BLK_LEN);
+		p += STORED_BLK_LEN;
+	}
+	*p++ = crc & 0xff;			/* CRC32, little endian */
+	*p++ = (crc >> 8) & 0xff;
+	*p++ = (crc >> 16) & 0xff;
+	*p++ = (crc >> 24) & 0xff;
+	*p++ = rawsize & 0xff;			/* ISIZE, little endian */
+	*p++ = (rawsize >> 8) & 0xff;
+	*p++ = (rawsize >> 16) & 0xff;
+	*p++ = (rawsize >> 24) & 0xff;
+	gzlen = p - gzmap;
+
+	ut_assertok(run_commandf("gzwrite mmc 9 %lx %zx %x", loadaddr,
+				 gzlen, STORED_BLK_LEN));
+	ut_assert_skip_to_line("\t%zu bytes, crc 0x%08x", rawsize, crc);
+
+	ut_asserteq(9, blk_get_device_by_str("mmc", "9", &mmc_dev_desc));
+	cnt = rawsize / mmc_dev_desc->blksz;
+	ut_assertok(run_commandf("mmc dev 9"));
+	ut_assert_nextline("switch to partitions #0, OK");
+	ut_assert_nextline("mmc9 is current device");
+
+	ut_assertok(run_commandf("mmc read %lx 0 %zx", decaddr, cnt));
+	ut_assert_nextline("MMC read: dev # 9, block # 0, count %zu ... %zu blocks read: OK",
+			   cnt, cnt);
+
+	ut_asserteq_mem(raw, decmap, rawsize);
+
+	ut_assert_console_end();
+
+	unmap_sysmem(gzmap);
+	unmap_sysmem(decmap);
+
+	return 0;
+}
+
+static int dm_test_cmd_gzwrite_chunk_boundary(struct unit_test_state *uts)
+{
+	int ret;
+
+	/* Input chunk: exactly one stored block header plus its payload */
+	ut_assertok(env_set_ulong("gzwrite_chunk",
+				  STORED_BLK_HDR_LEN + STORED_BLK_LEN));
+	ret = gzwrite_chunk_boundary(uts);
+	ut_assertok(env_set("gzwrite_chunk", NULL));
+
+	return ret;
+}
+DM_TEST(dm_test_cmd_gzwrite_chunk_boundary, UTF_CONSOLE);