From 51ecab034047ecba9f63bba9c669d12544e8a103 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Mon, 31 Aug 2026 10:36:23 -0700 Subject: [PATCH 1/9] gz: intercept the position and error family, and stop losing data at open Only five gz functions exist for acceleration -- gzopen, gzdopen, gzread, gzwrite, gzclose. The other 27 share zlib's single internal gz_state: the descriptor, both buffers, the stream position and the error latch. Once the shim owns the payload, those 27 describe a reality that no longer exists. Fifteen were fixed in #72. This closes the remaining ten, plus two cases of silent data loss found while working on them. A 30-check differential suite that runs the same program bare and under LD_PRELOAD went from 14 diverging checks to one, and that one is not comparable by construction (it corrupts a fixed offset in the compressed stream, which lands in different data in each arm because the shim's compressed bytes are not zlib's). Its four error-latch fields agree exactly. Two wrong answers reported as success ------------------------------------- gzseek followed by gzread returned data from the wrong offset, and gzrewind reported success without rewinding. Both consult zlib's error latch, so the latch is a precondition of fixing them rather than extra scope. zlib's seek is lazy: it records the distance, returns the position it promises, and the next read or write pays for it. That is reproduced here, including the parts that are easy to get wrong and were measured against bare zlib rather than assumed: - A forward seek on a write-mode file is allowed, and the gap is filled with zeros -- at the next write, at gzflush, and at gzclose_w. - A refused seek abandons a pending skip, because zlib clears state->seek before it checks for a negative offset. - gzungetc's pushed-back bytes are ahead of the file, so a forward seek passes over those first. Two files that plain zlib reads perfectly ----------------------------------------- A complete gzip member followed by bytes that are not 1f 8b: zlib ignores the trailer and reports a clean end of file. The shim returned -1 and lost the entire payload -- 1000 bytes and eof=1 bare, total=0 last=-1 under the shim. An empty .gz file, from a touch, a truncate or an interrupted write: zlib reads it as zero bytes with eof=1. The shim returned -1 with no error message set. Both are fixed by letting zlib decide whether a file is gzip at all, instead of writing a second copy of that test here. The real gzdirect is called once in read-mode gzopen/gzdopen and its verdict is used; the descriptor is rewound only when the shim is going to own reads. gzdirect itself is deliberately not intercepted -- zlib's peek is guarded by how == LOOK, so after this one preemptive call every later application gzdirect answers from cached state and reads nothing. The default answer is already the truthful one in all four cases. The invariant the rewind demands -------------------------------- A rewound file must never reach orig_gzread. Measured, with no shim involved: rewinding the descriptor under zlib yields 51,456 bytes of duplicates and then Z_DATA_ERROR. GzreadOwnedFile re-derived "should zlib read this" on every call, so descriptor ownership was never pinned to the file. Harmless before; not harmless once we rewind. It is now decided once, at open (shim_owns_reads), and a later config change may still choose which backend decompresses but never who reads the descriptor. Two tests cover it: a config flip mid-read, and gzopen(path, "rb0"), which reaches the same branch with the config untouched. zlib_owns_file is the mirror image. A file handed to zlib at open never has a byte of it pass through the shim, so zlib's own position and error state stay authoritative and every entry point below simply forwards. Also decided at open, for the same reason: path can turn into ZLIB part way through a file the shim already has bytes of. gzbuffer -------- Included here, not left for later, because the open-time look above makes zlib allocate its buffers, which would make zlib's own gzbuffer start refusing the first call on every read file. The refusals are now replicated against the shim's state. The requested size is accepted and then not applied: the shim's buffers are a fixed 256 KiB / 512 KiB and the accelerator paths are sized around that split. For any size smaller than those this is a performance difference, not a correctness one. The alternative -- pinning any file whose caller calls gzbuffer to plain zlib -- would take acceleration away from exactly the callers trying to tune for speed. Behaviour changes worth stating rather than discovering ------------------------------------------------------ - One 8 KB read per read-mode gzopen, and 31,776 bytes of zlib allocation per concurrently-open read-mode file (measured with mallinfo2 over 64 files). gzopen alone costs 256 bytes, so for files the shim ends up owning this is a 124x increase in per-file footprint: zlib allocates buffers and an inflate state the shim never uses. Nothing for a handful of files; for thousands of concurrent readers it is the per-stream footprint problem again. If that ever shows up, the mitigation is gzbuffer(file, 512) before the look, not a private magic check here. - gzoffset now reports where the shim's descriptor actually is, which is further into the file than zlib would be -- 512 KiB of compressed input per read against zlib's 8 KiB. Strictly better than before, when it reported the position of a stream zlib was not reading. - gz_load does not retry EINTR and latches Z_ERRNO. Adding the latch therefore makes a signal-interrupted read stickier than it was here. That removes an accidental leniency and matches zlib, but it is a change. - Pipes keep today's behaviour. A non-seekable descriptor cannot be rewound, so looking would force every pipe to zlib and cost pipe users their acceleration. A pipe carrying non-gzip data stays broken, and gzdirect on an accelerated pipe still loses 8 KB. Both remain open gaps. gzseek64, gztell64 and gzoffset64 are defined alongside the plain names, since an application built with -D_FILE_OFFSET_BITS=64 calls the 64-bit ones and the pair would otherwise disagree about where a file is positioned. gzopen64 stays out of scope: not exporting it is fail-safe. Tests: 15 new cases in GzipFileTest, on a position-stamped payload -- a repeating pattern makes a seek test pass while the shim reads from offset 0. Full suite 3267 passing, 6 pre-existing skips, 0 failures; clean under ASAN with leak detection on, and across all 8 permutations of DEBUG_LOG x ENABLE_STATISTICS x Debug/Release under -Werror. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 609 ++++++++++++++++++++++++++++++++ zlib_accel.cpp | 726 +++++++++++++++++++++++++++++++++++++- 2 files changed, 1328 insertions(+), 7 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index c6a1dcb..f72a7fc 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8047,6 +8047,615 @@ TEST_F(GzipFileTest, GzwriteAndGzreadRejectALengthThatDoesNotFitInInt) { DestroyBlock(input); } +// --------------------------------------------------------------------------- +// Position, seek and error state. +// --------------------------------------------------------------------------- + +// EnableSomeGzUncompressPath falls back to SetUncompressPath(ZLIB) when no +// backend is compiled in, which clears every accelerator flag and so hands the +// file to zlib at open. That is not the state the bookkeeping below lives in. +// +// A deployed host keeps the flag set for a backend it has, and the shim reads +// the flag rather than the compile-time macro: the flag makes the shim own the +// descriptor, and if the backend is not there its own inflate stream does the +// decompressing. That combination is what the differential runs against plain +// zlib were captured under, and it is reachable with nothing compiled in, so it +// is what these tests ask for. +static void EnableShimOwnedGzReads() { + SetConfig(USE_IAA_UNCOMPRESS, 0); + SetConfig(USE_QAT_UNCOMPRESS, 1); + SetConfig(USE_IGZIP_UNCOMPRESS, 0); + SetConfig(USE_ZLIB_UNCOMPRESS, 1); +} + +// Sixteen bytes per record, each naming its own offset. A repeating payload +// would let a read from the wrong offset look correct -- which is how the +// original gzseek check passed while the shim was reading from byte 0. +static std::string PositionStampedPayload(size_t records) { + std::string payload; + payload.reserve(records * 16); + char record[17]; + for (size_t i = 0; i < records; i++) { + snprintf(record, sizeof(record), "[off%08zu]", i * 16); + payload.append(record, 16); + } + return payload; +} + +TEST_F(GzipFileTest, GztellCountsBytesOnBothSides) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + const char* filename = "file.gz"; + remove(filename); + + gzFile fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gztell(fp), 0); + ASSERT_EQ(gzwrite(fp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + // The write side is wrong without this: the shim buffers rather than handing + // the bytes to zlib, so zlib's own position stays at 0 for the whole file. + EXPECT_EQ(gztell(fp), static_cast(payload.size())); + EXPECT_EQ(gztell64(fp), static_cast(payload.size())); + ASSERT_EQ(gzclose(fp), Z_OK); + + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gztell(fp), 0); + char buf[48]; + ASSERT_EQ(gzread(fp, buf, 32), 32); + EXPECT_EQ(gztell(fp), 32); + EXPECT_EQ(gztell64(fp), 32); + // A pushed-back byte is available again, so the position moves back with it. + ASSERT_EQ(gzungetc(buf[31], fp), static_cast(buf[31])); + EXPECT_EQ(gztell(fp), 31); + ASSERT_EQ(gzread(fp, buf, 1), 1); + EXPECT_EQ(gztell(fp), 32); + EXPECT_GT(gzoffset(fp), 0); + EXPECT_EQ(gzoffset(fp), gzoffset64(fp)); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +TEST_F(GzipFileTest, GzseekReadsFromTheOffsetItReports) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(6000); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + + char buf[17] = {0}; + + // Forward, absolute. This is the reported failure: without the fix gzseek + // returns 2560 and the read that follows comes back with byte 0 of the file. + EXPECT_EQ(gzseek(fp, 2560, SEEK_SET), 2560); + // gztell has to agree with what gzseek just promised, before any read has + // made the skip real. + EXPECT_EQ(gztell(fp), 2560); + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_STREQ(buf, "[off00002560]"); + EXPECT_EQ(gztell(fp), 2576); + + // Backwards, which has to rewind and skip forward again. + EXPECT_EQ(gzseek(fp, 1024, SEEK_SET), 1024); + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_STREQ(buf, "[off00001024]"); + + // Relative, from wherever that left us. + EXPECT_EQ(gzseek(fp, 496, SEEK_CUR), 1536); + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_STREQ(buf, "[off00001536]"); + + // Two seeks with no read between them: the second replaces the first rather + // than adding to it, and only the second is paid for. + EXPECT_EQ(gzseek(fp, 3200, SEEK_SET), 3200); + EXPECT_EQ(gzseek(fp, 4096, SEEK_SET), 4096); + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_STREQ(buf, "[off00004096]"); + + // zlib's refusals: an unsupported whence, and a target before the start of + // the file. + EXPECT_EQ(gzseek(fp, 0, SEEK_END), -1); + EXPECT_EQ(gzseek(fp, -1, SEEK_SET), -1); + + // A seek past the end lands at the end, and the read that follows is short + // rather than wrong. + EXPECT_EQ(gzseek(fp, static_cast(payload.size()) + 4096, SEEK_SET), + static_cast(payload.size()) + 4096); + EXPECT_EQ(gzread(fp, buf, 16), 0); + EXPECT_NE(gzeof(fp), 0); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +TEST_F(GzipFileTest, GzrewindStartsTheFileOver) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(2000); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + + // Read well past the shim's first refill so the rewind has real buffered + // state to discard, and leave a pushed-back byte for it to drop too. + std::vector block(20000, 0); + ASSERT_EQ(gzread(fp, block.data(), static_cast(block.size())), + static_cast(block.size())); + ASSERT_EQ(gzungetc('X', fp), 'X'); + + ASSERT_EQ(gzrewind(fp), 0); + EXPECT_EQ(gztell(fp), 0); + EXPECT_EQ(gzeof(fp), 0); + + char buf[17] = {0}; + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_STREQ(buf, "[off00000000]"); + + // And the whole file still reads correctly from there, so the rewind put the + // descriptor and the inflate stream back rather than just the counters. + std::string got(buf, 16); + std::vector rest(payload.size(), 0); + int rest_length = gzread(fp, rest.data(), static_cast(rest.size())); + ASSERT_GT(rest_length, 0); + got.append(rest.data(), static_cast(rest_length)); + EXPECT_EQ(got, payload); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +TEST_F(GzipFileTest, GzseekOnAWriteFileFillsTheGapWithZeros) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const char* filename = "file.gz"; + remove(filename); + gzFile fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + + ASSERT_EQ(gzwrite(fp, "head", 4), 4); + // zlib allows a forward seek while writing and writes zeros over the gap. + EXPECT_EQ(gzseek(fp, 12, SEEK_CUR), 16); + // The gap counts towards the position before anything has filled it, which is + // the whole point of gzseek returning the offset it promises. + EXPECT_EQ(gztell(fp), 16); + ASSERT_EQ(gzwrite(fp, "tail", 4), 4); + EXPECT_EQ(gztell(fp), 20); + + // Backwards is refused, there being nothing to go back to. Asserted after the + // gap has been filled, not before: measured in bare zlib, a refused seek + // abandons a skip that was still pending, so the sequence + // "seek +12, refused seek, write" produces an 8-byte file in zlib as well. + // Interesting, but it is zlib's behavior and not something to assert here. + EXPECT_EQ(gzseek(fp, 0, SEEK_SET), -1); + ASSERT_EQ(gzclose(fp), Z_OK); + + EnableShimOwnedGzReads(); + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + char buf[32] = {0}; + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), 20); + EXPECT_EQ(std::string(buf, 4), "head"); + EXPECT_EQ(std::string(buf + 4, 12), std::string(12, '\0')); + EXPECT_EQ(std::string(buf + 16, 4), "tail"); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// A gap left by a seek that was never written past still has to reach the file: +// zlib fills it at close, so the file is 16 bytes long, not 4. +TEST_F(GzipFileTest, GzseekOnAWriteFileIsPaidForAtClose) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const char* filename = "file.gz"; + remove(filename); + gzFile fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, "head", 4), 4); + EXPECT_EQ(gzseek(fp, 12, SEEK_CUR), 16); + ASSERT_EQ(gzclose(fp), Z_OK); + + EnableShimOwnedGzReads(); + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + char buf[32] = {0}; + EXPECT_EQ(gzread(fp, buf, sizeof(buf)), 16); + EXPECT_EQ(std::string(buf, 4), "head"); + EXPECT_EQ(std::string(buf + 4, 12), std::string(12, '\0')); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +TEST_F(GzipFileTest, GzerrorLatchesAndGzclearerrClearsIt) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const size_t input_length = 8192; + char* input = GenerateSeededCompressibleBlock(input_length, /*seed=*/0x11f7); + ASSERT_NE(input, nullptr); + ASSERT_EQ(ZlibCompressGzipFile(input, input_length), Z_OK); + + const char* filename = "file.gz"; + + // Corrupt the CRC32 in the gzip trailer: the 8th byte from the end, wherever + // the member was written and whatever it contains. + int fd = open(filename, O_RDWR); + ASSERT_NE(fd, -1); + off_t end = lseek(fd, 0, SEEK_END); + ASSERT_GT(end, 8); + unsigned char crc_byte = 0; + ASSERT_EQ(pread(fd, &crc_byte, 1, end - 8), 1); + crc_byte ^= 0xff; + ASSERT_EQ(pwrite(fd, &crc_byte, 1, end - 8), 1); + close(fd); + + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + + int errnum = Z_OK; + EXPECT_STREQ(gzerror(fp, &errnum), ""); + EXPECT_EQ(errnum, Z_OK); + + std::vector output(input_length + 512, 0); + int total = 0; + int ret = 0; + while ((ret = gzread(fp, output.data(), + static_cast(output.size()))) > 0) { + total += ret; + } + EXPECT_EQ(ret, -1); + + const char* message = gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_DATA_ERROR); + ASSERT_NE(message, nullptr); + // zlib formats the message as ": ". + EXPECT_NE(std::string(message).find(filename), std::string::npos); + + // The latch is sticky: a second read is refused without touching the file. + EXPECT_EQ(gzread(fp, output.data(), 16), -1); + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_DATA_ERROR); + + // And gzclearerr takes it back off, which is what makes the file readable + // again rather than permanently dead. + gzclearerr(fp); + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + EXPECT_EQ(gzeof(fp), 0); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); + DestroyBlock(input); +} + +TEST_F(GzipFileTest, GzclearerrClearsEndOfFile) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(4); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + + char buf[128] = {0}; + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), static_cast(payload.size())); + ASSERT_NE(gzeof(fp), 0); + gzclearerr(fp); + EXPECT_EQ(gzeof(fp), 0); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// gzbuffer takes a size only before any reading or writing, because that is +// when zlib would still be allocating. The shim has to answer from its own +// state: the open-time header look makes zlib allocate, so zlib's own gzbuffer +// would refuse even the first call. +TEST_F(GzipFileTest, GzbufferAcceptsOnlyBeforeAnyIo) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzbuffer(fp, 8192), 0); + // A size zlib could not double without overflowing is refused. + EXPECT_EQ(gzbuffer(fp, 0x80000000u), -1); + // Below 8 is raised to 8 by zlib, not rejected. + EXPECT_EQ(gzbuffer(fp, 1), 0); + + char buf[16]; + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), static_cast(sizeof(buf))); + EXPECT_EQ(gzbuffer(fp, 16384), -1); + EXPECT_EQ(gzclose(fp), Z_OK); + + remove(filename); + fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzbuffer(fp, 8192), 0); + ASSERT_EQ(gzwrite(fp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + EXPECT_EQ(gzbuffer(fp, 16384), -1); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// --------------------------------------------------------------------------- +// The open-time header look, and the descriptor ownership it settles. +// --------------------------------------------------------------------------- + +// A file that is not a gzip member at all. zlib reads it straight through; the +// shim's job is to notice at open and stay out of the way, because it has no +// copy-through path of its own and used to return -1 for the whole file. +TEST_F(GzipFileTest, GzopenReadsAPlainFileThroughZlib) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const char* filename = "file.gz"; + remove(filename); + const std::string plain = "not gzip at all, just text\n"; + FILE* raw = fopen(filename, "wb"); + ASSERT_NE(raw, nullptr); + ASSERT_EQ(fwrite(plain.data(), 1, plain.size(), raw), plain.size()); + ASSERT_EQ(fclose(raw), 0); + + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + char buf[64] = {0}; + EXPECT_EQ(gzread(fp, buf, sizeof(buf)), static_cast(plain.size())); + EXPECT_EQ(std::string(buf, plain.size()), plain); + EXPECT_NE(gzeof(fp), 0); + int errnum = Z_OK; + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// An empty .gz -- touched, truncated, or an interrupted write. zlib reads it as +// zero bytes at end of file; the shim used to return -1. +TEST_F(GzipFileTest, GzopenReadsAnEmptyFileAsEndOfFile) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const char* filename = "file.gz"; + remove(filename); + FILE* raw = fopen(filename, "wb"); + ASSERT_NE(raw, nullptr); + ASSERT_EQ(fclose(raw), 0); + + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + char buf[64] = {0}; + EXPECT_EQ(gzread(fp, buf, sizeof(buf)), 0); + EXPECT_NE(gzeof(fp), 0); + int errnum = Z_OK; + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// Bytes after a complete member that cannot begin another one. zlib ignores +// them and reports a clean end of file; the shim used to return -1 and lose the +// entire payload with it. +TEST_F(GzipFileTest, GzreadIgnoresATrailerThatIsNotAMember) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + FILE* raw = fopen(filename, "ab"); + ASSERT_NE(raw, nullptr); + const char junk[] = "THIS IS NOT A GZIP MEMBER"; + ASSERT_EQ(fwrite(junk, 1, sizeof(junk) - 1, raw), sizeof(junk) - 1); + ASSERT_EQ(fclose(raw), 0); + + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + std::vector output(payload.size() + 512, 0); + EXPECT_EQ(gzread(fp, output.data(), static_cast(output.size())), + static_cast(payload.size())); + EXPECT_EQ(std::string(output.data(), payload.size()), payload); + EXPECT_EQ(gzread(fp, output.data(), 16), 0); + EXPECT_NE(gzeof(fp), 0); + int errnum = Z_OK; + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// The invariant the header look depends on, and the one thing here that fails +// silently rather than loudly if it is wrong. +// +// The look rewinds the descriptor, which leaves zlib holding up to 8 KB of +// input read from a position the file is no longer at. Measured with plain zlib +// and no shim: asking zlib to read such a file returns 51,456 bytes of +// duplicates and then Z_DATA_ERROR. So a rewound file must never reach +// orig_gzread -- and the branch that would send it there is chosen per call, +// from the configuration, not per file. +// +// Turning every uncompress flag off mid-read is the config change that used to +// flip that branch. The file has to keep reading correctly through it: which +// engine decompresses may change, but not who reads the descriptor. +TEST_F(GzipFileTest, ConfigChangeMidReadDoesNotHandARewoundFileToZlib) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(6000); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + + std::string got; + std::vector buf(4096, 0); + ASSERT_EQ(gzread(fp, buf.data(), 4096), 4096); + got.append(buf.data(), 4096); + + // Every accelerator off, part way through. Without the fix the next read + // takes the orig_gzread branch and serves zlib's stale header bytes again. + SetUncompressPath(ZLIB, false, false); + + int ret = 0; + while ((ret = gzread(fp, buf.data(), static_cast(buf.size()))) > + 0) { + got.append(buf.data(), static_cast(ret)); + } + EXPECT_EQ(ret, 0); + int errnum = Z_OK; + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + EXPECT_EQ(got.size(), payload.size()); + EXPECT_EQ(got, payload); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// The same branch, reached with the configuration untouched. zlib parses a +// level digit out of the mode string even in read mode, so "rb0" yields level +// 0, which no backend can serve, and the file is pinned to zlib at open. It +// must never have been rewound in the first place -- and unlike the case above, +// nothing about the configuration is involved, so this one is reachable in +// production. +TEST_F(GzipFileTest, GzopenLevelZeroReadFileIsNeverRewound) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(6000); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb0"); + ASSERT_NE(fp, nullptr); + + std::string got; + std::vector buf(4096, 0); + int ret = 0; + while ((ret = gzread(fp, buf.data(), static_cast(buf.size()))) > + 0) { + got.append(buf.data(), static_cast(ret)); + } + EXPECT_EQ(ret, 0); + int errnum = Z_OK; + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + EXPECT_EQ(got.size(), payload.size()); + EXPECT_EQ(got, payload); + EXPECT_NE(gzeof(fp), 0); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// A multi-member file, which is what the trailing-trailer rule has to keep +// working: two members concatenated read as one stream, and the magic test that +// stops at a non-member trailer must not stop at a real second member -- even +// when the two bytes of its header land either side of a buffer boundary. +TEST_F(GzipFileTest, GzreadStillJoinsConcatenatedMembers) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string first = PositionStampedPayload(500); + const std::string second = PositionStampedPayload(700); + + const char* filename = "file.gz"; + remove(filename); + gzFile w = gzopen(filename, "wb"); + ASSERT_NE(w, nullptr); + ASSERT_EQ(gzwrite(w, first.data(), static_cast(first.size())), + static_cast(first.size())); + ASSERT_EQ(gzclose(w), Z_OK); + w = gzopen(filename, "ab"); + ASSERT_NE(w, nullptr); + ASSERT_EQ(gzwrite(w, second.data(), static_cast(second.size())), + static_cast(second.size())); + ASSERT_EQ(gzclose(w), Z_OK); + + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + std::vector output(first.size() + second.size() + 512, 0); + std::string got; + int ret = 0; + while ((ret = gzread(fp, output.data(), + static_cast(output.size()))) > 0) { + got.append(output.data(), static_cast(ret)); + } + EXPECT_EQ(ret, 0); + EXPECT_EQ(got, first + second); + EXPECT_NE(gzeof(fp), 0); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// A member that stops part way through. zlib returns the bytes it managed to +// inflate and latches Z_BUF_ERROR, which -- unlike a data error -- does not +// stop a later read. +TEST_F(GzipFileTest, GzreadLatchesBufErrorOnATruncatedMember) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const size_t input_length = 16 << 10; + char* input = GenerateSeededCompressibleBlock(input_length, /*seed=*/0x11fb); + ASSERT_NE(input, nullptr); + ASSERT_EQ(ZlibCompressGzipFile(input, input_length), Z_OK); + + const char* filename = "file.gz"; + int fd = open(filename, O_RDWR); + ASSERT_NE(fd, -1); + off_t end = lseek(fd, 0, SEEK_END); + ASSERT_GT(end, 64); + // Cut the member short, well inside the deflate body. + ASSERT_EQ(ftruncate(fd, end / 2), 0); + close(fd); + + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + std::vector output(input_length + 512, 0); + int total = 0; + int ret = 0; + while ((ret = gzread(fp, output.data(), + static_cast(output.size()))) > 0) { + total += ret; + } + // Some of it came back, and the stream ending early is recorded rather than + // reported as a clean end of file. + EXPECT_GT(total, 0); + EXPECT_EQ(ret, 0); + int errnum = Z_OK; + const char* message = gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_BUF_ERROR); + ASSERT_NE(message, nullptr); + EXPECT_NE(std::string(message).find(filename), std::string::npos); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); + DestroyBlock(input); +} + class ShardedMapTest : public ::testing::Test {}; TEST_F(ShardedMapTest, BasicSetAndGet) { diff --git a/zlib_accel.cpp b/zlib_accel.cpp index e4aea5e..33666bf 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -1,6 +1,15 @@ // Copyright (C) 2025 Intel Corporation // SPDX-License-Identifier: Apache-2.0 +// libz exports gzseek64, gztell64 and gzoffset64 alongside their plain-named +// counterparts, and an application built with -D_FILE_OFFSET_BITS=64 calls the +// 64-bit names, so the shim has to define both sets or the pair would disagree +// about where a file is positioned. zlib.h only declares them when asked +// (zconf.h:506), and this is the ask. Note it is _LARGEFILE64_SOURCE, not +// _FILE_OFFSET_BITS: the latter would additionally rename gzseek to gzseek64 +// here and leave the plain names undefined. +#define _LARGEFILE64_SOURCE 1 + #include "zlib_accel.h" #include @@ -99,6 +108,19 @@ static int (*orig_gzungetc)(int c, gzFile file); static char* (*orig_gzgets)(gzFile file, char* buf, int len); static z_size_t (*orig_gzfread)(voidp buf, z_size_t size, z_size_t nitems, gzFile file); +// Only ever called once per read-mode open, to let zlib decide whether the file +// is a gzip member at all. See GzLookAtOpen. +static int (*orig_gzdirect)(gzFile file); +static const char* (*orig_gzerror)(gzFile file, int* errnum); +static void (*orig_gzclearerr)(gzFile file); +static z_off_t (*orig_gztell)(gzFile file); +static z_off64_t (*orig_gztell64)(gzFile file); +static z_off_t (*orig_gzoffset)(gzFile file); +static z_off64_t (*orig_gzoffset64)(gzFile file); +static z_off_t (*orig_gzseek)(gzFile file, z_off_t offset, int whence); +static z_off64_t (*orig_gzseek64)(gzFile file, z_off64_t offset, int whence); +static int (*orig_gzrewind)(gzFile file); +static int (*orig_gzbuffer)(gzFile file, unsigned size); // Forward declaration — defined after DeflateStreamSettings, // InflateStreamSettings, and GzipFiles class definitions below @@ -234,6 +256,27 @@ static int init_zlib_accel(void) { LOAD_SYMBOL(orig_gzfread, z_size_t(*)(voidp, z_size_t, z_size_t, gzFile), "gzfread"); + LOAD_SYMBOL(orig_gzdirect, int (*)(gzFile), "gzdirect"); + + LOAD_SYMBOL(orig_gzerror, const char* (*)(gzFile, int*), "gzerror"); + + LOAD_SYMBOL(orig_gzclearerr, void (*)(gzFile), "gzclearerr"); + + LOAD_SYMBOL(orig_gztell, z_off_t(*)(gzFile), "gztell"); + + LOAD_SYMBOL(orig_gztell64, z_off64_t(*)(gzFile), "gztell64"); + + LOAD_SYMBOL(orig_gzoffset, z_off_t(*)(gzFile), "gzoffset"); + + LOAD_SYMBOL(orig_gzoffset64, z_off64_t(*)(gzFile), "gzoffset64"); + + LOAD_SYMBOL(orig_gzseek, z_off_t(*)(gzFile, z_off_t, int), "gzseek"); + + LOAD_SYMBOL(orig_gzseek64, z_off64_t(*)(gzFile, z_off64_t, int), "gzseek64"); + + LOAD_SYMBOL(orig_gzrewind, int (*)(gzFile), "gzrewind"); + LOAD_SYMBOL(orig_gzbuffer, int (*)(gzFile, unsigned), "gzbuffer"); + if (missing_symbol_count > 0) { Log(LogLevel::LOG_ERROR, "init_zlib_accel Line ", __LINE__, " ", missing_symbol_count, @@ -2079,16 +2122,95 @@ struct GzipFile { const int alloc_size = 512 << 10; + // Where the file was positioned when it was opened, and so what gzrewind + // seeks back to. zlib records the same thing in state->start (gzlib.c): 0 for + // gzopen, wherever the descriptor already was for gzdopen. -1 means the + // descriptor is not seekable, which is how gzseek and gzrewind know to + // refuse. + off_t start = 0; + // Uncompressed bytes handed to the application so far, which is what gztell + // reports. zlib's state->x.pos. gzungetc decrements it: the byte is available + // again, so the position has moved back. + z_off64_t pos = 0; + // A gzseek that has been promised but not yet performed. zlib's seek is lazy + // (gzlib.c): it records the distance, returns the position it will be at, and + // the next read skips the bytes. gztell has to add this in or it would + // contradict the value gzseek just returned. + z_off64_t pending_skip = 0; + // The latched error, and the message gzerror reports with it. zlib keeps the + // message as ": " and never clears err until gzclearerr or a + // rewind, so a failed read stays failed for every later call. + int err = Z_OK; + std::string msg; + // The name to put in that message: the path for gzopen, "" for gzdopen, + // which is what zlib synthesizes (gzlib.c). Not called `path` -- that name is + // taken by the ExecutionPath above. + std::string file_name; + // Set when GzLookAtOpen rewound the descriptor after letting zlib read the + // header. From that point zlib's own buffered input describes a position the + // descriptor is no longer at, so handing the file to orig_gzread would serve + // those bytes a second time and then fail. Reads stay with the shim for the + // life of the file; only the choice of decompressor may still change. + bool shim_owns_reads = false; + // The mirror image: this file was handed to zlib at open and every call on it + // has been forwarded since, so zlib's own position and error state are the + // complete and correct ones and the position entry points below just + // delegate. Decided once, at open, for the same reason shim_owns_reads is: + // `path` can still turn into ZLIB later, part way through a file the shim + // already has bytes of, and zlib's position would then be missing that part. + bool zlib_owns_file = false; + // Set by the first read or write on this file, whichever path it took. This + // is the shim's answer to zlib's "have I already allocated my buffers" test, + // which is what gzbuffer refuses on: zlib allocates on its own first read or + // write, so the two flip at the same moment. + bool io_started = false; + // True when the next byte in io_buf begins a gzip member rather than + // continuing one. Only then does a magic-number test mean anything: + // mid-member the bytes are deflate output and will not look like a header. + bool at_member_boundary = true; + // Stream to use zlib in case of accelerator errors z_stream deflate_stream; z_stream inflate_stream; }; +// Latch an error on a file the shim owns, the way zlib's gz_error does: keep +// the code, and build the message zlib would have built. Z_MEM_ERROR is the one +// case zlib does not allocate a message for, because allocating is what just +// failed. +static void GzSetError(GzipFile* gz, int err, const char* text) { + gz->err = err; + if (err == Z_OK || err == Z_MEM_ERROR) { + gz->msg.clear(); + return; + } + try { + gz->msg = gz->file_name + ": " + (text != nullptr ? text : ""); + } catch (...) { + // Out of memory while reporting an error is not worth a second error; the + // code is the part callers branch on. + gz->msg.clear(); + } +} + +// zlib refuses a read outright once a serious error is latched, but treats +// Z_BUF_ERROR as recoverable -- it means "the input ended sooner than the +// stream said", and a caller may legitimately keep going. The write side has no +// such tolerance. Both asymmetries are zlib's (gzread.c, gzwrite.c). +static bool GzReadableAfterError(const GzipFile* gz) { + return gz->err == Z_OK || gz->err == Z_BUF_ERROR; +} + class GzipFiles { public: - void Set(gzFile file, int fd, const GzOpenParams& params) { + // Returns the entry it just created, so the caller can finish initializing it + // (the file name, and the open-time header look) without a second lookup. + std::shared_ptr Set(gzFile file, int fd, + const GzOpenParams& params) { auto f = std::make_shared(fd, params); + auto created = f; map.Set(file, std::move(f)); + return created; } void Unset(gzFile file) { map.Unset(file); } @@ -2108,6 +2230,68 @@ static void InitStreamRegistries() { gzip_files.Init(); } +// Ask zlib, once per read-mode open, whether this file is a gzip member at all. +// +// The shim deliberately has no magic-number test of its own: two +// implementations of "is this a gzip header" would drift, and zlib already has +// one in gz_look(). gzdirect() is the public way to reach it. That call is +// guarded inside zlib by how == LOOK && x.have == 0, so it reads at most once +// for the life of the file +// -- which is what makes this affordable, and is also why gzdirect itself needs +// no interception: after this call zlib answers from state it already has, so +// every later gzdirect the application makes is truthful and costs nothing. +// +// A file that is not a gzip member is not the shim's business. Hand it to zlib +// and stay out of the way: zlib has already buffered the bytes and switched +// itself to copy-through, so it reads the file correctly with no help. The same +// applies to an empty file and to a file too short to hold a header. +static void GzLookAtOpen(gzFile file, GzipFile* gz) { + if (gz->mode != FileMode::READ || orig_gzdirect == nullptr) { + return; + } + + // Where the file is now is both zlib's state->start and the position to put + // the descriptor back to afterwards. A descriptor that cannot be seeked + // cannot be put back, so it cannot be looked at either: doing the look anyway + // would leave the 8 KB zlib read stranded in zlib's buffer, unreachable by + // the shim. Skip it and keep the existing behaviour rather than quietly + // moving every pipe onto zlib and taking its acceleration away. + gz->start = lseek(gz->fd, 0, SEEK_CUR); + if (gz->start == static_cast(-1)) { + return; + } + + if (orig_gzdirect(file) != 0) { + // Not a gzip member. zlib owns it from here. + gz->path = ZLIB; + return; + } + + // A real gzip member, but only rewind if the shim is actually going to read + // it. Two ways it will not: no uncompress accelerator is configured, or the + // mode string already pinned the file to zlib (a level digit no backend can + // serve, which zlib parses in read mode too). In both cases zlib reads the + // file, and it must keep the bytes it has just buffered. + const bool accelerator_selected = configs[USE_IAA_UNCOMPRESS] || + configs[USE_QAT_UNCOMPRESS] || + configs[USE_IGZIP_UNCOMPRESS]; + if (!accelerator_selected || gz->path == ZLIB) { + gz->path = ZLIB; + return; + } + + // The shim reads it, so the header bytes zlib consumed have to come back. + // zlib's copy of them is now stale, and serving them again on top of a + // rewound descriptor would duplicate that much of the file and then fail the + // checksum + // -- so from here the descriptor belongs to the shim alone. + if (lseek(gz->fd, gz->start, SEEK_SET) == static_cast(-1)) { + gz->path = ZLIB; + return; + } + gz->shim_owns_reads = true; +} + // Inspired by gz_open in gzlib.c int GetOpenFlags(const char* mode, GzOpenParams* params) { bool cloexec = false; @@ -2207,7 +2391,20 @@ gzFile ZEXPORT gzopen(const char* path, const char* mode) { Log(LogLevel::LOG_INFO, "gzopen Line ", __LINE__, ", file ", static_cast(file), ", path ", path, ", mode ", mode, "\n"); - gzip_files.Set(file, fd, params); + auto gz = gzip_files.Set(file, fd, params); + if (gz != nullptr) { + try { + gz->file_name = path != nullptr ? path : ""; + } catch (...) { + // Only the text of a later gzerror message is lost. + } + GzLookAtOpen(file, gz.get()); + // Whatever pinned it -- the header look above, or a mode string the + // constructor found unoffloadable -- a file already on the zlib path at + // open never has a byte of it pass through the shim, so zlib's own position + // and error state stay authoritative for it. + gz->zlib_owns_file = gz->path == ZLIB; + } return file; } @@ -2230,7 +2427,22 @@ gzFile ZEXPORT gzdopen(int fd, const char* mode) { GzOpenParams params; GetOpenFlags(mode, ¶ms); - gzip_files.Set(file, fd, params); + auto gz = gzip_files.Set(file, fd, params); + if (gz != nullptr) { + // The name zlib synthesizes for a descriptor it was handed, so a gzerror + // message on this file reads the same as zlib's would. + try { + gz->file_name = ""; + } catch (...) { + // Only the text of a later gzerror message is lost. + } + GzLookAtOpen(file, gz.get()); + // Whatever pinned it -- the header look above, or a mode string the + // constructor found unoffloadable -- a file already on the zlib path at + // open never has a byte of it pass through the shim, so zlib's own position + // and error state stay authoritative for it. + gz->zlib_owns_file = gz->path == ZLIB; + } return file; } @@ -2513,6 +2725,29 @@ static int FlushBufferedWrite(gzFile file, GzipFile* gz) { return 0; } +// A forward gzseek on a write-mode file is allowed by zlib, which fills the gap +// with zeros (gz_zero, gzwrite.c:225). zlib defers the fill until the next +// write, flush or close; doing it as soon as the gap is known produces the same +// bytes in the same order, and keeps gztell answerable from pos alone. +static int GzWriteZeros(gzFile file, GzipFile* gz) { + z_off64_t left = gz->pending_skip; + // Cleared before the writes, not after: gzwrite consumes pending_skip itself, + // so leaving it set would recurse here forever. + gz->pending_skip = 0; + char zeros[4096]; + memset(zeros, 0, sizeof(zeros)); + while (left > 0) { + unsigned n = left > static_cast(sizeof(zeros)) + ? static_cast(sizeof(zeros)) + : static_cast(left); + if (gzwrite(file, zeros, n) != static_cast(n)) { + return -1; + } + left -= n; + } + return 0; +} + int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { auto gz = gzip_files.Get(file); if (gz == nullptr) { @@ -2527,6 +2762,22 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { orig_deflateReset == nullptr || orig_deflateInit2_ == nullptr) { Log(LogLevel::LOG_ERROR, "gzwrite Line ", __LINE__, " a required zlib symbol is unresolved, cannot write\n"); + GzSetError(gz.get(), Z_STREAM_ERROR, "required zlib symbol is unresolved"); + return 0; + } + + // The write side demands a clean latch, where the read side tolerates + // Z_BUF_ERROR (gz_write, gzwrite.c:249). + if (gz->err != Z_OK) { + return 0; + } + + // Past every refusal above, so this write is going to happen: from here on + // gzbuffer is too late, exactly as it is in zlib. + gz->io_started = true; + + // Pay off a forward seek before adding anything after it. + if (gz->pending_skip > 0 && GzWriteZeros(file, gz.get()) != 0) { return 0; } @@ -2544,6 +2795,8 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { // is refused by zlib itself instead, which also latches the error gzerror // reports. if (len > static_cast(INT_MAX)) { + GzSetError(gz.get(), Z_DATA_ERROR, + "requested length does not fit in int"); return 0; } @@ -2568,7 +2821,17 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { // Compress and write the buffer if (written_bytes < len) { - if (FlushBufferedWrite(file, gz.get()) != 0) { + int flush_ret = FlushBufferedWrite(file, gz.get()); + if (flush_ret != 0) { + // Z_STREAM_ERROR means a symbol was missing and nothing was + // attempted; anything else came from the write itself, so errno + // describes it. + if (flush_ret == Z_STREAM_ERROR) { + GzSetError(gz.get(), Z_STREAM_ERROR, + "required zlib symbol is unresolved"); + } else { + GzSetError(gz.get(), Z_ERRNO, strerror(errno)); + } written_bytes = 0; goto gzwrite_end; } @@ -2589,6 +2852,11 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { } gzwrite_end: + // Both branches land here, so this counts what zlib wrote on our behalf as + // well as what the accelerator buffered. gztell needs the total, not the part + // either side happens to know about. + gz->pos += written_bytes; + Log(LogLevel::LOG_INFO, "gzwrite Line ", __LINE__, ", file ", static_cast(file), ", written ", written_bytes, ", buffered ", gz->data_buf_pos, ", path ", static_cast(gz->path), "\n"); @@ -2681,14 +2949,30 @@ int ZEXPORT gzflush(gzFile file, int flush) { static_cast(file), ", flush ", flush, "\n"); auto gz = gzip_files.Get(file); - if (gz == nullptr || gz->path == ZLIB) { + if (gz == nullptr) { + return orig_gzflush != nullptr ? orig_gzflush(file, flush) : Z_STREAM_ERROR; + } + + // A gap left by a forward seek has to be filled before the flush, or it would + // land after the data that follows it -- and it has to happen on this side of + // the delegation below, because the skip is in the shim's state and zlib's + // own flush knows nothing about it. Same reasoning as in GzCloseCommon. + if (gz->pending_skip > 0 && GzIsWriteMode(gz->mode) && gz->err == Z_OK && + GzWriteZeros(file, gz.get()) != 0) { + return gz->err; + } + + if (gz->path == ZLIB) { return orig_gzflush != nullptr ? orig_gzflush(file, flush) : Z_STREAM_ERROR; } // zlib's own checks, in zlib's order: a write-mode file and a flush value in // range. Z_NO_FLUSH is in range and asks only that pending input be // compressed, which is what the flush below does. - if (!GzIsWriteMode(gz->mode) || flush < 0 || flush > Z_FINISH) { + // A latched error is Z_STREAM_ERROR here, not the latched code itself + // (gzflush, gzwrite.c:562). + if (!GzIsWriteMode(gz->mode) || gz->err != Z_OK || flush < 0 || + flush > Z_FINISH) { return Z_STREAM_ERROR; } @@ -2696,9 +2980,11 @@ int ZEXPORT gzflush(gzFile file, int flush) { // write; a flush that could not run at all reports itself. int flush_ret = FlushBufferedWrite(file, gz.get()); if (flush_ret == Z_STREAM_ERROR) { + GzSetError(gz.get(), Z_STREAM_ERROR, "required zlib symbol is unresolved"); return Z_STREAM_ERROR; } if (flush_ret != 0) { + GzSetError(gz.get(), Z_ERRNO, strerror(errno)); return Z_ERRNO; } return Z_OK; @@ -2835,18 +3121,74 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { orig_inflateReset == nullptr || orig_inflateInit2_ == nullptr) { Log(LogLevel::LOG_ERROR, "gzread Line ", __LINE__, " a required zlib symbol is unresolved, cannot read\n"); + GzSetError(gz, Z_STREAM_ERROR, "a required zlib symbol is unresolved"); + return -1; + } + + // A latched error stops every later read, as it does in zlib -- except + // Z_BUF_ERROR, which only says the input ended sooner than the stream claimed + // and which zlib lets a caller read past. + if (!GzReadableAfterError(gz)) { return -1; } Log(LogLevel::LOG_INFO, "gzread Line ", __LINE__, ", file ", static_cast(file), ", buf ", buf, ", len ", len, "\n"); + // Past every refusal above, so this read is going to happen: from here on + // gzbuffer is too late, exactly as it is in zlib. + gz->io_started = true; + + // A gzseek that has been promised but not performed: the bytes it moved over + // have to be read and discarded before this read can be served. zlib does + // this at the same moment, for the same reason -- the skip cannot happen at + // gzseek time because the file is compressed and the target offset is not + // known until the data has been inflated. + if (gz->pending_skip > 0) { + z_off64_t to_skip = gz->pending_skip; + gz->pending_skip = 0; + // Bytes gzungetc pushed back are ahead of the file, so a forward seek + // passes over those first. zlib does the same, out of its output buffer, + // which is where its own ungetc leaves them (gzseek64, gzlib.c:404). + while (to_skip > 0 && !gz->pushback.empty()) { + gz->pushback.pop_back(); + gz->pos++; + to_skip--; + } + char scratch[4096]; + while (to_skip > 0) { + const unsigned chunk = to_skip > static_cast(sizeof(scratch)) + ? static_cast(sizeof(scratch)) + : static_cast(to_skip); + const int skipped = GzreadOwnedFile(file, gz, scratch, chunk); + if (skipped <= 0) { + // End of file, or an error that is now latched. Either way the skip + // cannot finish, and the read below reports whatever state that left. + break; + } + to_skip -= skipped; + } + if (!GzReadableAfterError(gz)) { + return -1; + } + } + int ret = 1; uint32_t read_bytes = 0; bool accelerator_selected = configs[USE_IAA_UNCOMPRESS] || configs[USE_QAT_UNCOMPRESS] || configs[USE_IGZIP_UNCOMPRESS]; - if (gz->path != ZLIB && accelerator_selected) { + // shim_owns_reads is checked first and on its own: the descriptor was rewound + // after the open-time header look, so zlib's buffered copy of the header + // describes a position the file is no longer at. Handing this file to + // orig_gzread would serve those bytes twice and then fail the checksum. The + // configuration may still change which decompressor runs -- the fallback + // below uses the shim's own inflate stream -- but never who reads the + // descriptor. + if (gz->shim_owns_reads || (gz->path != ZLIB && accelerator_selected)) { + if (!accelerator_selected) { + gz->use_zlib_for_decompression = true; + } // zlib's own limit, as in gzwrite: the length has to be representable in // the int this returns, and a file on the zlib path is refused by zlib. if (len > static_cast(INT_MAX)) { @@ -2903,10 +3245,36 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { // probably won't work either // TODO if this is the first call to gzread we could try to call // orig_gzread + GzSetError(gz, Z_ERRNO, strerror(errno)); read_bytes = -1; goto gzread_end; } + // At a member boundary the next bytes either start another gzip + // member or are not gzip data at all. zlib ignores a trailer it + // cannot read as a header and reports a clean end of file (the direct + // == 0 branch of gz_look), so a complete file followed by junk must + // not become a read error that throws the whole payload away. The + // test is only meaningful at a boundary: mid-member these bytes are + // deflate output and will not look like a header. + const uint32_t unread = gz->io_buf_content - gz->io_buf_pos; + if (gz->at_member_boundary && unread > 0) { + const unsigned char* next = reinterpret_cast( + gz->io_buf + gz->io_buf_pos); + // Fewer than two bytes is only conclusive once the file has ended; + // otherwise a header could still be split across a refill, which a + // genuine multi-member file does. + const bool conclusive = unread >= 2 || gz->reached_eof; + const bool starts_a_member = + unread >= 2 && next[0] == 0x1f && next[1] == 0x8b; + if (conclusive && !starts_a_member) { + gz->reached_eof = true; + gz->io_buf_content = 0; + gz->io_buf_pos = 0; + continue; + } + } + // Decompress content of io_buf into data_buf uint32_t input_len = gz->io_buf_content; uint8_t* input = reinterpret_cast(gz->io_buf); @@ -2930,6 +3298,9 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { } else { gz->io_buf_pos += input_len; gz->data_buf_content += output_len; + // The accelerator only reports success when it consumed a whole + // member, so this always lands on a boundary. + gz->at_member_boundary = true; } } @@ -2951,10 +3322,28 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { (gz->io_buf_content - gz->inflate_stream.avail_in); gz->data_buf_content += (gz->data_buf_size - gz->inflate_stream.avail_out); + // Z_STREAM_END means the member finished, so whatever follows is + // either a new member or a trailer. Z_OK means it did not, so the + // next bytes continue this one and must not be header-tested. + gz->at_member_boundary = (ret == Z_STREAM_END); if (ret == Z_STREAM_END) { orig_inflateReset(&gz->inflate_stream); } } else if (ret != Z_OK) { + // The codes and texts zlib's own gz_decomp reports, so a caller + // that prints gzerror sees what it would have seen without the + // shim. Z_MEM_ERROR is the one case with no allocated message. + if (ret == Z_MEM_ERROR) { + GzSetError(gz, Z_MEM_ERROR, nullptr); + } else if (ret == Z_DATA_ERROR || ret == Z_NEED_DICT) { + GzSetError(gz, Z_DATA_ERROR, + gz->inflate_stream.msg != nullptr + ? gz->inflate_stream.msg + : "compressed data error"); + } else { + GzSetError(gz, Z_STREAM_ERROR, + "internal error: inflate failed"); + } read_bytes = -1; goto gzread_end; } @@ -2972,6 +3361,15 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { // indicator gzeof reports. A read error takes the goto above instead // and does not set it, matching zlib, which records that as an error // rather than as end of file. + // + // Ending part-way through a member is a different matter: the stream + // said more was coming and the file stopped. zlib latches that as + // Z_BUF_ERROR "unexpected end of file" while still returning the + // bytes it did manage to inflate, which is why Z_BUF_ERROR does not + // stop a later read. + if (!gz->at_member_boundary && gz->err == Z_OK) { + GzSetError(gz, Z_BUF_ERROR, "unexpected end of file"); + } gz->read_past_end = true; more_data = false; } @@ -2983,6 +3381,11 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { } gzread_end: + // Every byte this returns has been handed to the application, so it counts + // towards the position gztell reports. + if (static_cast(read_bytes) > 0) { + gz->pos += static_cast(read_bytes); + } Log(LogLevel::LOG_INFO, "gzread Line ", __LINE__, ", file ", static_cast(file), ", return code ", ret, ", read ", read_bytes, ", buffered compressed ", gz->io_buf_content, ", buffered uncompressed ", @@ -3000,6 +3403,7 @@ static int GzGetcOwned(gzFile file, GzipFile* gz) { if (!gz->pushback.empty()) { const unsigned char pushed = gz->pushback.back(); gz->pushback.pop_back(); + gz->pos++; return static_cast(pushed); } unsigned char ch = 0; @@ -3012,6 +3416,12 @@ int ZEXPORT gzread(gzFile file, voidp buf, unsigned len) { return orig_gzread != nullptr ? orig_gzread(file, buf, len) : -1; } + // A latched error refuses the read before anything is served, including bytes + // waiting in push-back, which is the order zlib checks in. + if (!GzReadableAfterError(gz.get())) { + return -1; + } + // Bytes pushed back by gzungetc are the first thing the next read returns, // most recent first; the rest of the request comes from the file as usual. // @@ -3026,6 +3436,7 @@ int ZEXPORT gzread(gzFile file, voidp buf, unsigned len) { static_cast(gz->pushback.back()); gz->pushback.pop_back(); } + gz->pos += served; if (served == len) { return static_cast(served); } @@ -3088,6 +3499,9 @@ int ZEXPORT gzungetc(int c, gzFile file) { return -1; } gz->read_past_end = false; + // There is a byte to read again, so the position moves back with it -- zlib + // decrements x.pos here for the same reason. + gz->pos--; return static_cast(ch); } @@ -3199,6 +3613,20 @@ static int GzCloseCommon(gzFile file, FileMode required_mode, return Z_STREAM_ERROR; } + // A forward seek the application never wrote past still has to reach the + // file: zlib fills the gap at close too (gzclose_w, gzwrite.c:640). Measured + // in bare zlib, no shim: gzwrite "head", gzseek +12, gzclose gives a 16-byte + // file. + // + // Not conditioned on the path. The skip was recorded in the shim's own state, + // by the shim's own gzseek, so zlib does not know about it and will not fill + // anything -- whichever engine ends up compressing the zeros. And this has to + // run before the Unset below, because it writes through gzwrite, which would + // otherwise no longer recognize the file and hand it to zlib. + if (gz->pending_skip > 0 && GzIsWriteMode(gz->mode)) { + GzWriteZeros(file, gz.get()); + } + // Unregister up front, before orig_close frees the gzFile. Unsetting after // the free would erase the entry of whatever file has since been allocated at // the same address, so do it once here rather than on each exit path. Holding @@ -3295,6 +3723,290 @@ int ZEXPORT gzeof(gzFile file) { return gz->read_past_end; } +// --------------------------------------------------------------------------- +// Position and error state. +// +// None of the functions below moves any compressed bytes, but every one of them +// reports on work the shim did, and zlib's answer describes only the part of +// that work zlib itself performed -- on a file the shim owns, that is either +// nothing at all or the wrong half. gzseek and gzrewind are worse than +// inaccurate: they report success and leave the next read returning data from +// the wrong offset. +// +// zlib_owns_file is the dividing line, and it is settled at open. A file that +// went to zlib there has every call on it forwarded, so zlib's own state is the +// complete and correct one and these entry points delegate untouched. +// --------------------------------------------------------------------------- + +// zlib's gzrewind is gz_reset plus an lseek back to where the file was opened +// (gzlib.c:361), so this has to put back the same set of things: the +// descriptor, both buffers, the position, the error latch and the inflate +// stream. +// +// Deliberately not GzipFile::Reset(). That is a constructor helper: it memsets +// deflate_stream and inflate_stream before re-initializing them, so calling it +// on a live file leaks both of the streams it is standing on. +static int GzRewindOwned(GzipFile* gz) { + if (gz->mode != FileMode::READ || !GzReadableAfterError(gz)) { + return -1; + } + // A pipe cannot be rewound, and GzLookAtOpen leaves start at -1 to say so. + if (gz->start == static_cast(-1)) { + return -1; + } + if (lseek(gz->fd, gz->start, SEEK_SET) == static_cast(-1)) { + GzSetError(gz, Z_ERRNO, strerror(errno)); + return -1; + } + + gz->data_buf_pos = 0; + gz->data_buf_content = 0; + gz->io_buf_pos = 0; + gz->io_buf_content = 0; + gz->pushback.clear(); + gz->pos = 0; + gz->pending_skip = 0; + gz->reached_eof = false; + gz->read_past_end = false; + // Back at the start of the file is back at a member boundary. It also gives + // the accelerator another chance at a stream that a mid-file fallback had + // taken away from it -- there is nothing left of that stream to be consistent + // with. + gz->at_member_boundary = true; + gz->use_zlib_for_decompression = false; + // Clearing the latch is zlib's behavior, not an embellishment: gz_reset calls + // gz_error(state, Z_OK, NULL), so a rewind is the other way besides + // gzclearerr to make a failed file readable again. + GzSetError(gz, Z_OK, nullptr); + if (orig_inflateReset != nullptr) { + orig_inflateReset(&gz->inflate_stream); + } + return 0; +} + +int ZEXPORT gzrewind(gzFile file) { + Log(LogLevel::LOG_INFO, "gzrewind Line ", __LINE__, ", file ", + static_cast(file), "\n"); + + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gzrewind != nullptr ? orig_gzrewind(file) : -1; + } + return GzRewindOwned(gz.get()); +} + +// zlib's seek is lazy (gzseek64, gzlib.c:411): it records the distance, returns +// the position it is going to be at, and lets the next read or write pay for +// it. Returning the promised position is the whole reason gztell has to add +// pending_skip in -- otherwise gztell would contradict the value gzseek just +// handed back. +static z_off64_t GzSeekOwned(GzipFile* gz, z_off64_t offset, int whence) { + // zlib's refusals, in zlib's order. + if (gz->mode == FileMode::NONE || !GzReadableAfterError(gz)) { + return -1; + } + if (whence != SEEK_SET && whence != SEEK_CUR) { + return -1; + } + + // Normalize to a distance from here, absorbing a seek that was promised but + // never performed. + if (whence == SEEK_SET) { + offset -= gz->pos; + } else { + offset += gz->pending_skip; + } + gz->pending_skip = 0; + + if (offset < 0) { + // Backwards. Only a reader can go there, and only by starting over: the + // shim has no index of the compressed stream, exactly as zlib has none. + if (gz->mode != FileMode::READ) { + return -1; + } + offset += gz->pos; + if (offset < 0) { + return -1; + } + if (GzRewindOwned(gz) != 0) { + return -1; + } + } + + if (offset > 0) { + gz->pending_skip = offset; + } + return gz->pos + offset; +} + +z_off64_t ZEXPORT gzseek64(gzFile file, z_off64_t offset, int whence) { + Log(LogLevel::LOG_INFO, "gzseek64 Line ", __LINE__, ", file ", + static_cast(file), ", offset ", offset, ", whence ", whence, "\n"); + + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gzseek64 != nullptr ? orig_gzseek64(file, offset, whence) : -1; + } + return GzSeekOwned(gz.get(), offset, whence); +} + +z_off_t ZEXPORT gzseek(gzFile file, z_off_t offset, int whence) { + Log(LogLevel::LOG_INFO, "gzseek Line ", __LINE__, ", file ", + static_cast(file), ", offset ", offset, ", whence ", whence, "\n"); + + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gzseek != nullptr ? orig_gzseek(file, offset, whence) : -1; + } + // zlib's gzseek is gzseek64 with the result narrowed the same way (gzlib.c). + return static_cast(GzSeekOwned(gz.get(), offset, whence)); +} + +z_off64_t ZEXPORT gztell64(gzFile file) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gztell64 != nullptr ? orig_gztell64(file) : -1; + } + if (gz->mode == FileMode::NONE) { + return -1; + } + return gz->pos + gz->pending_skip; +} + +z_off_t ZEXPORT gztell(gzFile file) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gztell != nullptr ? orig_gztell(file) : -1; + } + if (gz->mode == FileMode::NONE) { + return -1; + } + return static_cast(gz->pos + gz->pending_skip); +} + +// Where the *compressed* file is positioned, which zlib reports as the +// descriptor offset less the input it has read but not consumed (gzoffset64, +// gzlib.c:435). The shim's equivalent of that unconsumed input is whatever is +// left in io_buf. Only a reader has any: on the write side io_buf holds output +// already written, so the descriptor offset is the answer on its own. +static z_off64_t GzOffsetOwned(const GzipFile* gz) { + if (gz->mode == FileMode::NONE) { + return -1; + } + z_off64_t offset = lseek(gz->fd, 0, SEEK_CUR); + if (offset == static_cast(-1)) { + return -1; + } + if (gz->mode == FileMode::READ) { + offset -= gz->io_buf_content - gz->io_buf_pos; + } + return offset; +} + +z_off64_t ZEXPORT gzoffset64(gzFile file) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gzoffset64 != nullptr ? orig_gzoffset64(file) : -1; + } + return GzOffsetOwned(gz.get()); +} + +z_off_t ZEXPORT gzoffset(gzFile file) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gzoffset != nullptr ? orig_gzoffset(file) : -1; + } + return static_cast(GzOffsetOwned(gz.get())); +} + +const char* ZEXPORT gzerror(gzFile file, int* errnum) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + return orig_gzerror != nullptr ? orig_gzerror(file, errnum) : nullptr; + } + if (errnum != nullptr) { + *errnum = gz->err; + } + // zlib keeps no message for an allocation failure -- there was no memory to + // keep one in -- and answers with a literal instead (gzerror, gzlib.c:604). + if (gz->err == Z_MEM_ERROR) { + return "out of memory"; + } + return gz->msg.empty() ? "" : gz->msg.c_str(); +} + +void ZEXPORT gzclearerr(gzFile file) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || gz->zlib_owns_file) { + if (orig_gzclearerr != nullptr) { + orig_gzclearerr(file); + } + return; + } + // zlib clears the two end-of-file flags for a reader only, and the error + // latch either way (gzclearerr, gzlib.c:615). + if (gz->mode == FileMode::READ) { + gz->reached_eof = false; + gz->read_past_end = false; + } + GzSetError(gz.get(), Z_OK, nullptr); + // zlib's own latch belongs to the same file and is what an unintercepted + // helper would consult, so clear that too rather than leave the two + // disagreeing. + if (orig_gzclearerr != nullptr) { + orig_gzclearerr(file); + } +} + +// zlib's gzbuffer accepts a size only before any reading or writing has begun, +// because that is when it would still be allocating (gzbuffer, gzlib.c:299: +// "make sure we haven't already allocated memory"). Without interception the +// shim gets that backwards twice over: +// +// - before the open-time header look existed, zlib never allocated at all on a +// file the shim read, so its size stayed 0 and gzbuffer accepted every call, +// including the ones zlib itself would have refused; +// - the header look does make zlib allocate, at open, so zlib's gzbuffer went +// the other way and started refusing the *first* call as well -- confirmed +// in bare zlib with no shim loaded: gzbuffer returns 0 after gzopen and -1 +// after a gzdirect on the same file. +// +// So the refusals have to be replicated against the shim's own state, which is +// what io_started tracks. +// +// Judgment call, worth a reviewer's attention: the size itself is accepted and +// then not applied. The shim's buffers are a fixed 256 KiB of uncompressed and +// 512 KiB of compressed data, and the accelerator paths are sized around that +// split, so plumbing an arbitrary size through it is a change to the read and +// write paths rather than to this function. Ignoring it is a performance +// difference and not a correctness one for anything smaller than the shim's own +// buffers -- which is every default and most requests. The alternative, pinning +// any file whose caller calls gzbuffer to plain zlib, would take acceleration +// away from exactly the callers trying to tune for speed. +int ZEXPORT gzbuffer(gzFile file, unsigned size) { + Log(LogLevel::LOG_INFO, "gzbuffer Line ", __LINE__, ", file ", + static_cast(file), ", size ", size, "\n"); + + auto gz = gzip_files.Get(file); + if (gz == nullptr) { + return orig_gzbuffer != nullptr ? orig_gzbuffer(file, size) : -1; + } + + // zlib's checks, in zlib's order. Append counts as writing, which is how zlib + // records it. Note the last one is not a refusal in zlib either: a size below + // 8 is raised to 8, not rejected. + if (gz->mode == FileMode::NONE) { + return -1; + } + if (gz->io_started) { + return -1; + } + if ((size << 1) < size) { // zlib needs to be able to double it + return -1; + } + return 0; +} + ExecutionPath GetGzipFileExecutionPath(gzFile file) { auto gz = gzip_files.Get(file); if (gz == nullptr) { From 7cde6063843fb852d7170d622f37d66e3980319c Mon Sep 17 00:00:00 2001 From: Olasoji Date: Mon, 31 Aug 2026 14:23:53 -0700 Subject: [PATCH 2/9] gz: intercept gzopen64 zlib's gzopen64 is not a 64-bit variant of anything. It has the same signature as gzopen with no offset argument (zlib.h), and both symbols in libz are thunks onto the same internal gz_open. The "64" exists only so that the rename zlib.h performs under _FILE_OFFSET_BITS=64 has a symbol to land on. Leaving it unintercepted was safe but silent. The rename happens in the application's translation unit, so a program built that way calls gzopen64, the file is never registered with the shim, every other gz entry point delegates to zlib, and the result is correct end to end and simply unaccelerated. There is no sign that acceleration was lost, which is why a differential test built with -D_FILE_OFFSET_BITS=64 comes back with zero divergence and zero coverage. Forwarding to gzopen is all that is needed: gzopen opens the descriptor itself, with O_LARGEFILE where the platform has it. libz has no gzdopen64, so this has no counterpart. Correctness cannot detect this defect, so the test measures read-ahead instead. The shim pulls 512 KiB of compressed input per gzread where zlib's input buffer holds 16 KiB, so after one small gzread of a file between those two sizes, gzoffset reports which of the two did the reading. Verified against the previous library: 2419, unaccelerated. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 46 +++++++++++++++++++++++++++++++++++++++ zlib_accel.cpp | 16 ++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index f72a7fc..fae100d 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8082,6 +8082,52 @@ static std::string PositionStampedPayload(size_t records) { return payload; } +// gzopen64 is not a 64-bit variant of anything: in zlib it is the same function +// as gzopen, with the same signature and no offset argument. It exists only so +// the rename zlib.h performs under _FILE_OFFSET_BITS=64 has a symbol to land +// on. +// +// Leaving it unexported took the shim off the path entirely for any application +// built that way, and did so invisibly: zlib opened the file, the shim never +// learned of it, every other entry point delegated, and the file was correct +// end to end and merely unaccelerated. Correctness therefore cannot detect the +// bug. Read-ahead can. The shim pulls 512 KiB of compressed input per gzread +// where zlib's input buffer holds 16 KiB, so after one small gzread of a file +// larger than 16 KiB compressed, zlib cannot have consumed more than 16 KiB and +// the shim has consumed the lot. gzoffset reports which of the two happened. +TEST_F(GzipFileTest, Gzopen64RegistersTheFileWithTheShim) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(24000); + const char* filename = "file.gz"; + remove(filename); + + gzFile fp = gzopen64(filename, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + ASSERT_EQ(gzclose(fp), Z_OK); + + // Between the two buffer sizes, so the comparison below can tell them apart. + const auto compressed = std::filesystem::file_size(filename); + ASSERT_GT(compressed, static_cast(16 << 10)); + ASSERT_LT(compressed, static_cast(512 << 10)); + + fp = gzopen64(filename, "rb"); + ASSERT_NE(fp, nullptr); + char buf[24]; + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), static_cast(sizeof(buf))); + EXPECT_EQ(memcmp(buf, payload.data(), sizeof(buf)), 0); + EXPECT_EQ(gztell(fp), static_cast(sizeof(buf))); + // Plain zlib cannot report more than the 16 KiB it is able to hold, so this + // is only reachable with the file registered and the shim doing the reading. + EXPECT_GT(gzoffset(fp), static_cast(16 << 10)); + + ASSERT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + TEST_F(GzipFileTest, GztellCountsBytesOnBothSides) { EnableSomeGzCompressPath(); EnableShimOwnedGzReads(); diff --git a/zlib_accel.cpp b/zlib_accel.cpp index 33666bf..b616bcb 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -2408,6 +2408,22 @@ gzFile ZEXPORT gzopen(const char* path, const char* mode) { return file; } +// zlib's gzopen64 is not a 64-bit variant of anything: it is the same function +// as gzopen, with the same signature and no offset argument at all (zlib.h), +// and both symbols in libz are thunks onto the same internal gz_open. The "64" +// exists only so that the rename zlib.h performs under _FILE_OFFSET_BITS=64 +// (zconf.h, Z_WANT64) has a symbol to land on. +// +// Leaving it unintercepted was safe but silent: the rename happens in the +// application's translation unit, so a program built that way calls gzopen64, +// never registers the file with the shim, and runs correctly on plain zlib with +// no sign that acceleration was lost. Forwarding is all that is needed -- +// gzopen above opens the descriptor itself, with O_LARGEFILE where the platform +// has it. There is no gzdopen64 in libz, so this has no counterpart. +gzFile ZEXPORT gzopen64(const char* path, const char* mode) { + return gzopen(path, mode); +} + gzFile ZEXPORT gzdopen(int fd, const char* mode) { if (orig_gzdopen == nullptr) { return nullptr; From a266406dd66c1f0eebe9affd7d798436f79e08f7 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Mon, 31 Aug 2026 14:38:15 -0700 Subject: [PATCH 3/9] gz: run the header test in the shim for non-seekable descriptors The shim decides at open whether a file is a gzip member at all, by asking zlib through gzdirect and then rewinding the descriptor. That needs a seekable descriptor, so it skipped pipes -- which left the shim reading a pipe without having answered its one question, and two defects followed. A pipe carrying data that is not gzip was unreadable. Having learned nothing, the shim assumed gzip, tried to inflate plain bytes, and gzread returned -1 on a pipe plain zlib reads without difficulty. gzdirect on a pipe consumed the front of it. The shim never called gzdirect, so zlib was still sitting in its LOOK state on a descriptor the shim was reading. An application gzdirect made zlib look right then, pulling up to 8 KB out of the pipe into zlib's private buffer and punching a hole in the shim's input. Reading a gzip pipe worked, but gzdirect afterwards answered 1 -- zlib looking at a drained pipe and calling it not gzip, about a file the shim had just decompressed. So the shim takes two bytes of its own instead of borrowing zlib's 8 KB look. On a pipe that costs nothing, because putting them back never arises: the bytes are wanted by whoever reads next and the shim is that reader either way. It is also far less blocking than zlib's own look. The peek loops rather than trusting a single read, because zlib's gz_load loops until its buffer is full, so zlib always has two bytes to judge by; a single read returning one byte would call a real gzip pipe transparent where zlib calls it gzip. Where the peek says gzip the two bytes go back in at the front of io_buf on the first read; where it does not, the bytes are copied through, which is zlib's COPY mode. This forces gzdirect to be intercepted, which the earlier change had deliberately avoided. It is gated on the shim having done the peek, so nothing changes for a seekable file -- there zlib has looked and its cached answer is the true one in all four cases -- or for write mode. Every gz symbol libz exports is now intercepted. One asymmetry was found by measuring rather than by reading zlib, and it is the reason a test asserts two different answers to the same call. zlib's forward seek on a transparent file depends on whether a read has happened: before the first read how is still LOOK, so the seek is lazy and succeeds and the next read pays for it by reading and discarding, which a pipe permits; after the first read how is COPY, where gzseek64 lseeks the descriptor and a pipe refuses. Verified with the differential conformance probe, which gained six pipe checks that did not exist -- which is why both defects went unnoticed. Plain zlib and this branch now agree on all seven pipe checks; the unfixed library differs on all seven. Also a 1 MiB poorly-compressible stream through a pipe with a concurrent writer, so the peek is exercised across an io_buf refill. Known limitation, unchanged by this commit: the accelerator read loop fills its whole 512 KiB io_buf before decompressing, so a gzip pipe fed by a writer that has not closed blocks until that much arrives. That is pre-existing behaviour for gzip pipes, not introduced here. The transparent path added by this commit reads only what was asked for. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 265 ++++++++++++++++++++++++++++++++++++++ zlib_accel.cpp | 205 +++++++++++++++++++++++++++-- 2 files changed, 461 insertions(+), 9 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index fae100d..c95c3c1 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8128,6 +8128,271 @@ TEST_F(GzipFileTest, Gzopen64RegistersTheFileWithTheShim) { remove(filename); } +// --------------------------------------------------------------------------- +// Non-seekable descriptors. +// +// A pipe cannot be rewound, so the shim cannot borrow zlib's header look for +// it: the 8 KB zlib reads to do the look would be stranded in zlib's private +// buffer with the shim reading the descriptor from behind them. The shim +// therefore takes two bytes of its own and keeps them. Every test below writes +// far less than a pipe's 64 KiB capacity, so nothing blocks on an unread pipe. + +// Fills a pipe with plain bytes and returns the read end, write end closed. +static int PipeOfPlainBytes(const std::string& bytes) { + int fds[2]; + if (pipe(fds) != 0) return -1; + if (write(fds[1], bytes.data(), bytes.size()) != + static_cast(bytes.size())) { + close(fds[0]); + close(fds[1]); + return -1; + } + close(fds[1]); + return fds[0]; +} + +// The same, but the bytes are a gzip member written through the shim. +static int PipeOfGzipBytes(const std::string& payload) { + int fds[2]; + if (pipe(fds) != 0) return -1; + gzFile w = gzdopen(fds[1], "wb6"); + if (w == nullptr) { + close(fds[0]); + close(fds[1]); + return -1; + } + const bool ok = + gzwrite(w, payload.data(), static_cast(payload.size())) == + static_cast(payload.size()); + gzclose(w); + if (!ok) { + close(fds[0]); + return -1; + } + return fds[0]; +} + +// The case that already worked and must keep working: the peek says gzip, its +// two bytes go back in at the front of io_buf, and the read loop reads on top +// of them. If those bytes were lost the header would be truncated and the read +// would fail outright. +TEST_F(GzipFileTest, GzipPipeIsReadThroughTheAccelerator) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + const int fd = PipeOfGzipBytes(payload); + ASSERT_NE(fd, -1); + + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + std::string got(payload.size(), '\0'); + ASSERT_EQ(gzread(fp, &got[0], static_cast(got.size())), + static_cast(payload.size())); + EXPECT_EQ(got, payload); + // The peek said gzip, so this is not a transparent file. + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(gztell(fp), static_cast(payload.size())); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// The peek only sits at the front of io_buf for the first refill, so a stream +// bigger than io_buf is where it could be double-counted or lost at the +// boundary. A pipe holds 64 KiB, so this one needs a writer running alongside +// the reader -- which is also the shape a pipe is actually used in. +TEST_F(GzipFileTest, LargeGzipPipeSurvivesBufferRefills) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + // Poorly compressible, so the compressed stream is larger than the shim's + // 512 KiB io_buf and the read loop has to refill at least once. + const size_t size = 1 << 20; + std::string payload; + payload.reserve(size); + uint32_t x = 0x12345678; + for (size_t i = 0; i < size; i++) { + x = x * 1664525u + 1013904223u; + payload.push_back(static_cast(x >> 24)); + } + + int fds[2]; + ASSERT_EQ(pipe(fds), 0); + std::thread writer([&] { + gzFile w = gzdopen(fds[1], "wb6"); + if (w == nullptr) { + close(fds[1]); + return; + } + gzwrite(w, payload.data(), static_cast(payload.size())); + gzclose(w); + }); + + gzFile fp = gzdopen(fds[0], "rb"); + ASSERT_NE(fp, nullptr); + std::string got; + got.reserve(payload.size()); + std::vector buf(64 << 10); + int n; + while ((n = gzread(fp, buf.data(), static_cast(buf.size()))) > 0) { + got.append(buf.data(), static_cast(n)); + } + writer.join(); + + EXPECT_EQ(n, 0); + ASSERT_EQ(got.size(), payload.size()); + EXPECT_EQ(got, payload); + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(gztell(fp), static_cast(payload.size())); + int err = 0; + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// The defect: with nothing known about the descriptor the shim assumed gzip and +// tried to inflate plain text, so gzread returned -1 on a pipe plain zlib reads +// without difficulty. +TEST_F(GzipFileTest, NonGzipPipeIsCopiedThrough) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string plain = "not gzip at all, just text on a pipe\n"; + const int fd = PipeOfPlainBytes(plain); + ASSERT_NE(fd, -1); + + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + char buf[128]; + memset(buf, 0, sizeof(buf)); + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), static_cast(plain.size())); + EXPECT_EQ(std::string(buf, plain.size()), plain); + // Asked for more than there was, which is the only thing zlib's end-of-file + // indicator is set by. + EXPECT_EQ(gzeof(fp), 1); + EXPECT_EQ(gzdirect(fp), 1); + EXPECT_EQ(gztell(fp), static_cast(plain.size())); + int err = 0; + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// The second defect. Without an intercepted gzdirect, zlib is still in its LOOK +// state on a descriptor the shim is reading, so an application gzdirect makes +// zlib look right then -- pulling up to 8 KB out of the pipe into zlib's buffer +// and punching a hole in the front of the shim's input. The read after it is +// what proves nothing was taken. +TEST_F(GzipFileTest, GzdirectBeforeFirstReadDoesNotConsumeThePipe) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + const int fd = PipeOfGzipBytes(payload); + ASSERT_NE(fd, -1); + + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzdirect(fp), 0); + std::string got(payload.size(), '\0'); + ASSERT_EQ(gzread(fp, &got[0], static_cast(got.size())), + static_cast(payload.size())); + EXPECT_EQ(got, payload); + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// One byte is why the peek loops instead of trusting a single read: 0x1f alone +// is not a header, and zlib -- whose own load loops until its buffer is full -- +// calls this transparent. An empty pipe is the same conclusion with no bytes. +TEST_F(GzipFileTest, ShortAndEmptyPipesAreTransparent) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const int fd = PipeOfPlainBytes(std::string(1, '\x1f')); + ASSERT_NE(fd, -1); + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + unsigned char buf[8] = {0}; + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), 1); + EXPECT_EQ(buf[0], 0x1f); + EXPECT_EQ(gzdirect(fp), 1); + EXPECT_EQ(gzeof(fp), 1); + EXPECT_EQ(gzclose(fp), Z_OK); + + const int empty_fd = PipeOfPlainBytes(""); + ASSERT_NE(empty_fd, -1); + gzFile empty = gzdopen(empty_fd, "rb"); + ASSERT_NE(empty, nullptr); + EXPECT_EQ(gzread(empty, buf, sizeof(buf)), 0); + EXPECT_EQ(gzdirect(empty), 1); + EXPECT_EQ(gzeof(empty), 1); + int err = 0; + gzerror(empty, &err); + EXPECT_EQ(err, Z_OK); + EXPECT_EQ(gzclose(empty), Z_OK); +} + +// gzdirect is gated on the shim having done the peek, so a seekable file keeps +// answering from zlib's own cached look exactly as before. Both answers, +// because delegating the wrong way round would be invisible in only one of +// them. +TEST_F(GzipFileTest, GzdirectOnSeekableFilesStillComesFromZlib) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + const char* filename = "file.gz"; + remove(filename); + gzFile fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + ASSERT_EQ(gzclose(fp), Z_OK); + + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(gzclose(fp), Z_OK); + + // The same file with no gzip header, which zlib reads by copying through. + { + std::ofstream plain(filename, std::ios::binary | std::ios::trunc); + plain << "plain text in a file called .gz\n"; + } + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzdirect(fp), 1); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// zlib's forward seek on a transparent file is not one behaviour but two, and +// which one you get depends on whether a read has happened. Before the first +// read zlib is still in LOOK, so the seek is lazy and succeeds, and the read +// that follows pays for it by reading and discarding -- which a pipe permits. +// After the first read zlib is in COPY, where gzseek64 lseeks the descriptor +// directly, and a pipe refuses that. Both halves are measured against plain +// zlib by the conformance suite's O12 check. +TEST_F(GzipFileTest, ForwardSeekOnATransparentPipeFollowsZlib) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string plain = "0123456789ABCDEF"; + int fd = PipeOfPlainBytes(plain); + ASSERT_NE(fd, -1); + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzseek(fp, 4, SEEK_SET), 4); + char buf[8]; + memset(buf, 0, sizeof(buf)); + ASSERT_EQ(gzread(fp, buf, 4), 4); + EXPECT_EQ(std::string(buf, 4), "4567"); + EXPECT_EQ(gztell(fp), 8); + // Now a read has happened, so the same seek becomes an lseek on a pipe. + EXPECT_EQ(gzseek(fp, 12, SEEK_SET), -1); + EXPECT_EQ(gzclose(fp), Z_OK); +} + TEST_F(GzipFileTest, GztellCountsBytesOnBothSides) { EnableSomeGzCompressPath(); EnableShimOwnedGzReads(); diff --git a/zlib_accel.cpp b/zlib_accel.cpp index b616bcb..e062e6d 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -2152,6 +2152,20 @@ struct GzipFile { // those bytes a second time and then fail. Reads stay with the shim for the // life of the file; only the choice of decompressor may still change. bool shim_owns_reads = false; + // The header bytes of a descriptor that could not be rewound, and how many of + // them are really there -- 0, 1 or 2. On a pipe the bytes cannot be put back, + // so the shim keeps them and hands them to the read loop instead. They cannot + // live in io_buf: that is allocated lazily, on the first read. + unsigned char peek[2] = {0, 0}; + uint8_t peek_len = 0; + // Set when the shim, rather than zlib, ran the header test. This is what + // gzdirect answers from, and gating on it keeps the seekable case exactly as + // it was: there zlib has looked and answers for itself. + bool shim_peeked = false; + // The peek said this is not a gzip member, so there is nothing to decompress + // and the shim copies bytes through -- zlib's COPY mode, for the one case + // where zlib cannot be left to do it. + bool transparent_read = false; // The mirror image: this file was handed to zlib at open and every call on it // has been forwarded since, so zlib's own position and error state are the // complete and correct ones and the position entry points below just @@ -2237,27 +2251,88 @@ static void InitStreamRegistries() { // one in gz_look(). gzdirect() is the public way to reach it. That call is // guarded inside zlib by how == LOOK && x.have == 0, so it reads at most once // for the life of the file -// -- which is what makes this affordable, and is also why gzdirect itself needs -// no interception: after this call zlib answers from state it already has, so -// every later gzdirect the application makes is truthful and costs nothing. +// -- which is what makes this affordable, and is also why gzdirect needs no +// interception for a file that got here: after this call zlib answers from +// state it already has, so every later gzdirect the application makes is +// truthful and costs nothing. A descriptor that cannot be rewound never gets +// here; see GzPeekAtOpen, which is the case gzdirect does have to answer for. // // A file that is not a gzip member is not the shim's business. Hand it to zlib // and stay out of the way: zlib has already buffered the bytes and switched // itself to copy-through, so it reads the file correctly with no help. The same // applies to an empty file and to a file too short to hold a header. +// The same question, asked by the shim, for a descriptor that cannot be +// rewound. Borrowing zlib's answer is not possible there: zlib's look reads +// 8 KB, and on a pipe those bytes cannot be put back, so they would sit in +// zlib's private buffer with the shim reading the file from behind them. +// +// So the shim takes two bytes of its own. On a pipe that costs nothing, because +// putting them back never arises -- the bytes are wanted by whoever reads next, +// and the shim is that reader either way. It is also far less blocking than +// zlib's own look: two bytes rather than a 16 KB buffer. +// +// This is the second use of the magic-number test below in gzread, not a second +// implementation of it. It is also what forces gzdirect to be intercepted: zlib +// has not looked, so zlib cannot answer. +static void GzPeekAtOpen(GzipFile* gz) { + // zlib is going to read this pipe, so zlib must have every byte of it. Take + // nothing and leave it alone. Same two reasons as the seekable case below: + // nothing to offload to, or a mode string that already pinned the file. + const bool accelerator_selected = configs[USE_IAA_UNCOMPRESS] || + configs[USE_QAT_UNCOMPRESS] || + configs[USE_IGZIP_UNCOMPRESS]; + if (!accelerator_selected || gz->path == ZLIB) { + gz->path = ZLIB; + return; + } + + // Loop rather than one read. zlib's gz_load loops until its buffer is full or + // the input ends, so zlib always has two bytes to judge a header by; a single + // read that came back with one byte would call a real gzip pipe transparent + // where zlib calls it gzip. + while (gz->peek_len < sizeof(gz->peek)) { + const ssize_t got = + read(gz->fd, gz->peek + gz->peek_len, sizeof(gz->peek) - gz->peek_len); + if (got == 0) { + // Fewer than two bytes in the whole file. Not a header, and nothing more + // is coming -- the same conclusion zlib reaches. + break; + } + if (got < 0) { + // EINTR is not retried, matching gz_load and the error latch the rest of + // the read path already implements: the failure sticks to the file. + GzSetError(gz, Z_ERRNO, strerror(errno)); + break; + } + gz->peek_len += static_cast(got); + } + + gz->shim_peeked = true; + gz->transparent_read = + !(gz->peek_len == 2 && gz->peek[0] == 0x1f && gz->peek[1] == 0x8b); + // Both ways round: bytes have left the descriptor, so zlib must never read it + // again. That is the existing invariant, unchanged in meaning -- only the + // reason the descriptor moved is new. + gz->shim_owns_reads = true; +} + static void GzLookAtOpen(gzFile file, GzipFile* gz) { - if (gz->mode != FileMode::READ || orig_gzdirect == nullptr) { + if (gz->mode != FileMode::READ) { return; } // Where the file is now is both zlib's state->start and the position to put // the descriptor back to afterwards. A descriptor that cannot be seeked - // cannot be put back, so it cannot be looked at either: doing the look anyway - // would leave the 8 KB zlib read stranded in zlib's buffer, unreachable by - // the shim. Skip it and keep the existing behaviour rather than quietly - // moving every pipe onto zlib and taking its acceleration away. + // cannot be put back, so zlib's look cannot be borrowed for it and the shim + // runs its own test instead. -1 stays in start, which is what gzseek and + // gzrewind refuse on. gz->start = lseek(gz->fd, 0, SEEK_CUR); if (gz->start == static_cast(-1)) { + GzPeekAtOpen(gz); + return; + } + + if (orig_gzdirect == nullptr) { return; } @@ -3124,6 +3199,51 @@ int ZEXPORTVA gzprintf(gzFile file, const char* format, ...) { return ret; } +// zlib's COPY mode: the file is not a gzip member, so there is nothing to +// decompress and the bytes are simply handed through. zlib does this itself for +// every file it looked at, which is why the shim has no need of it -- except +// for a descriptor that could not be rewound, where the shim did the looking +// and now holds bytes zlib will never see. +// +// No push-back handling: the callers of GzreadOwnedFile drain it before they +// get here, which is the same arrangement the accelerator path relies on. +static int GzreadTransparent(GzipFile* gz, voidp buf, unsigned len) { + unsigned read_bytes = 0; + + // The header bytes the peek took. They were never anything but file content. + if (gz->peek_len > 0) { + const unsigned from_peek = gz->peek_len < len ? gz->peek_len : len; + memcpy(buf, gz->peek, from_peek); + // Whatever was not asked for stays at the front for the next call. + if (from_peek < gz->peek_len) { + memmove(gz->peek, gz->peek + from_peek, gz->peek_len - from_peek); + } + gz->peek_len -= static_cast(from_peek); + read_bytes = from_peek; + } + + while (read_bytes < len && !gz->reached_eof) { + const ssize_t got = + read(gz->fd, static_cast(buf) + read_bytes, len - read_bytes); + if (got == 0) { + gz->reached_eof = true; + break; + } + if (got < 0) { + GzSetError(gz, Z_ERRNO, strerror(errno)); + return -1; + } + read_bytes += static_cast(got); + } + + // Came up short of what was asked for, which is the one thing zlib sets its + // end-of-file indicator for. + if (read_bytes < len) { + gz->read_past_end = true; + } + return static_cast(read_bytes); +} + static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { // Check every symbol this function may need before touching any state. The // accelerator path can hand the rest of the file to zlib at any point, and a @@ -3201,7 +3321,13 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { // configuration may still change which decompressor runs -- the fallback // below uses the shim's own inflate stream -- but never who reads the // descriptor. - if (gz->shim_owns_reads || (gz->path != ZLIB && accelerator_selected)) { + // Not a gzip member, on a descriptor the shim had to test itself. Handled + // here rather than by returning early so that the tail below still counts the + // bytes towards gztell, and so that a pending gzseek above still runs first. + if (gz->transparent_read) { + read_bytes = GzreadTransparent(gz, buf, len); + } else if (gz->shim_owns_reads || + (gz->path != ZLIB && accelerator_selected)) { if (!accelerator_selected) { gz->use_zlib_for_decompression = true; } @@ -3215,6 +3341,18 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { gz->data_buf_size = 512 << 10; gz->io_buf_size = 512 << 10; + // The header bytes GzPeekAtOpen took off a non-seekable descriptor. They + // could not be put back, so they go in at the front of io_buf and the loop + // below reads on top of them: read() appends at io_buf_content, so this is + // just the buffer starting out holding its first two bytes. They cannot be + // stored here at open time because io_buf does not exist until now. + if (gz->peek_len > 0) { + memcpy(gz->io_buf, gz->peek, gz->peek_len); + gz->io_buf_content = gz->peek_len; + gz->io_buf_pos = 0; + gz->peek_len = 0; + } + bool more_data = true; while (read_bytes < len && more_data) { // Get uncompressed data from data_buf @@ -3739,6 +3877,24 @@ int ZEXPORT gzeof(gzFile file) { return gz->read_past_end; } +// Intercepted for one case only: a descriptor the shim had to run the header +// test on itself, because it could not be rewound. There zlib has not looked +// and cannot answer -- worse, an application gzdirect would make zlib look +// right then, pulling up to 8 KB out of the pipe into zlib's private buffer and +// punching a hole in the front of the shim's input. +// +// Everything else delegates, which is the whole point of the shim_peeked gate. +// A seekable file already has zlib's own cached answer, and it is the true one +// in all four cases (see GzLookAtOpen); a write-mode file keeps whatever zlib +// reports for "wT". Neither is touched here. +int ZEXPORT gzdirect(gzFile file) { + auto gz = gzip_files.Get(file); + if (gz == nullptr || !gz->shim_peeked) { + return orig_gzdirect != nullptr ? orig_gzdirect(file) : 0; + } + return gz->transparent_read ? 1 : 0; +} + // --------------------------------------------------------------------------- // Position and error state. // @@ -3834,6 +3990,37 @@ static z_off64_t GzSeekOwned(GzipFile* gz, z_off64_t offset, int whence) { } gz->pending_skip = 0; + // A transparent file is not compressed, so seeking in it needs no inflating: + // zlib's gzseek64 has a fast path that lseeks the descriptor and is done + // (gzlib.c), forward and backward alike. That path is guarded by how == COPY, + // which is worth being exact about, because it makes zlib's answer depend on + // whether anything has been read yet: + // + // before the first read how is still LOOK, so the seek is lazy and + // succeeds; the next read pays for it by reading and + // discarding, which works on a pipe + // after the first read how is COPY, so the lseek is attempted, and a pipe + // refuses it + // + // io_started is the shim's equivalent: it flips at the top of the first read, + // the same moment zlib's look runs. Measured rather than assumed -- the + // conformance suite's O11/O12 pipe seeks are what turned this asymmetry up. + if (gz->transparent_read && gz->io_started && gz->pos + offset >= 0) { + const off_t held = static_cast(gz->peek_len) + + static_cast(gz->pushback.size()); + if (lseek(gz->fd, static_cast(offset) - held, SEEK_CUR) == + static_cast(-1)) { + return -1; + } + gz->peek_len = 0; + gz->pushback.clear(); + gz->reached_eof = false; + gz->read_past_end = false; + GzSetError(gz, Z_OK, nullptr); + gz->pos += offset; + return gz->pos; + } + if (offset < 0) { // Backwards. Only a reader can go there, and only by starting over: the // shim has no index of the compressed stream, exactly as zlib has none. From 25206bce9d73b5173e0830ed65280e466c638e59 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Mon, 31 Aug 2026 14:52:11 -0700 Subject: [PATCH 4/9] gz: shrink zlib's open-time header look with gzbuffer The open-time gzdirect() call that decides whether a file is a gzip member costs more than the answer is worth. zlib's gz_look() sizes both of its buffers from `want` and reads `want` bytes to judge the header by, and `want` is settable beforehand through public gzbuffer. Ask for a small one and the look gets cheaper without anything else changing: zlib still does a genuine look and still sets its own how/direct, and no private state of zlib's is touched. Measured on this host with mallinfo2 across 64 files open for reading at once, and by watching where the descriptor lands: gzbuffer per-file look look reads gzgetc over 2 MB transparent default 31,776 8,192 B 0.002s 8 7,232 8 B 0.042s (21x) 64 7,392 64 B 0.011s 512 8,736 512 B 0.004s (2x) 1024 10,158 1,024 B 0.003s 4096 19,488 4,096 B 0.002s 512 is the knee. Going below it saves at most another 1.5 KB per file and costs a great deal on the one path that still runs through zlib's buffer, a transparent file read a byte at a time. Confirmed end to end through the shim rather than in bare zlib, same metric, .gz files created outside the process so the arena is not warmed by a write-mode open: 275,970 bytes per open read file on the base commit, 307,836 with the default look, 284,627 with this change. The look's own cost falls 72.8%. Two ordering constraints, both load-bearing: - gzbuffer has to come before gzdirect. zlib refuses gzbuffer once its buffers exist, and the look is what creates them. zlib.h states this. - The accelerator test has to come before gzbuffer, and this commit hoists it there. A file that no accelerator will read is handed to zlib to read in full, and zlib inflating a whole file through a tiny input buffer is 85x slower (0.004s -> 0.343s at want=8). Shrinking the buffer of a file zlib is about to read is a performance defect, not a saving. The hoisted test is now a named helper, since the peek path asks the same question. Also repairs two comments that the previous commit's insertions left attached to the wrong code: GzLookAtOpen's doc comment had run into GzPeekAtOpen's, and the paragraph explaining why shim_owns_reads is checked on its own had ended up above the transparent-read branch instead. No behaviour change for any application: the shim's own gzbuffer already answers from the shim's state, so a caller's later gzbuffer sees exactly what it saw before. Verified: full unit suite 3281 run, 3275 pass, 0 fail, 6 pre-existing skips; differential conformance output byte-identical to the previous commit's; clang-format clean; Debug and Release builds clean under -Werror. Signed-off-by: Olasoji --- zlib_accel.cpp | 123 ++++++++++++++++++++++++++++--------------------- 1 file changed, 71 insertions(+), 52 deletions(-) diff --git a/zlib_accel.cpp b/zlib_accel.cpp index e062e6d..80f12d1 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -2244,44 +2244,32 @@ static void InitStreamRegistries() { gzip_files.Init(); } -// Ask zlib, once per read-mode open, whether this file is a gzip member at all. -// -// The shim deliberately has no magic-number test of its own: two -// implementations of "is this a gzip header" would drift, and zlib already has -// one in gz_look(). gzdirect() is the public way to reach it. That call is -// guarded inside zlib by how == LOOK && x.have == 0, so it reads at most once -// for the life of the file -// -- which is what makes this affordable, and is also why gzdirect needs no -// interception for a file that got here: after this call zlib answers from -// state it already has, so every later gzdirect the application makes is -// truthful and costs nothing. A descriptor that cannot be rewound never gets -// here; see GzPeekAtOpen, which is the case gzdirect does have to answer for. -// -// A file that is not a gzip member is not the shim's business. Hand it to zlib -// and stay out of the way: zlib has already buffered the bytes and switched -// itself to copy-through, so it reads the file correctly with no help. The same -// applies to an empty file and to a file too short to hold a header. -// The same question, asked by the shim, for a descriptor that cannot be -// rewound. Borrowing zlib's answer is not possible there: zlib's look reads -// 8 KB, and on a pipe those bytes cannot be put back, so they would sit in -// zlib's private buffer with the shim reading the file from behind them. +// Two requests no backend can serve, and neither needs the file looked at: no +// uncompress accelerator is configured, or the mode string already pinned this +// file to zlib (a level digit no backend can serve, which zlib parses in read +// mode too). Both mean zlib is going to read the file, which is why this has to +// be settled before anything is spent on the header test -- a file zlib reads +// must keep both its bytes and its full-sized buffers. +static bool GzUncompressAcceleratorSelected() { + return configs[USE_IAA_UNCOMPRESS] || configs[USE_QAT_UNCOMPRESS] || + configs[USE_IGZIP_UNCOMPRESS]; +} + +// The header test asked by the shim rather than by zlib, for a descriptor that +// cannot be rewound. Borrowing zlib's answer is not possible there: zlib's look +// reads up to 8 KB, and on a pipe those bytes cannot be put back, so they would +// sit in zlib's private buffer with the shim reading the file from behind them. // // So the shim takes two bytes of its own. On a pipe that costs nothing, because // putting them back never arises -- the bytes are wanted by whoever reads next, // and the shim is that reader either way. It is also far less blocking than -// zlib's own look: two bytes rather than a 16 KB buffer. +// zlib's own look. // -// This is the second use of the magic-number test below in gzread, not a second +// This is the second use of the magic-number test in gzread below, not a second // implementation of it. It is also what forces gzdirect to be intercepted: zlib // has not looked, so zlib cannot answer. static void GzPeekAtOpen(GzipFile* gz) { - // zlib is going to read this pipe, so zlib must have every byte of it. Take - // nothing and leave it alone. Same two reasons as the seekable case below: - // nothing to offload to, or a mode string that already pinned the file. - const bool accelerator_selected = configs[USE_IAA_UNCOMPRESS] || - configs[USE_QAT_UNCOMPRESS] || - configs[USE_IGZIP_UNCOMPRESS]; - if (!accelerator_selected || gz->path == ZLIB) { + if (!GzUncompressAcceleratorSelected() || gz->path == ZLIB) { gz->path = ZLIB; return; } @@ -2316,6 +2304,22 @@ static void GzPeekAtOpen(GzipFile* gz) { gz->shim_owns_reads = true; } +// Ask zlib, once per read-mode open, whether this file is a gzip member at all. +// +// The shim deliberately has no magic-number test of its own for a file it can +// rewind: two implementations of "is this a gzip header" would drift, and zlib +// already has one in gz_look(). gzdirect() is the public way to reach it. That +// call is guarded inside zlib by how == LOOK && x.have == 0, so it reads at +// most once for the life of the file +// -- which is what makes this affordable, and is also why gzdirect needs no +// interception for a file that got here: after this call zlib answers from +// state it already has, so every later gzdirect the application makes is +// truthful and costs nothing. +// +// A file that is not a gzip member is not the shim's business. Hand it to zlib +// and stay out of the way: zlib has already buffered the bytes and switched +// itself to copy-through, so it reads the file correctly with no help. The same +// applies to an empty file and to a file too short to hold a header. static void GzLookAtOpen(gzFile file, GzipFile* gz) { if (gz->mode != FileMode::READ) { return; @@ -2332,25 +2336,39 @@ static void GzLookAtOpen(gzFile file, GzipFile* gz) { return; } - if (orig_gzdirect == nullptr) { + // Settled before the look, not after it: see GzUncompressAcceleratorSelected. + if (!GzUncompressAcceleratorSelected() || gz->path == ZLIB) { + gz->path = ZLIB; return; } - if (orig_gzdirect(file) != 0) { - // Not a gzip member. zlib owns it from here. - gz->path = ZLIB; + if (orig_gzdirect == nullptr) { return; } - // A real gzip member, but only rewind if the shim is actually going to read - // it. Two ways it will not: no uncompress accelerator is configured, or the - // mode string already pinned the file to zlib (a level digit no backend can - // serve, which zlib parses in read mode too). In both cases zlib reads the - // file, and it must keep the bytes it has just buffered. - const bool accelerator_selected = configs[USE_IAA_UNCOMPRESS] || - configs[USE_QAT_UNCOMPRESS] || - configs[USE_IGZIP_UNCOMPRESS]; - if (!accelerator_selected || gz->path == ZLIB) { + // Shrink the look before it happens. gz_look sizes both of its buffers from + // `want` and reads `want` bytes to judge the header by, and `want` is + // settable through public gzbuffer -- which zlib refuses once its buffers + // exist, so this must come before the gzdirect below and zlib.h says so. zlib + // does a genuine look either way and sets its own how/direct; no private + // state is touched. + // + // Measured on this host, per file open for reading at the same time: 31,776 + // bytes of zlib buffers and inflate state at the default, 8,736 at 512, and a + // 512-byte read of the descriptor instead of 8,192. Smaller sizes save little + // more -- 7,232 bytes at zlib's floor of 8 -- and cost a great deal on the + // one path that still goes through zlib's buffer: a transparent file read a + // byte at a time is 21x slower at 8 and 2x slower at 512. + // + // It matters that this is below the test above. zlib inflating a whole file + // through an 8-byte input buffer is 85x slower, and that is exactly the file + // the test above has already sent to zlib. + if (orig_gzbuffer != nullptr) { + orig_gzbuffer(file, 512); + } + + if (orig_gzdirect(file) != 0) { + // Not a gzip member. zlib owns it from here. gz->path = ZLIB; return; } @@ -3311,19 +3329,20 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { int ret = 1; uint32_t read_bytes = 0; - bool accelerator_selected = configs[USE_IAA_UNCOMPRESS] || - configs[USE_QAT_UNCOMPRESS] || - configs[USE_IGZIP_UNCOMPRESS]; - // shim_owns_reads is checked first and on its own: the descriptor was rewound - // after the open-time header look, so zlib's buffered copy of the header - // describes a position the file is no longer at. Handing this file to + const bool accelerator_selected = GzUncompressAcceleratorSelected(); + // First arm: not a gzip member, on a descriptor the shim had to test itself, + // so there is nothing to decompress. Handled here rather than by returning + // early so that the tail below still counts the bytes towards gztell, and so + // that a pending gzseek above still runs ahead of it. + // + // Second arm: shim_owns_reads is checked on its own, before the path and + // config, because the descriptor was moved at open -- rewound after zlib's + // look, or read from by the peek. zlib's buffered copy of the header + // describes a position the file is no longer at, so handing this file to // orig_gzread would serve those bytes twice and then fail the checksum. The // configuration may still change which decompressor runs -- the fallback // below uses the shim's own inflate stream -- but never who reads the // descriptor. - // Not a gzip member, on a descriptor the shim had to test itself. Handled - // here rather than by returning early so that the tail below still counts the - // bytes towards gztell, and so that a pending gzseek above still runs first. if (gz->transparent_read) { read_bytes = GzreadTransparent(gz, buf, len); } else if (gz->shim_owns_reads || From 1d1ec22a55b7abfd28f30c1564825b201f20f60c Mon Sep 17 00:00:00 2001 From: Olasoji Date: Tue, 1 Sep 2026 10:00:47 -0700 Subject: [PATCH 5/9] tests: drop an unused byte counter that clang rejects GzerrorLatchesAndGzclearerrClearsIt accumulated the return of each gzread into a local that nothing ever read. Clang treats that as an error under -Werror (-Wunused-but-set-variable); gcc does not warn, so it went unnoticed until the clang job ran. Removed rather than asserted on. The count is not a stable property to pin here: on a member whose CRC32 is corrupt the shim refuses the read outright and delivers no payload, where zlib may hand back the payload before it checks the trailer. What this test is about is the error latch and gzclearerr, and those assertions are unchanged. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index c95c3c1..a07e229 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8620,11 +8620,12 @@ TEST_F(GzipFileTest, GzerrorLatchesAndGzclearerrClearsIt) { EXPECT_EQ(errnum, Z_OK); std::vector output(input_length + 512, 0); - int total = 0; int ret = 0; + // Drain to the failure. How many bytes arrive first is deliberately not + // asserted: it depends on which path decompresses the member, and this + // test is about the error latch, not about byte counts. while ((ret = gzread(fp, output.data(), static_cast(output.size()))) > 0) { - total += ret; } EXPECT_EQ(ret, -1); From 95a17ba902249e5700db9a742a294a01d28343bf Mon Sep 17 00:00:00 2001 From: Olasoji Date: Tue, 1 Sep 2026 14:49:21 -0700 Subject: [PATCH 6/9] gz: match zlib on lazy header look, narrowing, and write errors Nine places where the shim's answer differed from the one plain zlib gives. Each was checked against zlib's own gzread.c/gzwrite.c/gzlib.c, and the version-sensitive ones were measured against the installed libz rather than read off a source copy. The header look for a descriptor that cannot be rewound now happens on the first call that needs the answer instead of inside gzopen/gzdopen. zlib does no I/O at all in the open, so reading there was a behaviour change with teeth: a single-threaded program that wraps a pipe's read end and only then writes to it deadlocked in gzdopen, and a non-blocking descriptor latched Z_ERRNO from EAGAIN before the application had asked for anything. Ownership is still settled at open, where it needs no I/O to decide; only the bytes move later. shim_must_peek says the shim owes this file a header test, shim_peeked says it has run, and gzdirect gates on the first while the transparent-seek path keys off the second through transparent_read. That seek was gated on io_started, which was a stand-in for the look back when the look was eager; it is now wrong, because gzdirect moves zlib to COPY without any application-visible I/O. gzseek, gztell and gzoffset now narrow the way zlib does -- an offset that does not survive the cast is -1, not a smaller plausible number. Latent where z_off_t is 64 bits, live on a 32-bit build without large-file support. Zero-length gzread and gzwrite no longer count as the start of I/O: zlib returns before it allocates, so a following gzbuffer is still accepted and a pending seek gap is left alone. gzungetc now does count, because zlib's runs its look and that allocates. gzflush validates the flush value before it fills a pending gap. It used to write the zeros and then refuse the call: measured, a rejected gzflush grew the file from 24 bytes to 4359. gzclose keeps what the gap fill returned. Dropping it meant a full disk produced a short file and Z_OK from the one call an application has to ask whether its data is safe. Write failures inside zlib are copied into the shim's latch. For a file that started on an accelerator and was moved to the zlib path later -- gzsetparams to a level no backend serves, or a mid-write fallback -- the failure landed in zlib's state while gzerror answered from the shim's, so both gzerror and gzclose reported success for data that never reached the file. One direction only: this can add an error, never clear one. gzsetparams checks its own error latch. On this path the shim did the writing, so the shim holds the latch; zlib's state never saw the failure and would answer Z_OK for a broken file. Measured in bare zlib: Z_STREAM_ERROR after a failed write, and Z_STREAM_ERROR even when the level asked for is the one already set, because the latch is checked before the no-change shortcut. Tests: eleven new cases, and one correction. FlushFailureReportsZErrno was asserting three wrong answers -- it failed identically against the pre-fix library, so it had never been right, and CI could not see it because CI builds no accelerator. Correcting it against the installed libz is what turned up the gzsetparams latch above. Three write-side cases were also skipping in CI for no reason. What puts gzwrite on the shim's own buffered route is the config flag, not a compiled-in backend, so EnableShimOwnedGzWrites replaces the #if guards and those fixes are now covered by the default build. The seek gaps in them are 4 MiB because the shim buffers 256 KiB before anything reaches the descriptor and zeros compress about 200:1 -- at 4 KiB the file never changes and the tests pass on a broken shim. Also fixes a 17-byte test record appended 16 bytes at a time after a 14-byte snprintf, which put two bytes of stack into a test payload. Differential suite against bare zlib, now 51 checks: main diverges in 27, this branch before these fixes in 10, after them in 1 -- R11, which corrupts a byte at a fixed offset in two files that are compressed differently and so cannot match by construction. Unit tests 3285 pass, 0 fail, 5 skipped; 56/56 GzipFileTest in an igzip build; clean under ASAN with leak detection, under clang, and across the eight DEBUG_LOG x ENABLE_STATISTICS x Debug/Release permutations with -Werror. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 389 ++++++++++++++++++++++++++++++++++++-- zlib_accel.cpp | 261 +++++++++++++++++++++---- 2 files changed, 592 insertions(+), 58 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index a07e229..d2ee2c9 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -7473,19 +7474,38 @@ TEST_F(GzipFileTest, GzsetparamsRejectsReadFile) { DestroyBlock(input); } -// Everything that flushes the shim's buffer has to report a failed write the -// way zlib does, as Z_ERRNO -- the code that tells the caller errno describes -// what happened. /dev/full makes that deterministic: every write to it fails -// with ENOSPC, so one file exercises all three entry points that flush. +// Put gzwrite on the shim's own buffered route. What decides that is the config +// flag, not whether a backend was compiled in: with a compress flag set, the +// shim keeps the file and drives its own deflate stream, falling back to zlib's +// deflate for the compression itself when no accelerator is present. Without a +// flag, gzwrite hands the whole file to zlib and the shim's write buffer -- +// with the seek gap it holds and the flush that empties it -- does not exist to +// be tested. So these tests run everywhere, including CI's no-accelerator +// build. +static void EnableShimOwnedGzWrites() { + SetConfig(USE_IAA_COMPRESS, 0); + SetConfig(USE_QAT_COMPRESS, 1); + SetConfig(USE_IGZIP_COMPRESS, 0); + SetConfig(USE_ZLIB_COMPRESS, 1); +} + +// What every write-side call answers once a write has already failed. /dev/full +// makes that deterministic: every write to it fails with ENOSPC, so one file +// exercises all three entry points that flush. +// +// The answers are not all the same, and they are not all Z_ERRNO. zlib latches +// Z_ERRNO -- errno describes what happened -- and gzerror reports it, but +// gzflush and gzsetparams both refuse outright on a latched error and return +// Z_STREAM_ERROR instead of passing the code through. gzsetparams does so even +// when the level asked for is the one the file already has, because zlib checks +// the latch before it checks whether anything would change. Only the close +// hands the latched code back. Measured in bare zlib 1.3 with no shim loaded, +// one call at a time. TEST_F(GzipFileTest, FlushFailureReportsZErrno) { -#if !defined(USE_IGZIP) && !defined(USE_QAT) && !defined(USE_IAA) - GTEST_SKIP() << "no backend compiled in, so the file is pinned to zlib and " - "these calls are zlib's to answer"; -#endif if (access("/dev/full", W_OK) != 0) { GTEST_SKIP() << "/dev/full is not available"; } - EnableSomeGzCompressPath(); + EnableShimOwnedGzWrites(); int fd = open("/dev/full", O_WRONLY); ASSERT_NE(fd, -1); @@ -7501,14 +7521,19 @@ TEST_F(GzipFileTest, FlushFailureReportsZErrno) { ASSERT_NE(input, nullptr); EXPECT_EQ(gzwrite(fp, input, static_cast(input_length)), 0); - EXPECT_EQ(gzflush(fp, Z_SYNC_FLUSH), Z_ERRNO); - // The level has to differ from the one the file was opened with, or there is - // nothing to flush for and zlib itself would not flush either. - EXPECT_EQ(gzsetparams(fp, 1, Z_DEFAULT_STRATEGY), Z_ERRNO); - // Which is the other half: asking for the level the file already has changes - // nothing, so it must not flush, and it succeeds even here. That the request - // above is still a change proves the failed call recorded nothing. - EXPECT_EQ(gzsetparams(fp, Z_DEFAULT_COMPRESSION, Z_DEFAULT_STRATEGY), Z_OK); + // The failure itself is Z_ERRNO, and that is the code the file is holding. + int err = Z_OK; + gzerror(fp, &err); + EXPECT_EQ(err, Z_ERRNO); + + // But these two refuse on the latch rather than reporting it. + EXPECT_EQ(gzflush(fp, Z_SYNC_FLUSH), Z_STREAM_ERROR); + EXPECT_EQ(gzsetparams(fp, 1, Z_DEFAULT_STRATEGY), Z_STREAM_ERROR); + // Including when the level asked for is the one the file already has, which + // would otherwise be a no-op: the latch is checked first. + EXPECT_EQ(gzsetparams(fp, Z_DEFAULT_COMPRESSION, Z_DEFAULT_STRATEGY), + Z_STREAM_ERROR); + // The close is the one call that hands the latched code back. EXPECT_EQ(gzclose_w(fp), Z_ERRNO); DestroyBlock(input); @@ -8074,8 +8099,13 @@ static void EnableShimOwnedGzReads() { static std::string PositionStampedPayload(size_t records) { std::string payload; payload.reserve(records * 16); - char record[17]; for (size_t i = 0; i < records; i++) { + // "[off" + 8 digits + "]" is 13 characters, and the record is zeroed first + // so that the three bytes after it are ones this function chose rather than + // whatever was on the stack -- the append below takes all 16. Keeping them + // NUL also keeps snprintf's terminator in place, which is what lets a test + // read 16 bytes and compare the result as a C string. + char record[17] = {0}; snprintf(record, sizeof(record), "[off%08zu]", i * 16); payload.append(record, 16); } @@ -8709,6 +8739,329 @@ TEST_F(GzipFileTest, GzbufferAcceptsOnlyBeforeAnyIo) { remove(filename); } +// --------------------------------------------------------------------------- +// The header test happens on demand, not at open. +// +// zlib performs no I/O at all inside gzopen or gzdopen: it decides nothing +// about the file until the first read. Reading two bytes at open to run the +// shim's own header test broke that in two visible ways, and these are those +// two ways. +// --------------------------------------------------------------------------- + +// gzdopen on a pipe that is empty but still has a writer must return, because a +// single-threaded program is allowed to wrap the read end first and write to it +// afterwards. An open-time read blocks there and the program never gets control +// back. +// +// The open runs on a thread only so the test can survive its own failure: if +// the open does block, the write below unblocks it and the join succeeds, and +// the expectation reports it. Run inline, a regression here would hang the +// suite. +TEST_F(GzipFileTest, GzdopenOnAPipeWithNoDataYetDoesNotBlock) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + int fds[2]; + ASSERT_EQ(pipe(fds), 0); + + const std::string payload = PositionStampedPayload(64); + gzFile fp = nullptr; + std::promise opened; + std::future opened_future = opened.get_future(); + std::thread opener([&] { + fp = gzdopen(fds[0], "rb"); + opened.set_value(); + }); + + const bool returned_before_any_data = + opened_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready; + + // Written whether or not the open came back, so the thread is always + // joinable. + gzFile wp = gzdopen(fds[1], "wb6"); + ASSERT_NE(wp, nullptr); + ASSERT_EQ(gzwrite(wp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + ASSERT_EQ(gzclose(wp), Z_OK); + opener.join(); + + EXPECT_TRUE(returned_before_any_data); + ASSERT_NE(fp, nullptr); + + // And the header test still runs when it is needed, so the deferral costs the + // file nothing. + std::string got(payload.size(), '\0'); + ASSERT_EQ(gzread(fp, &got[0], static_cast(got.size())), + static_cast(payload.size())); + EXPECT_EQ(got, payload); + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// A non-blocking descriptor with nothing on it yet. An open-time read comes +// back EAGAIN, which the peek's error path latches as Z_ERRNO -- so the file is +// broken before the application has asked it for anything, and plain zlib +// reports no such error. +TEST_F(GzipFileTest, GzdopenOnANonBlockingPipeLatchesNoError) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + int fds[2]; + ASSERT_EQ(pipe(fds), 0); + ASSERT_EQ(fcntl(fds[0], F_SETFL, O_NONBLOCK), 0); + + gzFile fp = gzdopen(fds[0], "rb"); + ASSERT_NE(fp, nullptr); + int err = Z_OK; + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + // Nothing has been read, so the gzbuffer opportunity is still open. This is + // the same fact from the other side: zlib refuses gzbuffer once it has + // allocated, and it allocates when it looks. + EXPECT_EQ(gzbuffer(fp, 8192), 0); + + // The whole member goes in before the first read, so no read below meets an + // empty pipe and O_NONBLOCK never comes into it. + const std::string payload = PositionStampedPayload(64); + gzFile wp = gzdopen(fds[1], "wb6"); + ASSERT_NE(wp, nullptr); + ASSERT_EQ(gzwrite(wp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + ASSERT_EQ(gzclose(wp), Z_OK); + + std::string got(payload.size(), '\0'); + ASSERT_EQ(gzread(fp, &got[0], static_cast(got.size())), + static_cast(payload.size())); + EXPECT_EQ(got, payload); + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// The other half of ForwardSeekOnATransparentPipeFollowsZlib, and the reason +// the lazy peek changes which flag gates that seek. gzdirect is the one call +// that makes zlib look without the application having read anything: after it +// zlib is in COPY, so gzseek lseeks the descriptor and a pipe refuses it. Keyed +// on "has any read happened" the shim would instead take the lazy path and +// return the offset, disagreeing with zlib on the same call sequence. +TEST_F(GzipFileTest, GzdirectThenSeekOnATransparentPipeIsRefused) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const int fd = PipeOfPlainBytes("0123456789ABCDEF"); + ASSERT_NE(fd, -1); + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + + EXPECT_EQ(gzdirect(fp), 1); + EXPECT_EQ(gzseek(fp, 4, SEEK_SET), -1); + // Refused, not damaged: the file still reads from where it was. + char buf[8] = {0}; + ASSERT_EQ(gzread(fp, buf, 4), 4); + EXPECT_EQ(std::string(buf, 4), "0123"); + EXPECT_EQ(gzclose(fp), Z_OK); +} + +// --------------------------------------------------------------------------- +// Boundary cases the shim used to answer differently from zlib. +// --------------------------------------------------------------------------- + +// zlib's gz_read and gz_write both return 0 for a zero-length request before +// they allocate anything and before they serve a pending seek. So a zero-length +// call is not the start of I/O: gzbuffer is still legal after it, and a +// promised seek is still outstanding. Measured in bare zlib with no shim +// loaded. +TEST_F(GzipFileTest, ZeroLengthIoIsNotTheStartOfIo) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + char buf[17] = {0}; + EXPECT_EQ(gzread(fp, buf, 0), 0); + EXPECT_EQ(gzbuffer(fp, 8192), 0); + + // And it leaves a promised seek alone rather than paying for it. + EXPECT_EQ(gzseek(fp, 32, SEEK_SET), 32); + EXPECT_EQ(gzread(fp, buf, 0), 0); + EXPECT_EQ(gztell(fp), 32); + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_STREQ(buf, "[off00000032]"); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); + + fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzwrite(fp, "", 0), 0); + EXPECT_EQ(gzbuffer(fp, 8192), 0); + // The pending seek survives it here too, so the zeros land at the close and + // not one call early. + EXPECT_EQ(gzseek(fp, 16, SEEK_CUR), 16); + EXPECT_EQ(gzwrite(fp, "", 0), 0); + EXPECT_EQ(gztell(fp), 16); + ASSERT_EQ(gzwrite(fp, payload.data(), static_cast(payload.size())), + static_cast(payload.size())); + ASSERT_EQ(gzclose(fp), Z_OK); + + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + std::string got(16 + payload.size(), 'x'); + ASSERT_EQ(gzread(fp, &got[0], static_cast(got.size())), + static_cast(got.size())); + EXPECT_EQ(got, std::string(16, '\0') + payload); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// zlib's gzungetc calls gz_look, which allocates, so the gzbuffer opportunity +// is gone afterwards even though the caller has read nothing. Measured against +// libz 1.3 rather than read off the vendored copy: that gz_look call arrived in +// 1.2.12 and the copy predates it. +TEST_F(GzipFileTest, GzungetcEndsTheGzbufferOpportunity) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzbuffer(fp, 8192), 0); + ASSERT_EQ(gzungetc('X', fp), 'X'); + EXPECT_EQ(gzbuffer(fp, 16384), -1); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// zlib's gzflush checks the flush value along with the mode and the error +// latch, all before it fills a pending seek. Filling first means a rejected +// call has already changed the file: the zeros are in it and the call still +// returns Z_STREAM_ERROR, so nothing tells the caller that happened. +// +// The gap is deliberately bigger than the shim's 256 KiB write buffer, because +// that is what makes the difference reach the file. A small gap is buffered and +// the file on disk looks the same either way. +TEST_F(GzipFileTest, GzflushRejectsBadFlushBeforeFillingASeek) { + EnableShimOwnedGzWrites(); + SetUncompressPath(ZLIB, false, false); + + const z_off_t gap = 4 << 20; + const char* filename = "file.gz"; + remove(filename); + gzFile fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, "head", 4), 4); + ASSERT_EQ(gzflush(fp, Z_SYNC_FLUSH), Z_OK); + const auto size_before = std::filesystem::file_size(filename); + + ASSERT_EQ(gzseek(fp, gap, SEEK_CUR), gap + 4); + EXPECT_EQ(gzflush(fp, Z_FINISH + 1), Z_STREAM_ERROR); + EXPECT_EQ(std::filesystem::file_size(filename), size_before); + EXPECT_EQ(gzflush(fp, -1), Z_STREAM_ERROR); + EXPECT_EQ(std::filesystem::file_size(filename), size_before); + + // The seek is still owed, so a flush zlib accepts pays it. + EXPECT_EQ(gzflush(fp, Z_SYNC_FLUSH), Z_OK); + EXPECT_GT(std::filesystem::file_size(filename), size_before); + EXPECT_EQ(gztell(fp), gap + 4); + ASSERT_EQ(gzclose(fp), Z_OK); + + gzFile rp = gzopen(filename, "rb"); + ASSERT_NE(rp, nullptr); + std::string got(static_cast(gap) + 4, 'x'); + ASSERT_EQ(gzread(rp, &got[0], static_cast(got.size())), + static_cast(got.size())); + EXPECT_EQ(got, "head" + std::string(static_cast(gap), '\0')); + EXPECT_EQ(gzclose(rp), Z_OK); + remove(filename); +} + +// /dev/full accepts an open and fails every write with ENOSPC, which is the one +// way to reach a write failure at close without a filesystem to fill up. +// +// gzclose has to report it. zlib's gzclose_w takes state->err as its return +// value, so a disk-full at close comes back as an error there; dropping the +// return of the zero-fill instead produces a short file and Z_OK, which is +// silent truncation. +// +// The gap is over the shim's 256 KiB write buffer for the same reason as in +// GzflushRejectsBadFlushBeforeFillingASeek: below that, the zero-fill buffers +// cleanly and it is the close's own flush that meets the failure. +TEST_F(GzipFileTest, GzcloseReportsAZeroFillThatCouldNotBeWritten) { + EnableShimOwnedGzWrites(); + + const int fd = open("/dev/full", O_WRONLY); + if (fd == -1) { + GTEST_SKIP() << "/dev/full is not available"; + } + gzFile fp = gzdopen(fd, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, "head", 4), 4); + ASSERT_EQ(gzseek(fp, 4 << 20, SEEK_CUR), (4 << 20) + 4); + EXPECT_NE(gzclose(fp), Z_OK); +} + +// A file that started on an accelerator and then fell back to zlib for the rest +// of its life -- gzsetparams to level 0 is the supported way there. From that +// point zlib does the writing and zlib latches the failures, so the shim's own +// error field stays clean and gzerror would report Z_OK for a write that did +// not happen. Mirroring zlib's latch back is what makes the answer true again. +TEST_F(GzipFileTest, GzerrorReportsAFailedWriteZlibPerformed) { + EnableSomeGzCompressPath(/*zlib_fallback=*/false); + + const int fd = open("/dev/full", O_WRONLY); + if (fd == -1) { + GTEST_SKIP() << "/dev/full is not available"; + } + gzFile fp = gzdopen(fd, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzsetparams(fp, 0, Z_DEFAULT_STRATEGY), Z_OK); + + // Large enough that zlib's own buffer fills and it really writes, rather than + // holding everything until the close. + const std::string payload = PositionStampedPayload(1 << 16); + const int written = + gzwrite(fp, payload.data(), static_cast(payload.size())); + EXPECT_LT(written, static_cast(payload.size())); + + int err = Z_OK; + const char* message = gzerror(fp, &err); + EXPECT_NE(err, Z_OK); + EXPECT_NE(message, nullptr); + EXPECT_NE(gzclose(fp), Z_OK); +} + +// The 32-bit position calls are not casts of the 64-bit ones: zlib narrows with +// a round trip and answers -1 for an offset that does not fit. That is +// unreachable where z_off_t is already 64 bits, so what this pins down is that +// the two families still agree -- the check was added around live code. +TEST_F(GzipFileTest, NarrowAndWidePositionCallsAgree) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(64); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + gzFile fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + EXPECT_EQ(gzseek(fp, 32, SEEK_SET), gzseek64(fp, 32, SEEK_SET)); + char buf[17] = {0}; + ASSERT_EQ(gzread(fp, buf, 16), 16); + EXPECT_EQ(gztell(fp), gztell64(fp)); + EXPECT_EQ(gztell(fp), 48); + EXPECT_EQ(gzoffset(fp), gzoffset64(fp)); + EXPECT_GT(gzoffset(fp), 0); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + // --------------------------------------------------------------------------- // The open-time header look, and the descriptor ownership it settles. // --------------------------------------------------------------------------- diff --git a/zlib_accel.cpp b/zlib_accel.cpp index 80f12d1..940eb0d 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -2158,9 +2158,18 @@ struct GzipFile { // live in io_buf: that is allocated lazily, on the first read. unsigned char peek[2] = {0, 0}; uint8_t peek_len = 0; - // Set when the shim, rather than zlib, ran the header test. This is what - // gzdirect answers from, and gating on it keeps the seekable case exactly as - // it was: there zlib has looked and answers for itself. + // Set at open for a descriptor that cannot be rewound and that an accelerator + // is configured for: the shim, rather than zlib, is going to have to run the + // header test on this file. It says nothing about whether that has happened + // yet, and it stays true for the life of the file, which is what makes it the + // gate gzdirect can be gated on -- the seekable case is untouched, because + // there zlib has looked and answers for itself. + bool shim_must_peek = false; + // Set once that test has actually run, which is the moment the descriptor + // moved. It is deliberately not done at open: zlib performs no I/O until the + // first read, and a read inside gzdopen deadlocks the single-threaded program + // that wraps a pipe's read end before writing to it. So this is also the + // shim's stand-in for zlib's how != LOOK. bool shim_peeked = false; // The peek said this is not a gzip member, so there is nothing to decompress // and the shim copies bytes through -- zlib's COPY mode, for the one case @@ -2215,6 +2224,41 @@ static bool GzReadableAfterError(const GzipFile* gz) { return gz->err == Z_OK || gz->err == Z_BUF_ERROR; } +// zlib's error latch copied into the shim's. +// +// Needed wherever the shim hands a write to zlib and zlib is the one that +// fails. The failure is then recorded in zlib's gz_state, while gzerror below +// answers from the shim's latch for every file the shim registered -- so +// without this the shim goes on reporting Z_OK for a write that did not happen, +// and gzclose says the same. The files at risk are the ones that did not start +// on the zlib path and were moved onto it later: gzsetparams to a level no +// backend can serve, or a mid-write fallback. A file zlib owned from the moment +// it was opened never gets here, because zlib_owns_file makes its gzerror +// delegate. +// +// One direction only. zlib's Z_OK is not copied over the shim's latch, so this +// can add an error but never clear one. +// +// The message is taken verbatim rather than through GzSetError: zlib has +// already prefixed it with the same file name that helper would add. +static void GzMirrorZlibError(gzFile file, GzipFile* gz) { + if (orig_gzerror == nullptr) { + return; + } + int err = Z_OK; + const char* text = orig_gzerror(file, &err); + if (err == Z_OK) { + return; + } + gz->err = err; + try { + gz->msg = text != nullptr ? text : ""; + } catch (...) { + // Only the text is lost; the code is the part callers branch on. + gz->msg.clear(); + } +} + class GzipFiles { public: // Returns the entry it just created, so the caller can finish initializing it @@ -2268,11 +2312,33 @@ static bool GzUncompressAcceleratorSelected() { // This is the second use of the magic-number test in gzread below, not a second // implementation of it. It is also what forces gzdirect to be intercepted: zlib // has not looked, so zlib cannot answer. -static void GzPeekAtOpen(GzipFile* gz) { +// +// Split in two along the line of what needs the descriptor. Deciding *that* the +// shim will have to look costs nothing and is settled at open, below; the +// looking itself waits, because zlib performs no I/O at all until the first +// read and a read from inside gzdopen is a behaviour change with teeth. A +// single-threaded program that wraps a pipe's read end and only then writes to +// it would deadlock in gzdopen, and a non-blocking descriptor would latch +// EAGAIN before the application had asked for anything. +static void GzDecideOwnershipAtOpen(GzipFile* gz) { if (!GzUncompressAcceleratorSelected() || gz->path == ZLIB) { gz->path = ZLIB; return; } + gz->shim_must_peek = true; + // Settled here even though no byte has moved yet. The shim is the only reader + // this descriptor is going to have: it is about to take the header bytes off + // it, and they cannot be put back for zlib to find. + gz->shim_owns_reads = true; +} + +// The look itself, run at the first moment the answer is actually needed -- the +// first read, or an application gzdirect. Idempotent, and a no-op for every +// file that is not the non-rewindable case. +static void GzEnsurePeeked(GzipFile* gz) { + if (!gz->shim_must_peek || gz->shim_peeked) { + return; + } // Loop rather than one read. zlib's gz_load loops until its buffer is full or // the input ends, so zlib always has two bytes to judge a header by; a single @@ -2298,10 +2364,6 @@ static void GzPeekAtOpen(GzipFile* gz) { gz->shim_peeked = true; gz->transparent_read = !(gz->peek_len == 2 && gz->peek[0] == 0x1f && gz->peek[1] == 0x8b); - // Both ways round: bytes have left the descriptor, so zlib must never read it - // again. That is the existing invariant, unchanged in meaning -- only the - // reason the descriptor moved is new. - gz->shim_owns_reads = true; } // Ask zlib, once per read-mode open, whether this file is a gzip member at all. @@ -2332,7 +2394,7 @@ static void GzLookAtOpen(gzFile file, GzipFile* gz) { // gzrewind refuse on. gz->start = lseek(gz->fd, 0, SEEK_CUR); if (gz->start == static_cast(-1)) { - GzPeekAtOpen(gz); + GzDecideOwnershipAtOpen(gz); return; } @@ -2881,6 +2943,15 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { return 0; } + // A write of nothing is where zlib stops: gz_write returns before it + // allocates its buffers and before it fills a pending seek, so neither the + // gzbuffer opportunity below nor the gap is touched. Confirmed in bare zlib, + // no shim: gzwrite(f, "", 0) returns 0 and the gzbuffer after it is still + // accepted. + if (len == 0) { + return 0; + } + // Past every refusal above, so this write is going to happen: from here on // gzbuffer is too late, exactly as it is in zlib. gz->io_started = true; @@ -2958,6 +3029,12 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { if (pinned || configs[USE_ZLIB_COMPRESS]) { gz->path = ZLIB; } + // zlib performed this write, so a failure of it is latched in zlib's state + // and not in this file's. Without copying it across, gzerror and gzclose + // would report Z_OK for data that never reached the file. + if (written_bytes < len) { + GzMirrorZlibError(file, gz.get()); + } } gzwrite_end: @@ -2994,20 +3071,33 @@ int ZEXPORT gzsetparams(gzFile file, int level, int strategy) { return orig_gzsetparams(file, level, strategy); } + // zlib's refusals, in zlib's order: a write-mode file, a clean error latch, + // and not a transparent one. They have to be answered from the shim's own + // state and cannot be delegated, because on this path the shim did the + // writing and so the shim holds the latch -- zlib's own state never saw the + // failure and would answer Z_OK for a file that is broken. Transparency is + // not checked because it is not reachable: a "wT" or level-0 file is pinned + // to ZLIB when it is opened and left above. + // + // Measured in bare zlib 1.3: after a write that failed, gzsetparams returns + // Z_STREAM_ERROR and not the latched code, and it does so even when the level + // asked for is the one the file already has -- the latch is checked before + // the no-change shortcut. + if (!GzIsWriteMode(gz->mode) || gz->err != Z_OK) { + return Z_STREAM_ERROR; + } + // Data still buffered was compressed at the old level, so it has to go out as // a member of its own before the new one takes effect -- and before zlib // records it, because zlib applies a parameter change only once the flush // that precedes it has succeeded. Forwarding first would leave zlib holding // the new level and this file the old one on a flush that fails. // - // Both guards are load-bearing. A file zlib refuses must not be flushed, and - // the mode is the only refusal reachable here: a transparent or level-0 file - // is pinned to ZLIB when it is opened, so it never gets past the check above. - // And zlib skips the flush entirely when the request changes nothing, so - // without the second guard a no-op call could report a failure zlib does not - // have. Only the level is compared, for the reason given below: zlib does not - // flush for a strategy it is not going to act on either. - if (GzIsWriteMode(gz->mode) && level != gz->level) { + // The guard is load-bearing: zlib skips the flush entirely when the request + // changes nothing, so without it a no-op call could report a failure zlib + // does not have. Only the level is compared, for the reason given below -- + // zlib does not flush for a strategy it is not going to act on either. + if (level != gz->level) { // Z_ERRNO is what zlib returns for an error writing the flushed data; a // flush that could not run at all reports itself instead. const int flush_ret = FlushBufferedWrite(file, gz.get()); @@ -3024,6 +3114,9 @@ int ZEXPORT gzsetparams(gzFile file, int level, int strategy) { // the one that compresses if this file is later handed back. const int ret = orig_gzsetparams(file, level, strategy); if (ret != Z_OK) { + // zlib refused, and if the reason was a flush of its own that failed, the + // error is in its latch rather than this file's. + GzMirrorZlibError(file, gz.get()); return ret; } @@ -3062,17 +3155,30 @@ int ZEXPORT gzflush(gzFile file, int flush) { return orig_gzflush != nullptr ? orig_gzflush(file, flush) : Z_STREAM_ERROR; } + // A flush zlib is going to refuse writes nothing at all: zlib checks the + // flush value before it fills a pending seek (gzflush, gzwrite.c). Measured + // in bare zlib, no shim: gzwrite "head", flush, gzseek +4096, then + // gzflush(999) returns Z_STREAM_ERROR and leaves the file at 20 bytes -- the + // next valid flush is what lands the gap, taking it to 45. So the gap is only + // filled once this call is known to be one zlib would have carried out. + const bool flush_is_valid = flush >= 0 && flush <= Z_FINISH; + // A gap left by a forward seek has to be filled before the flush, or it would // land after the data that follows it -- and it has to happen on this side of // the delegation below, because the skip is in the shim's state and zlib's // own flush knows nothing about it. Same reasoning as in GzCloseCommon. - if (gz->pending_skip > 0 && GzIsWriteMode(gz->mode) && gz->err == Z_OK && - GzWriteZeros(file, gz.get()) != 0) { + if (flush_is_valid && gz->pending_skip > 0 && GzIsWriteMode(gz->mode) && + gz->err == Z_OK && GzWriteZeros(file, gz.get()) != 0) { return gz->err; } if (gz->path == ZLIB) { - return orig_gzflush != nullptr ? orig_gzflush(file, flush) : Z_STREAM_ERROR; + const int ret = + orig_gzflush != nullptr ? orig_gzflush(file, flush) : Z_STREAM_ERROR; + // zlib performed the flush, so a failure of it is latched over there and + // this file's own latch would otherwise keep saying Z_OK. + GzMirrorZlibError(file, gz.get()); + return ret; } // zlib's own checks, in zlib's order: a write-mode file and a flush value in @@ -3080,8 +3186,7 @@ int ZEXPORT gzflush(gzFile file, int flush) { // compressed, which is what the flush below does. // A latched error is Z_STREAM_ERROR here, not the latched code itself // (gzflush, gzwrite.c:562). - if (!GzIsWriteMode(gz->mode) || gz->err != Z_OK || flush < 0 || - flush > Z_FINISH) { + if (!GzIsWriteMode(gz->mode) || gz->err != Z_OK || !flush_is_valid) { return Z_STREAM_ERROR; } @@ -3289,10 +3394,28 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { Log(LogLevel::LOG_INFO, "gzread Line ", __LINE__, ", file ", static_cast(file), ", buf ", buf, ", len ", len, "\n"); + // A read of nothing is where zlib stops: gz_read returns before it looks at + // the header, allocates, or serves a pending seek, so the gzbuffer + // opportunity below survives it. Confirmed in bare zlib, no shim: gzread(f, + // buf, 0) returns 0 and the gzbuffer after it is still accepted. + if (len == 0) { + return 0; + } + // Past every refusal above, so this read is going to happen: from here on // gzbuffer is too late, exactly as it is in zlib. gz->io_started = true; + // The header test, for a descriptor that could not be rewound and so had to + // be tested by the shim. This is the first point at which the answer is + // needed and the first at which zlib would have read anything either. It has + // to run before the skip below, which reads through this same function and + // would otherwise decide transparency on the strength of an untested file. + GzEnsurePeeked(gz); + if (!GzReadableAfterError(gz)) { + return -1; + } + // A gzseek that has been promised but not performed: the bytes it moved over // have to be read and discarded before this read can be served. zlib does // this at the same moment, for the same reason -- the skip cannot happen at @@ -3360,7 +3483,7 @@ static int GzreadOwnedFile(gzFile file, GzipFile* gz, voidp buf, unsigned len) { gz->data_buf_size = 512 << 10; gz->io_buf_size = 512 << 10; - // The header bytes GzPeekAtOpen took off a non-seekable descriptor. They + // The header bytes GzEnsurePeeked took off a non-seekable descriptor. They // could not be put back, so they go in at the front of io_buf and the loop // below reads on top of them: read() appends at io_buf_content, so this is // just the buffer starting out holding its first two bytes. They cannot be @@ -3659,6 +3782,13 @@ int ZEXPORT gzungetc(int c, gzFile file) { return -1; } + // zlib's gzungetc runs its header look if nothing has been read yet, which + // allocates and so closes the one-time gzbuffer window. Measured in bare + // zlib, no shim: gzopen then gzungetc then gzbuffer returns -1, where gzopen + // then gzbuffer returns 0. That call arrived in zlib 1.2.12, so it is a + // behaviour to check for rather than to read off an older copy of the source. + gz->io_started = true; + // reached_eof stays as it is: the file itself really has been read to its // end, and that is what stops gzread from reading it again. The end-of-file // indicator is a different fact and this clears it, as zlib's own gzungetc @@ -3796,8 +3926,18 @@ static int GzCloseCommon(gzFile file, FileMode required_mode, // anything -- whichever engine ends up compressing the zeros. And this has to // run before the Unset below, because it writes through gzwrite, which would // otherwise no longer recognize the file and hand it to zlib. - if (gz->pending_skip > 0 && GzIsWriteMode(gz->mode)) { - GzWriteZeros(file, gz.get()); + // + // The result is kept. zlib's gzclose_w records a failed fill in the code it + // returns and closes the file anyway (gzwrite.c), and dropping it here would + // mean a full disk produced a short file and a Z_OK from gzclose -- the one + // report an application has that its data is safe. Weakest of the failures + // below, in zlib's order: anything that goes wrong later replaces it. + int zeros_ret = Z_OK; + if (gz->pending_skip > 0 && GzIsWriteMode(gz->mode) && + GzWriteZeros(file, gz.get()) != 0) { + // gzwrite latched the reason on the way out; Z_ERRNO is the fallback for a + // failure that somehow left no latch, since gzwrite is what failed. + zeros_ret = gz->err != Z_OK ? gz->err : Z_ERRNO; } // Unregister up front, before orig_close frees the gzFile. Unsetting after @@ -3810,7 +3950,7 @@ static int GzCloseCommon(gzFile file, FileMode required_mode, static_cast(file), ", buffered ", gz->data_buf_content, ", path ", static_cast(gz->path), "\n"); - int ret = 0; + int ret = zeros_ret; if (gz->path != ZLIB && (gz->mode == FileMode::WRITE || gz->mode == FileMode::APPEND)) { // Compress any remaining buffered data. @@ -3823,7 +3963,17 @@ static int GzCloseCommon(gzFile file, FileMode required_mode, readlink(("/proc/self/fd/" + std::to_string(gz->fd)).c_str(), file_path, MAXPATHLEN - 1); if (readlink_ret == -1) { - ret = orig_close(file); + // Same precedence as the chain below, so that bailing out here does not + // silently drop a failed flush or a failed gap fill: the close reports + // itself if it failed, otherwise whichever write failure came first. + const int close_ret = orig_close(file); + if (close_ret != Z_OK) { + ret = close_ret; + } else if (write_ret == Z_STREAM_ERROR) { + ret = Z_STREAM_ERROR; + } else if (write_ret != 0) { + ret = Z_ERRNO; + } Log(LogLevel::LOG_ERROR, "GzCloseCommon Line ", __LINE__, ", readlink_ret return error \n"); return ret; @@ -3852,8 +4002,12 @@ static int GzCloseCommon(gzFile file, FileMode required_mode, } else if (truncate_ret != 0) { ret = Z_ERRNO; } + // Nothing later went wrong, so ret keeps whatever the gap fill left in it. } else { - ret = orig_close(file); + const int close_ret = orig_close(file); + if (close_ret != Z_OK) { + ret = close_ret; + } } Log(LogLevel::LOG_INFO, "GzCloseCommon Line ", __LINE__, ", file ", static_cast(file), ", return code ", ret, ", buffered processed ", @@ -3902,15 +4056,21 @@ int ZEXPORT gzeof(gzFile file) { // right then, pulling up to 8 KB out of the pipe into zlib's private buffer and // punching a hole in the front of the shim's input. // -// Everything else delegates, which is the whole point of the shim_peeked gate. -// A seekable file already has zlib's own cached answer, and it is the true one -// in all four cases (see GzLookAtOpen); a write-mode file keeps whatever zlib -// reports for "wT". Neither is touched here. +// Everything else delegates, which is the whole point of the shim_must_peek +// gate. A seekable file already has zlib's own cached answer, and it is the +// true one in all four cases (see GzLookAtOpen); a write-mode file keeps +// whatever zlib reports for "wT". Neither is touched here. +// +// The gate is shim_must_peek and not shim_peeked because the peek is lazy: on a +// file the shim owns the test may not have run yet, and this call is one of the +// two things that makes it run. zlib's gzdirect looks for the same reason, so +// the I/O this does is I/O zlib would also have done. int ZEXPORT gzdirect(gzFile file) { auto gz = gzip_files.Get(file); - if (gz == nullptr || !gz->shim_peeked) { + if (gz == nullptr || !gz->shim_must_peek) { return orig_gzdirect != nullptr ? orig_gzdirect(file) : 0; } + GzEnsurePeeked(gz.get()); return gz->transparent_read ? 1 : 0; } @@ -3929,6 +4089,21 @@ int ZEXPORT gzdirect(gzFile file) { // complete and correct one and these entry points delegate untouched. // --------------------------------------------------------------------------- +// Every one of zlib's three plain-int position functions is its 64-bit +// counterpart with the result put through this exact test (gzseek, gztell and +// gzoffset in gzlib.c): an answer that does not survive the narrowing is not +// reported as a smaller number, it is reported as a failure. A bare cast would +// hand back a truncated offset instead, and the plain and 64-bit entry points +// would then disagree about the same file. +// +// On a build where z_off_t is already 64 bits this never fires, which is +// exactly why it has to be written down rather than left to the cast: the +// platform that needs it is not the one this is developed on. +static z_off_t GzNarrowOffset(z_off64_t value) { + return value == static_cast(value) ? static_cast(value) + : -1; +} + // zlib's gzrewind is gz_reset plus an lseek back to where the file was opened // (gzlib.c:361), so this has to put back the same set of things: the // descriptor, both buffers, the position, the error latch and the inflate @@ -4021,10 +4196,16 @@ static z_off64_t GzSeekOwned(GzipFile* gz, z_off64_t offset, int whence) { // after the first read how is COPY, so the lseek is attempted, and a pipe // refuses it // - // io_started is the shim's equivalent: it flips at the top of the first read, - // the same moment zlib's look runs. Measured rather than assumed -- the - // conformance suite's O11/O12 pipe seeks are what turned this asymmetry up. - if (gz->transparent_read && gz->io_started && gz->pos + offset >= 0) { + // transparent_read carries that distinction on its own, because the header + // test is lazy: it is only ever set by GzEnsurePeeked, so it cannot be true + // before the test has run, and the test runs at exactly the moments zlib's + // look does -- the first read, or an application gzdirect. So a false here + // means "zlib is still in LOOK" and the lazy path below is the matching one. + // It is deliberately not io_started, which is now a different event: gzdirect + // moves zlib to COPY without any application-visible I/O, and gating on + // io_started would take the lazy path there and disagree. Measured rather + // than assumed -- the conformance suite's O11/O12 pipe seeks turned this up. + if (gz->transparent_read && gz->pos + offset >= 0) { const off_t held = static_cast(gz->peek_len) + static_cast(gz->pushback.size()); if (lseek(gz->fd, static_cast(offset) - held, SEEK_CUR) == @@ -4081,7 +4262,7 @@ z_off_t ZEXPORT gzseek(gzFile file, z_off_t offset, int whence) { return orig_gzseek != nullptr ? orig_gzseek(file, offset, whence) : -1; } // zlib's gzseek is gzseek64 with the result narrowed the same way (gzlib.c). - return static_cast(GzSeekOwned(gz.get(), offset, whence)); + return GzNarrowOffset(GzSeekOwned(gz.get(), offset, whence)); } z_off64_t ZEXPORT gztell64(gzFile file) { @@ -4103,7 +4284,7 @@ z_off_t ZEXPORT gztell(gzFile file) { if (gz->mode == FileMode::NONE) { return -1; } - return static_cast(gz->pos + gz->pending_skip); + return GzNarrowOffset(gz->pos + gz->pending_skip); } // Where the *compressed* file is positioned, which zlib reports as the @@ -4138,7 +4319,7 @@ z_off_t ZEXPORT gzoffset(gzFile file) { if (gz == nullptr || gz->zlib_owns_file) { return orig_gzoffset != nullptr ? orig_gzoffset(file) : -1; } - return static_cast(GzOffsetOwned(gz.get())); + return GzNarrowOffset(GzOffsetOwned(gz.get())); } const char* ZEXPORT gzerror(gzFile file, int* errnum) { From 3f576f36667efcbed9a98fb06465ea40b79e69c9 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Fri, 4 Sep 2026 13:32:09 -0700 Subject: [PATCH 7/9] gz: reject -D_FILE_OFFSET_BITS=64 for the shim, and assert gz linkage in CI Compiling zlib_accel.cpp with -D_FILE_OFFSET_BITS=64 breaks the build with four redefinition errors, as reported in review: zlib_accel.cpp:2601: redefinition of gzFile_s* gzopen64(const char*, const char*) zlib_accel.cpp:4256: redefinition of off_t gzseek64(gzFile, off_t, int) ... and the same for gztell64 and gzoffset64 zlib offers two large-file mechanisms and they are not interchangeable. _LARGEFILE64_SOURCE is additive: zconf.h:506 derives Z_LARGE64 and zlib.h declares gzopen64/gzseek64/gztell64/gzoffset64 next to the plain names. _FILE_OFFSET_BITS=64 is substitutive: zconf.h:510 derives Z_WANT64 and zlib.h:1868 renames the plain names into the *64 names, skipping the branch that would have declared the plain prototypes. This file has to DEFINE both sets as separate functions -- an application built either way has to reach the shim -- so the rename collapses each pair onto one symbol. The substitutive macro is therefore wrong for this translation unit and right for an application, which is a distinction worth stating where it is enforced rather than leaving to a comment. The guard tests Z_WANT64 rather than _FILE_OFFSET_BITS directly: Z_WANT64 is the macro zconf.h derives, so it is true exactly when the rename is about to happen, and it also covers the Z_PREFIX_SET spelling at zlib.h:1870. Applications are unaffected, and that is checked: a probe built with -D_FILE_OFFSET_BITS=64 calls gzopen64/gzseek64/gztell64/gzoffset64 and LD_DEBUG=bindings shows all four binding to the shim, which forwards them to libz. The comment above the #define is extended to say which mechanism is which, and corrected on a point it previously overstated. What is at stake is linkage, not only offset width: there is no extern "C" anywhere in this file, so every symbol gets C linkage by matching a declaration zlib.h already made, and an interceptor zlib.h does not declare is emitted as ordinary C++ -- _Z8gzseek64P8gzFile_sli -- which LD_PRELOAD cannot interpose, with nothing failing to compile or link. But the #define is not what prevents that here. On glibc, features.h defines _LARGEFILE64_SOURCE itself whenever _GNU_SOURCE is set, every C++ front end predefines _GNU_SOURCE, and features.h is reached before zconf.h tests the macro, so Z_LARGE64 is on either way. Verified, including that an explicit #undef in this file does not stick because features.h re-establishes it afterwards. The line stays: the requirement belongs in the file that has it instead of resting on a front end's choice of feature macros, and a C translation unit would genuinely need it, since gcc -x c leaves the macro unset. So the silent-mangling state is not reachable through the LFS macros, but it is reachable by adding or renaming an interceptor, and it would pass review, compile, link and any unit test that calls the function directly. The new CI step asserts the invariant on the built library instead: all eight large-file entry points present as unmangled T symbols, and no gz symbol mangled at all. On the current tree it reports 32 gz symbols with C linkage, which is the existing count, so this is an assertion on correct output rather than a change to it. The blanket pattern cannot match the three intentionally-C++ test hooks, which have no lowercase "gz" directly after the length digits. Tests: unchanged, 3285 passed and the same 5 skipped before and after. Signed-off-by: Olasoji --- .github/workflows/tests.yml | 28 +++++++++++++++++++++- zlib_accel.cpp | 47 +++++++++++++++++++++++++++++++------ 2 files changed, 67 insertions(+), 8 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8246b95..fc3f866 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -43,7 +43,33 @@ jobs: cd build cmake -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ .. make - + + # The shim's exported symbols get C linkage only by matching a declaration + # zlib.h already made; there is no extern "C" in zlib_accel.cpp. An + # interceptor zlib.h does not declare is therefore compiled as ordinary C++, + # emitted under a mangled name, and silently not interposed by LD_PRELOAD -- + # it still compiles, still links, and still passes every unit test that calls + # it directly. Adding or renaming an interceptor is the way to reach that, so + # this asserts the invariant on the built library rather than trusting review. + # The eight named below are the large-file pairs the surrounding LFS logic + # turns on; the blanket pattern covers the rest. It cannot match the + # intentionally-C++ test hooks (DeflateOwnsIgzipState and friends), which have + # no lowercase "gz" directly after the length digits. + - name: Check gz symbol export and linkage + run: | + pwd + cd build + nm -D --defined-only libzlib-accel.so > /tmp/gzsyms.txt + for s in gzopen gzopen64 gzseek gzseek64 gztell gztell64 gzoffset gzoffset64; do + grep -qE " T $s\$" /tmp/gzsyms.txt \ + || { echo "MISSING or non-C symbol: $s"; exit 1; } + done + if grep -E '_Z[0-9]+gz' /tmp/gzsyms.txt; then + echo "C++-mangled gz symbol above: LD_PRELOAD cannot interpose it" + exit 1 + fi + echo "gz symbols with C linkage: $(grep -cE ' T gz' /tmp/gzsyms.txt)" + - name: Build tests run: | pwd diff --git a/zlib_accel.cpp b/zlib_accel.cpp index 940eb0d..d7723b4 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -1,17 +1,50 @@ // Copyright (C) 2025 Intel Corporation // SPDX-License-Identifier: Apache-2.0 -// libz exports gzseek64, gztell64 and gzoffset64 alongside their plain-named -// counterparts, and an application built with -D_FILE_OFFSET_BITS=64 calls the -// 64-bit names, so the shim has to define both sets or the pair would disagree -// about where a file is positioned. zlib.h only declares them when asked -// (zconf.h:506), and this is the ask. Note it is _LARGEFILE64_SOURCE, not -// _FILE_OFFSET_BITS: the latter would additionally rename gzseek to gzseek64 -// here and leave the plain names undefined. +// libz exports gzopen64, gzseek64, gztell64 and gzoffset64 alongside their +// plain-named counterparts, and an application built with +// -D_FILE_OFFSET_BITS=64 calls the 64-bit names, so the shim has to define both +// sets or the pair would disagree about where a file is positioned. zlib.h only +// declares them when asked (zconf.h:506), and this is the ask. +// +// zlib offers two large-file mechanisms and they are not interchangeable, which +// zlib.h says in its own words at the top of the block: "provide 64-bit offset +// functions if _LARGEFILE64_SOURCE defined, and/or change the regular functions +// to 64 bits if _FILE_OFFSET_BITS is 64". _LARGEFILE64_SOURCE is *additive*: it +// declares the *64 names next to the plain ones. _FILE_OFFSET_BITS=64 is +// *substitutive*: it renames the plain names into the *64 names and skips the +// branch that would have declared the plain prototypes. This file has to DEFINE +// both sets as separate functions, so only the additive one can be used here. +// +// What is at stake is linkage, not only offset width. There is no extern "C" +// anywhere in this file: every symbol gets C linkage by matching a declaration +// zlib.h already made inside its own extern "C" block. An interceptor that +// zlib.h does not declare is compiled as ordinary C++, comes out as (for +// gzseek64) _Z8gzseek64P8gzFile_sli, and cannot be interposed by LD_PRELOAD -- +// with no compile error, no link error and no sign at runtime. +// +// The line below is not what prevents that here, and the note is worth leaving +// rather than implying otherwise: on glibc, features.h defines +// _LARGEFILE64_SOURCE itself whenever _GNU_SOURCE is set, every C++ front end +// predefines _GNU_SOURCE, and features.h is reached before zconf.h tests the +// macro. So Z_LARGE64 is on either way -- checked, including that an explicit +// #undef here does not stick, because features.h re-establishes it afterwards. +// The line stays because the requirement belongs in the file that has it +// instead of resting on a C++ front end's choice of feature macros, and because +// a C translation unit would genuinely need it (gcc -x c leaves +// _LARGEFILE64_SOURCE unset). #define _LARGEFILE64_SOURCE 1 #include "zlib_accel.h" +// Guarding on Z_WANT64 rather than on _FILE_OFFSET_BITS directly: Z_WANT64 is +// what zconf.h:510 derives, so it is true exactly when the rename is about to +// happen, and it also covers the Z_PREFIX_SET spelling at zlib.h:1870. +#ifdef Z_WANT64 +#error \ + "zlib_accel.cpp must not be compiled with -D_FILE_OFFSET_BITS=64. zlib.h then renames gzopen to gzopen64 (and gzseek, gztell, gzoffset likewise), and this file has to DEFINE both names, so the rename collapses each pair onto one symbol. Large-file support for the shim comes from _LARGEFILE64_SOURCE above, which declares both sets without renaming either. Applications may use _FILE_OFFSET_BITS freely -- that is what the gzopen64/gzseek64/gztell64/gzoffset64 interceptors are for." +#endif + #include #include #include From 3f840d65e4599645076deb0ccf2cd937c5937a45 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Fri, 4 Sep 2026 13:52:11 -0700 Subject: [PATCH 8/9] gz: refuse gzwrite on a read-mode file and gzread on a write-mode one Reported in review of PR #74 against gzwrite: "Bug: writes into a file that is opened in read-only mode." Reproduced with the reviewer's own program on a build with no accelerator compiled in -- bare zlib returns 0 and then reads back "hello world"; the shim returned 4 and then read back "XXXXhello world". The cause is where the predicate lived, not that anyone forgot it. GzIsWriteMode was defined below gzwrite, so the six write entry points underneath it all guarded on it and gzwrite -- the only one above -- could not. It is now defined next to the FileMode enum it tests, above every caller, and gzwrite guards on it. Past the missing guard the accelerator path allocated gz->data_buf and copied into it, and that buffer is shared with the read path, so the four bytes came back out of the next gzread ahead of the file's own contents. Placing the check ahead of the length check, which is zlib's own order (mode at gzwrite.c:249, length at :252), also drops a Z_DATA_ERROR the shim used to latch on an oversized write to a read-mode file where zlib latches nothing. gzread had the mirror-image hole, and it was worse. gzgetc, gzungetc, gzgets and gzfread all check the mode; gzread did not. What that cost depends on how much the write had buffered, and both halves were measured under LD_PRELOAD: * asked for less than is buffered, the read was served out of the buffer the write is still filling, so the application got its own pending output back as if it were file content -- gzread(g, b, 4) returned 4 and "hell" after gzwrite(g, "hello world", 11). * asked for more, the buffer ran out, the read reached the descriptor, and read(2) on a write-only fd failed with EBADF. The Z_ERRNO that latched then made gzclose write nothing at all and still return Z_OK: a 31-byte file became a 0-byte file with no error reported to the application. zlib returns -1 there and latches nothing (gzread.c:378), which is what gzread now does. Tests: two cases in GzipFileTest, one per direction, both covering the return value, the absence of a latch, and what the following call sees -- which is the part that failed. The write-side case also asserts that an oversized write to a read-mode file leaves no Z_DATA_ERROR behind. Both were confirmed by mutation: with either guard deleted the matching case fails. Worth recording that the write-side case needs EnableShimOwnedGzWrites rather than EnableSomeGzCompressPath, because the latter clears every compress flag on a build with no backend and the call then goes straight to zlib, never reaching the branch that was wrong -- the test passed against the unguarded code until that was fixed. The differential suite against bare zlib had covered one direction of this pair and not the other: it asked what the read family does to a write-mode file and nothing asked the reverse. It now has both, and the write-mode-file check grew a payload and a round-trip, because return values alone could not see either failure above. Divergences from bare zlib across 49 checks: 3 before (W8, R15 and the R11 corrupt-byte check that is documented as not comparable), 1 after. Existing suite unchanged at 0 failures and the same 5 skips; the only new results are the two above. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 109 ++++++++++++++++++++++++++++++++++++++ zlib_accel.cpp | 35 ++++++++++-- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index d2ee2c9..56713c0 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8093,6 +8093,115 @@ static void EnableShimOwnedGzReads() { SetConfig(USE_ZLIB_UNCOMPRESS, 1); } +// The two mode-mismatch refusals. They live here rather than beside the other +// head-of-gzwrite checks because reaching the code that used to be wrong needs +// the file on the shim's route, and the helper that arranges that is the one +// directly above. +// +// A write to a read-mode file: zlib refuses it and returns 0, leaving nothing +// latched. The shim used to accept it, and the cost was not the return value -- +// the buffer it copies into is the same one the read path serves out of, so the +// four bytes came back from the next gzread ahead of the file's own contents. +// That is what the third assertion covers, and it is the one that failed. +TEST_F(GzipFileTest, GzwriteOnAReadModeFileIsRefusedAndChangesNothing) { + // Both halves are load-bearing, and EnableSomeGzCompressPath is not enough + // for the first: on a build with no backend compiled in it clears every + // compress flag, so gzwrite hands the call straight to zlib and never reaches + // the branch that was wrong. The flag has to be set for the shim to keep the + // write, and the read file has to be the shim's for gz->path to be anything + // other than ZLIB. Checked by mutation -- with EnableSomeGzCompressPath here, + // this test passes even with the guard deleted. + EnableShimOwnedGzWrites(); + EnableShimOwnedGzReads(); + + const char* payload = "hello world"; + const unsigned payload_length = 11; + const char* filename = "file.gz"; + remove(filename); + + gzFile fp = gzopen(filename, "wb"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, payload, payload_length), + static_cast(payload_length)); + ASSERT_EQ(gzclose(fp), Z_OK); + + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + + EXPECT_EQ(gzwrite(fp, "XXXX", 4), 0); + + // Nothing latched. zlib tests the mode ahead of the length, so an oversized + // write to a read-mode file is refused on the mode and never reaches the + // length check that would have latched Z_DATA_ERROR. + int err = Z_OK; + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + EXPECT_EQ( + gzwrite(fp, "XXXX", + static_cast(std::numeric_limits::max()) + 1u), + 0); + err = Z_OK; + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + + // The refused writes left the read untouched: the file's own first byte is + // still the first byte served, and the position is still zero. + EXPECT_EQ(gztell(fp), static_cast(0)); + char out[32] = {0}; + EXPECT_EQ(gzread(fp, out, sizeof(out) - 1), static_cast(payload_length)); + EXPECT_STREQ(out, payload); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// The mirror, which gzread was missing for the same reason: the four other read +// entry points guard on the mode and it did not. zlib answers -1 and latches +// nothing, and the buffered write it was refused against must survive intact. +TEST_F(GzipFileTest, GzreadOnAWriteModeFileIsRefused) { + EnableShimOwnedGzWrites(); + EnableShimOwnedGzReads(); + + const char* payload = "hello world"; + const unsigned payload_length = 11; + const char* filename = "file.gz"; + remove(filename); + + gzFile fp = gzopen(filename, "wb6"); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(gzwrite(fp, payload, payload_length), + static_cast(payload_length)); + + // Two lengths, because the unguarded call went wrong in two different ways + // depending on how much the write had buffered. Asking for less than is + // buffered was served out of the write's own buffer -- the application got + // its pending output back as if it were file content. Asking for more ran the + // buffer out, reached the descriptor, and read(2) on a write-only fd failed + // with EBADF; the Z_ERRNO that latched then made gzclose write nothing while + // still returning Z_OK, which is why the round-trip below is part of the + // test. + char out[32] = {0}; + EXPECT_EQ(gzread(fp, out, 4), -1); + EXPECT_STREQ(out, ""); + EXPECT_EQ(gzread(fp, out, sizeof(out) - 1), -1); + // gzgetc reads through gzread, so it has to answer the same way. + EXPECT_EQ(gzgetc(fp), -1); + int err = Z_OK; + gzerror(fp, &err); + EXPECT_EQ(err, Z_OK); + + ASSERT_EQ(gzclose(fp), Z_OK); + + // The refusals did not disturb what was still buffered for the write. + fp = gzopen(filename, "rb"); + ASSERT_NE(fp, nullptr); + memset(out, 0, sizeof(out)); + EXPECT_EQ(gzread(fp, out, sizeof(out) - 1), static_cast(payload_length)); + EXPECT_STREQ(out, payload); + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + // Sixteen bytes per record, each naming its own offset. A repeating payload // would let a read from the wrong offset look correct -- which is how the // original gzseek check passed while the shim was reading from byte 0. diff --git a/zlib_accel.cpp b/zlib_accel.cpp index d7723b4..691af9b 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -2016,6 +2016,15 @@ bool InflateOwnsIgzipState(z_streamp strm) { enum class FileMode { NONE, READ, WRITE, APPEND }; +// Beside the enum rather than beside its first caller. Every gz entry point +// that writes has to refuse a read-mode file, and the topmost of them is +// gzwrite -- which is how gzwrite came to be the one that did not: this +// predicate used to be defined below it, so it was the only write entry point +// that could not reach it. +static bool GzIsWriteMode(FileMode mode) { + return mode == FileMode::WRITE || mode == FileMode::APPEND; +} + // What a gzopen/gzdopen mode string asks for beyond the open(2) flags. zlib // parses all of this out of the same string in gz_open(), so the shim has to as // well or it acts on settings the application asked for and zlib recorded. @@ -2970,6 +2979,20 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { return 0; } + // A read-mode file. zlib refuses one before it does anything else, in the + // same condition as the error latch below (gzwrite.c:249), and returns 0 + // without touching the file or that latch -- so an application ignoring the + // return value sees nothing change. The shim has more at stake than zlib + // does: past this point the accelerator path allocates gz->data_buf and + // memcpys into it, and that buffer is shared with the read path, so the + // written bytes come back out of the next gzread ahead of the file's own + // content. Refusing here, ahead of the length check further down, also drops + // a Z_DATA_ERROR that zlib never latches -- zlib tests the mode first and the + // length second (gzwrite.c:252). + if (!GzIsWriteMode(gz->mode)) { + return 0; + } + // The write side demands a clean latch, where the read side tolerates // Z_BUF_ERROR (gz_write, gzwrite.c:249). if (gz->err != Z_OK) { @@ -3083,10 +3106,6 @@ int ZEXPORT gzwrite(gzFile file, voidpc buf, unsigned len) { return written_bytes; } -static bool GzIsWriteMode(FileMode mode) { - return mode == FileMode::WRITE || mode == FileMode::APPEND; -} - // Without interception the level the application asks for here is recorded by // zlib and honored by nobody: the shim compresses through its own streams, so // subsequent writes keep the level the file was opened with. Same class of @@ -3745,6 +3764,14 @@ int ZEXPORT gzread(gzFile file, voidp buf, unsigned len) { return orig_gzread != nullptr ? orig_gzread(file, buf, len) : -1; } + // The mirror of the check in gzwrite. gzgetc, gzungetc, gzgets and gzfread + // all make it and gzread did not, which is the same one-sided gap gzwrite had + // among the write entry points. zlib refuses a write-mode file here too, in + // the same condition as the error latch below (gzread.c:378), returning -1. + if (gz->mode != FileMode::READ) { + return -1; + } + // A latched error refuses the read before anything is served, including bytes // waiting in push-back, which is the order zlib checks in. if (!GzReadableAfterError(gz.get())) { From 46dbdb8ae95d0eaa79942abec4ed4aa88fd45c95 Mon Sep 17 00:00:00 2001 From: Olasoji Date: Fri, 4 Sep 2026 14:13:26 -0700 Subject: [PATCH 9/9] gz: run the open-time header test in the shim instead of borrowing zlib's Review comment on gzbuffer asked whether a zlib-owned file should delegate too. It should, and it could not, because of what the shim was doing at open. gzopen and gzdopen called zlib's own gzdirect() to find out whether the file was a gzip member. That call runs gz_look, and gz_look allocates -- and having allocated is the only thing zlib's gzbuffer tests (gzlib.c:299). So from open onwards zlib refused every size an application asked for, the first one included. Confirmed against bare zlib with no shim loaded: gzbuffer returns 0 after a gzopen and -1 after a gzdirect on the same file. Delegating to it would have answered -1 where bare zlib answers 0. The shim was also picking, at open, which buffer size zlib would be stuck with for the life of the file, on behalf of a caller who had not spoken yet -- 512 bytes, chosen to hold the allocation down. So the shim asks the question itself, with the two-byte magic test it already had for pipes, and leaves zlib untouched: nothing allocated, how still LOOK, start still right. Two bytes is not a shortcut -- a gzip header is identified by its first two bytes and gz_look tests exactly those. The rest of zlib's read was it filling its input buffer at the same time, which is read-ahead the shim threw away on every file it went on to own. The peek is at open where the descriptor can be seeked, because there the bytes go back. On one that cannot it stays deferred to the first read, unchanged: zlib performs no I/O inside gzopen, and an open-time read would deadlock a single-threaded program that wraps a pipe's read end before writing to it, or latch EAGAIN on a non-blocking descriptor before the application asked for anything. Every gz entry point then draws the same line. gzbuffer and gzdirect gate on ownership, gzbuffer taking the reviewer's condition as written. A seekable file with no gzip header goes to zlib at open -- which is a better reader for it than the shim, at its caller's buffer size and with no shim buffers allocated. An unseekable one keeps the shim's copy-through, because its header bytes cannot be put back. Two things have to differ between the eager caller and the lazy one, and both would be silent if wrong. The peek bytes: a pipe keeps them for the read loop, a seekable file must clear peek_len after seeking back, or the read path seeds io_buf from gz->peek and inflate is handed the header twice. And the error latch: a pipe's peek runs where zlib would have reported the errno, so the latch is kept; a seekable file's runs at open, where bare zlib reports nothing, so it is cleared and the file handed to zlib to meet the same error at the first read. That is why the clearing is in GzPeekAtOpen and not in GzEnsurePeeked. One case is handled differently from how it used to be: an lseek back that fails after the SEEK_CUR succeeded. It now becomes the pipe case -- start stays -1 and the shim keeps the bytes it has -- rather than going to zlib, which would have started the file two bytes in. Measured, LD_PRELOAD, before and after: gzip file the shim owns 512-byte read at open 2-byte read + one lseek heap per open read file 284,918 bytes 276,190 bytes (-8,728) plain file, gzbuffer(1M) read 512 at a time 1048576/1041606, as bare plain file offset at open 512 0, as bare The plain-file arm of gzbuffer_hazard_gzip now takes 0.047s where it took 0.005s, and bare zlib takes 0.043s: honouring the size means honouring an 8-byte one too. A gzip file the shim reads still ignores the size, and that divergence is unchanged and deliberate -- the shim's buffers are fixed. Differential conformance suite against bare zlib, two new checks added for the two populations gzbuffer answers for (P8 plain via gzdopen, P9 gzip via gzdopen), 52 checks total: 3 divergent checks before, 2 after. P8 stops diverging; R11 is the documented pre-existing one; P9 is the intended difference, the shim not needing zlib's look. ASAN arm clean and identical. Unit suite 3290 passed, 5 skipped as before, with three new cases. Also rewrites the six comments that described the borrowed look, including the one above gzdirect that stated the dependency this removes -- left alone it would have talked the next reader into putting the look back. Signed-off-by: Olasoji --- tests/zlib_accel_test.cpp | 230 +++++++++++++++++++++---- zlib_accel.cpp | 349 +++++++++++++++++++++----------------- 2 files changed, 388 insertions(+), 191 deletions(-) diff --git a/tests/zlib_accel_test.cpp b/tests/zlib_accel_test.cpp index 56713c0..828f7ea 100644 --- a/tests/zlib_accel_test.cpp +++ b/tests/zlib_accel_test.cpp @@ -8270,11 +8270,12 @@ TEST_F(GzipFileTest, Gzopen64RegistersTheFileWithTheShim) { // --------------------------------------------------------------------------- // Non-seekable descriptors. // -// A pipe cannot be rewound, so the shim cannot borrow zlib's header look for -// it: the 8 KB zlib reads to do the look would be stranded in zlib's private -// buffer with the shim reading the descriptor from behind them. The shim -// therefore takes two bytes of its own and keeps them. Every test below writes -// far less than a pipe's 64 KiB capacity, so nothing blocks on an unread pipe. +// The shim runs its own two-byte header test on every file it might read. A +// descriptor that can be seeked gets those two bytes put back afterwards; a +// pipe cannot, so it keeps them and hands them to whoever reads next. That one +// difference is what this section is about, and it has consequences all the way +// out to gzseek. Every test below writes far less than a pipe's 64 KiB +// capacity, so nothing blocks on an unread pipe. // Fills a pipe with plain bytes and returns the read end, write end closed. static int PipeOfPlainBytes(const std::string& bytes) { @@ -8471,11 +8472,12 @@ TEST_F(GzipFileTest, ShortAndEmptyPipesAreTransparent) { EXPECT_EQ(gzclose(empty), Z_OK); } -// gzdirect is gated on the shim having done the peek, so a seekable file keeps -// answering from zlib's own cached look exactly as before. Both answers, -// because delegating the wrong way round would be invisible in only one of -// them. -TEST_F(GzipFileTest, GzdirectOnSeekableFilesStillComesFromZlib) { +// gzdirect is gated on who owns the file, and a seekable file can be either, so +// the two answers come from two different places. The gzip file is the shim's +// and answers from the shim's own peek; the plain one was handed to zlib at +// open and answers from zlib's look. Both, because delegating the wrong way +// round would be invisible in only one of them. +TEST_F(GzipFileTest, GzdirectOnSeekableFilesAnswersFromWhoeverOwnsThem) { EnableSomeGzCompressPath(); EnableShimOwnedGzReads(); @@ -8813,9 +8815,11 @@ TEST_F(GzipFileTest, GzclearerrClearsEndOfFile) { } // gzbuffer takes a size only before any reading or writing, because that is -// when zlib would still be allocating. The shim has to answer from its own -// state: the open-time header look makes zlib allocate, so zlib's own gzbuffer -// would refuse even the first call. +// when zlib would still be allocating. On a file the shim reads, zlib never +// allocates at all, so its answer would be "yes" forever; the shim has to +// replicate the refusal against its own state. The size itself is accepted and +// dropped -- the shim's buffers are fixed. A file zlib owns is the other case +// and delegates instead; see GzbufferOnAPlainFileIsHonouredByZlib. TEST_F(GzipFileTest, GzbufferAcceptsOnlyBeforeAnyIo) { EnableSomeGzCompressPath(); EnableShimOwnedGzReads(); @@ -8849,12 +8853,13 @@ TEST_F(GzipFileTest, GzbufferAcceptsOnlyBeforeAnyIo) { } // --------------------------------------------------------------------------- -// The header test happens on demand, not at open. +// On a descriptor that cannot be seeked, the header test waits. // -// zlib performs no I/O at all inside gzopen or gzdopen: it decides nothing -// about the file until the first read. Reading two bytes at open to run the -// shim's own header test broke that in two visible ways, and these are those -// two ways. +// Everywhere else the shim runs it at open, where the two bytes can be put +// back. On a pipe they cannot, and reading them early costs something zlib +// never charges: zlib performs no I/O at all inside gzopen or gzdopen and +// decides nothing about the file until the first read. These are the two ways +// that showed up when the peek was done at open for pipes as well. // --------------------------------------------------------------------------- // gzdopen on a pipe that is empty but still has a writer must return, because a @@ -8925,9 +8930,10 @@ TEST_F(GzipFileTest, GzdopenOnANonBlockingPipeLatchesNoError) { int err = Z_OK; gzerror(fp, &err); EXPECT_EQ(err, Z_OK); - // Nothing has been read, so the gzbuffer opportunity is still open. This is - // the same fact from the other side: zlib refuses gzbuffer once it has - // allocated, and it allocates when it looks. + // Nothing has been read, so the gzbuffer opportunity is still open -- the + // same fact from the other side. zlib refuses gzbuffer once it has allocated, + // and an open-time read on this descriptor is exactly what would have made + // it. EXPECT_EQ(gzbuffer(fp, 8192), 0); // The whole member goes in before the first read, so no read below meets an @@ -9172,7 +9178,7 @@ TEST_F(GzipFileTest, NarrowAndWidePositionCallsAgree) { } // --------------------------------------------------------------------------- -// The open-time header look, and the descriptor ownership it settles. +// The open-time header test, and the descriptor ownership it settles. // --------------------------------------------------------------------------- // A file that is not a gzip member at all. zlib reads it straight through; the @@ -9259,19 +9265,22 @@ TEST_F(GzipFileTest, GzreadIgnoresATrailerThatIsNotAMember) { remove(filename); } -// The invariant the header look depends on, and the one thing here that fails -// silently rather than loudly if it is wrong. +// The invariant the whole ownership split depends on, and the one thing here +// that fails silently rather than loudly if it is wrong. // -// The look rewinds the descriptor, which leaves zlib holding up to 8 KB of -// input read from a position the file is no longer at. Measured with plain zlib -// and no shim: asking zlib to read such a file returns 51,456 bytes of -// duplicates and then Z_DATA_ERROR. So a rewound file must never reach -// orig_gzread -- and the branch that would send it there is chosen per call, -// from the configuration, not per file. +// Once the shim has read part of a file, zlib cannot be handed the rest. zlib +// has read nothing and so is still expecting a gzip header, and the descriptor +// is somewhere in the middle of the compressed stream; it would try to parse +// deflate output as a header and fail, or worse. So a file the shim has started +// must never reach orig_gzread -- and the branch that would send it there is +// chosen per call, from the configuration, not per file. // // Turning every uncompress flag off mid-read is the config change that used to // flip that branch. The file has to keep reading correctly through it: which -// engine decompresses may change, but not who reads the descriptor. +// engine decompresses may change, but not who reads the descriptor. Measured +// against the version of this that borrowed zlib's header look, where zlib was +// left holding up to 8 KB of stale input as well: reading such a file through +// zlib returned 51,456 bytes of duplicates and then Z_DATA_ERROR. TEST_F(GzipFileTest, ConfigChangeMidReadDoesNotHandARewoundFileToZlib) { EnableSomeGzCompressPath(); EnableShimOwnedGzReads(); @@ -9289,7 +9298,8 @@ TEST_F(GzipFileTest, ConfigChangeMidReadDoesNotHandARewoundFileToZlib) { got.append(buf.data(), 4096); // Every accelerator off, part way through. Without the fix the next read - // takes the orig_gzread branch and serves zlib's stale header bytes again. + // takes the orig_gzread branch, where zlib meets the middle of a deflate + // stream and reads it as a header. SetUncompressPath(ZLIB, false, false); int ret = 0; @@ -9344,6 +9354,162 @@ TEST_F(GzipFileTest, GzopenLevelZeroReadFileIsNeverRewound) { remove(filename); } +// The three tests below all go through gzdopen on a descriptor the test opened +// itself, for one reason: it is the only way to watch the file offset from +// outside. What zlib does or does not read is invisible through the gzFile API +// and is exactly what is being asserted, so lseek(fd, 0, SEEK_CUR) is the +// measurement. + +// Nothing has been read at open, on a gzip file the shim is going to own. Two +// bytes went off the descriptor and went back, so the offset is where zlib's +// own gzopen would have left it, and an application gzdirect is answered out of +// the shim's own test with no I/O at all. +// +// The regression this is here for is delegating gzdirect for this file. zlib +// has not looked, so the delegated call would make it look -- allocating, and +// pulling its whole input buffer out of the descriptor from under the shim's +// read loop. +TEST_F(GzipFileTest, GzdirectOnAShimOwnedGzipFileMovesNothing) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(6000); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + int fd = open(filename, O_RDONLY); + ASSERT_GE(fd, 0); + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + + EXPECT_EQ(lseek(fd, 0, SEEK_CUR), static_cast(0)); + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(lseek(fd, 0, SEEK_CUR), static_cast(0)); + // Asked twice, because the shim's answer is cached and zlib's is not: a + // second call must not be the one that looks either. + EXPECT_EQ(gzdirect(fp), 0); + EXPECT_EQ(lseek(fd, 0, SEEK_CUR), static_cast(0)); + + // And the file still reads, which is the assertion that catches the peek + // being left in gz->peek after the seek back: the read path seeds io_buf from + // it, so inflate would be handed the two header bytes twice. + std::string got; + std::vector buf(4096, 0); + int ret = 0; + while ((ret = gzread(fp, buf.data(), static_cast(buf.size()))) > + 0) { + got.append(buf.data(), static_cast(ret)); + } + EXPECT_EQ(ret, 0); + EXPECT_EQ(got, payload); + + // Again from the top. gzrewind puts the descriptor back at gz->start, which + // is only the right place if the peek was accounted for, and re-reads through + // the same seeding. + ASSERT_EQ(gzrewind(fp), 0); + got.clear(); + while ((ret = gzread(fp, buf.data(), static_cast(buf.size()))) > + 0) { + got.append(buf.data(), static_cast(ret)); + } + EXPECT_EQ(ret, 0); + EXPECT_EQ(got, payload); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// A file that is not a gzip member goes to zlib at open, and this is what that +// buys. zlib is in the state it would be in with no shim loaded -- nothing +// allocated, nothing read -- so gzbuffer is not just accepted but applied, and +// the size the caller asked for is the size zlib reads with. +// +// Measured through the offset because that is the only visible difference. The +// version of this that borrowed zlib's header look had already made zlib +// allocate at 512 bytes by the time the application could speak, so a gzbuffer +// of any size changed nothing and the file was read 512 bytes at a time. +TEST_F(GzipFileTest, GzbufferOnAPlainFileIsHonouredByZlib) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + // Comfortably more than zlib's 8 KiB default, so "read it all" and "read a + // bufferful" are different offsets. + const std::string plain = PositionStampedPayload(40000); + const char* filename = "file.gz"; + remove(filename); + FILE* raw = fopen(filename, "wb"); + ASSERT_NE(raw, nullptr); + ASSERT_EQ(fwrite(plain.data(), 1, plain.size(), raw), plain.size()); + ASSERT_EQ(fclose(raw), 0); + + int fd = open(filename, O_RDONLY); + ASSERT_GE(fd, 0); + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + + // The two peek bytes went back, so zlib starts where it always would. + EXPECT_EQ(lseek(fd, 0, SEEK_CUR), static_cast(0)); + EXPECT_EQ(gzbuffer(fp, 1u << 20), 0); + + char first = '\0'; + ASSERT_EQ(gzread(fp, &first, 1), 1); + EXPECT_EQ(first, plain[0]); + // One byte asked for, the whole file read: zlib filled the buffer it was told + // to use. At the old 512 this offset was 1,024. + EXPECT_EQ(lseek(fd, 0, SEEK_CUR), static_cast(plain.size())); + // And zlib's own refusal after allocating, which is the half of the fidelity + // the shim used to have to imitate. + EXPECT_EQ(gzbuffer(fp, 1u << 20), -1); + + std::string got(1, first); + std::vector buf(4096, 0); + int ret = 0; + while ((ret = gzread(fp, buf.data(), static_cast(buf.size()))) > + 0) { + got.append(buf.data(), static_cast(ret)); + } + EXPECT_EQ(ret, 0); + EXPECT_EQ(got, plain); + EXPECT_NE(gzeof(fp), 0); + int errnum = Z_OK; + gzerror(fp, &errnum); + EXPECT_EQ(errnum, Z_OK); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + +// The other side of the same line: a gzip file the shim reads gets the shim's +// own answer, which is 0 before any I/O and -1 after, and no size is applied +// because the shim's buffers are fixed. What must not happen is zlib being left +// holding a buffer for a file it is not reading. +TEST_F(GzipFileTest, GzbufferOnAShimOwnedGzipFileLeavesZlibUnallocated) { + EnableSomeGzCompressPath(); + EnableShimOwnedGzReads(); + + const std::string payload = PositionStampedPayload(6000); + ASSERT_EQ(ZlibCompressGzipFile(payload.data(), payload.size()), Z_OK); + + const char* filename = "file.gz"; + int fd = open(filename, O_RDONLY); + ASSERT_GE(fd, 0); + gzFile fp = gzdopen(fd, "rb"); + ASSERT_NE(fp, nullptr); + + EXPECT_EQ(gzbuffer(fp, 1u << 20), 0); + // Still nothing read on zlib's behalf, which is what "unallocated" looks like + // from out here. A delegated gzbuffer would have been refused instead. + EXPECT_EQ(lseek(fd, 0, SEEK_CUR), static_cast(0)); + + char buf[16] = {0}; + ASSERT_EQ(gzread(fp, buf, sizeof(buf)), static_cast(sizeof(buf))); + EXPECT_EQ(std::string(buf, sizeof(buf)), payload.substr(0, sizeof(buf))); + EXPECT_EQ(gzbuffer(fp, 1u << 20), -1); + + EXPECT_EQ(gzclose(fp), Z_OK); + remove(filename); +} + // A multi-member file, which is what the trailing-trailer rule has to keep // working: two members concatenated read as one stream, and the magic test that // stops at a non-member trailer must not stop at a real second member -- even diff --git a/zlib_accel.cpp b/zlib_accel.cpp index 691af9b..c05797f 100644 --- a/zlib_accel.cpp +++ b/zlib_accel.cpp @@ -141,8 +141,9 @@ static int (*orig_gzungetc)(int c, gzFile file); static char* (*orig_gzgets)(gzFile file, char* buf, int len); static z_size_t (*orig_gzfread)(voidp buf, z_size_t size, z_size_t nitems, gzFile file); -// Only ever called once per read-mode open, to let zlib decide whether the file -// is a gzip member at all. See GzLookAtOpen. +// Only ever called on a file zlib owns, where zlib is doing the reading and its +// answer is the true one. Never on a file the shim reads: the call is what +// makes zlib look, and the look allocates and reads ahead. See gzdirect below. static int (*orig_gzdirect)(gzFile file); static const char* (*orig_gzerror)(gzFile file, int* errnum); static void (*orig_gzclearerr)(gzFile file); @@ -2188,34 +2189,32 @@ struct GzipFile { // which is what zlib synthesizes (gzlib.c). Not called `path` -- that name is // taken by the ExecutionPath above. std::string file_name; - // Set when GzLookAtOpen rewound the descriptor after letting zlib read the - // header. From that point zlib's own buffered input describes a position the - // descriptor is no longer at, so handing the file to orig_gzread would serve - // those bytes a second time and then fail. Reads stay with the shim for the - // life of the file; only the choice of decompressor may still change. + // Set for a gzip file the shim's own header test claimed, and for any + // descriptor whose header bytes it took and could not put back. Either way + // zlib's idea of where the file is no longer matches the descriptor, so + // handing the file to orig_gzread would serve bytes twice and then fail. + // Reads stay with the shim for the life of the file; only the choice of + // decompressor may still change. bool shim_owns_reads = false; - // The header bytes of a descriptor that could not be rewound, and how many of - // them are really there -- 0, 1 or 2. On a pipe the bytes cannot be put back, - // so the shim keeps them and hands them to the read loop instead. They cannot - // live in io_buf: that is allocated lazily, on the first read. + // The header bytes the shim read, and how many of them are really there -- 0, + // 1 or 2. Held only while they are still owed to somebody: a descriptor that + // can be seeked gets them put back and this returns to 0, and one that cannot + // keeps them here for the read loop. They cannot live in io_buf: that is + // allocated lazily, on the first read. unsigned char peek[2] = {0, 0}; uint8_t peek_len = 0; - // Set at open for a descriptor that cannot be rewound and that an accelerator - // is configured for: the shim, rather than zlib, is going to have to run the - // header test on this file. It says nothing about whether that has happened - // yet, and it stays true for the life of the file, which is what makes it the - // gate gzdirect can be gated on -- the seekable case is untouched, because - // there zlib has looked and answers for itself. + // Set at open for every read-mode file an accelerator is configured for: the + // shim, rather than zlib, is going to run the header test on it. Says nothing + // about whether that has happened yet -- for a descriptor that cannot be + // seeked the test is deliberately deferred to the first read. bool shim_must_peek = false; // Set once that test has actually run, which is the moment the descriptor - // moved. It is deliberately not done at open: zlib performs no I/O until the - // first read, and a read inside gzdopen deadlocks the single-threaded program - // that wraps a pipe's read end before writing to it. So this is also the - // shim's stand-in for zlib's how != LOOK. + // moved. So this is also the shim's stand-in for zlib's how != LOOK. bool shim_peeked = false; - // The peek said this is not a gzip member, so there is nothing to decompress - // and the shim copies bytes through -- zlib's COPY mode, for the one case - // where zlib cannot be left to do it. + // The peek said this is not a gzip member and the bytes could not be put + // back, so the shim copies them through itself -- zlib's COPY mode, for the + // one case where zlib cannot be left to do it. A seekable plain file goes to + // zlib instead and this stays false. bool transparent_read = false; // The mirror image: this file was handed to zlib at open and every call on it // has been forwarded since, so zlib's own position and error state are the @@ -2304,7 +2303,7 @@ static void GzMirrorZlibError(gzFile file, GzipFile* gz) { class GzipFiles { public: // Returns the entry it just created, so the caller can finish initializing it - // (the file name, and the open-time header look) without a second lookup. + // (the file name, and the open-time header test) without a second lookup. std::shared_ptr Set(gzFile file, int fd, const GzOpenParams& params) { auto f = std::make_shared(fd, params); @@ -2341,42 +2340,42 @@ static bool GzUncompressAcceleratorSelected() { configs[USE_IGZIP_UNCOMPRESS]; } -// The header test asked by the shim rather than by zlib, for a descriptor that -// cannot be rewound. Borrowing zlib's answer is not possible there: zlib's look -// reads up to 8 KB, and on a pipe those bytes cannot be put back, so they would -// sit in zlib's private buffer with the shim reading the file from behind them. +// The header test, asked by the shim rather than by zlib, for every read-mode +// file the shim might read. zlib's own answer is not borrowable: reaching it +// means calling zlib's gzdirect, which makes zlib allocate and read ahead, and +// both of those have to be undone afterwards -- the read-ahead cannot be, on a +// descriptor that cannot be seeked, and the allocation cannot be at all. See +// GzPeekAtOpen below for the whole of that argument. // -// So the shim takes two bytes of its own. On a pipe that costs nothing, because -// putting them back never arises -- the bytes are wanted by whoever reads next, -// and the shim is that reader either way. It is also far less blocking than -// zlib's own look. -// -// This is the second use of the magic-number test in gzread below, not a second -// implementation of it. It is also what forces gzdirect to be intercepted: zlib -// has not looked, so zlib cannot answer. +// So the shim takes two bytes of its own. This is the second use of the +// magic-number test in gzread below, not a second implementation of it. It is +// also what forces gzdirect to be intercepted: zlib has not looked, so zlib +// cannot answer. // // Split in two along the line of what needs the descriptor. Deciding *that* the -// shim will have to look costs nothing and is settled at open, below; the -// looking itself waits, because zlib performs no I/O at all until the first -// read and a read from inside gzdopen is a behaviour change with teeth. A -// single-threaded program that wraps a pipe's read end and only then writes to -// it would deadlock in gzdopen, and a non-blocking descriptor would latch -// EAGAIN before the application had asked for anything. +// shim will have to look costs nothing and is settled at open, below. The +// looking itself is at open too for a descriptor that can be seeked, because +// there the bytes can be put back; on one that cannot it waits until the first +// read, which is where zlib would have done its own reading and so is the +// moment a blocking read or an EAGAIN belongs to. static void GzDecideOwnershipAtOpen(GzipFile* gz) { if (!GzUncompressAcceleratorSelected() || gz->path == ZLIB) { gz->path = ZLIB; return; } gz->shim_must_peek = true; - // Settled here even though no byte has moved yet. The shim is the only reader - // this descriptor is going to have: it is about to take the header bytes off - // it, and they cannot be put back for zlib to find. + // Provisional, and set here rather than after the peek so that a descriptor + // whose peek is still to come is already accounted for. The shim is about to + // take the header bytes off this descriptor, and until they are back on it + // the shim is the only reader it can have. GzPeekAtOpen clears this again in + // the one case where they do go back and zlib turns out to want the file. gz->shim_owns_reads = true; } -// The look itself, run at the first moment the answer is actually needed -- the -// first read, or an application gzdirect. Idempotent, and a no-op for every -// file that is not the non-rewindable case. +// The look itself. Idempotent, and a no-op for every file the shim did not +// undertake to test. Called from GzPeekAtOpen for a descriptor that can be +// seeked, and otherwise at the first moment the answer is really needed -- the +// first read, or an application gzdirect. static void GzEnsurePeeked(GzipFile* gz) { if (!gz->shim_must_peek || gz->shim_peeked) { return; @@ -2408,85 +2407,107 @@ static void GzEnsurePeeked(GzipFile* gz) { !(gz->peek_len == 2 && gz->peek[0] == 0x1f && gz->peek[1] == 0x8b); } -// Ask zlib, once per read-mode open, whether this file is a gzip member at all. +// Decide, once per read-mode open, whether this file is a gzip member at all, +// and who is going to read it. +// +// This used to borrow zlib's answer for a descriptor it could rewind: call +// zlib's own gzdirect() and believe it. That worked, and it cost more than it +// looked like. zlib's gzdirect runs gz_look, and gz_look *allocates* -- so from +// that call onwards zlib's gzbuffer refuses every size an application asks for, +// including the first, because having allocated is the only thing gzbuffer +// tests (gzlib.c:299). Confirmed against bare zlib with no shim loaded: +// gzbuffer returns 0 after a gzopen and -1 after a gzdirect on the same file. +// Borrowing the answer meant the shim picking, at open, which size zlib was +// going to be stuck with for the life of the file, on behalf of a caller who +// had not spoken yet. // -// The shim deliberately has no magic-number test of its own for a file it can -// rewind: two implementations of "is this a gzip header" would drift, and zlib -// already has one in gz_look(). gzdirect() is the public way to reach it. That -// call is guarded inside zlib by how == LOOK && x.have == 0, so it reads at -// most once for the life of the file -// -- which is what makes this affordable, and is also why gzdirect needs no -// interception for a file that got here: after this call zlib answers from -// state it already has, so every later gzdirect the application makes is -// truthful and costs nothing. +// So the shim asks the question itself, with the two-byte test it already +// needed for pipes, and leaves zlib untouched: nothing allocated, how still +// LOOK, start still right. Two bytes against zlib's up-to-8 KB is not a +// shortcut -- a gzip header is identified by its first two bytes and gz_look +// tests exactly those. The other 8 KB was zlib filling its input buffer at the +// same time, which is read-ahead the shim throws away on every file it goes on +// to own. // -// A file that is not a gzip member is not the shim's business. Hand it to zlib -// and stay out of the way: zlib has already buffered the bytes and switched -// itself to copy-through, so it reads the file correctly with no help. The same -// applies to an empty file and to a file too short to hold a header. -static void GzLookAtOpen(gzFile file, GzipFile* gz) { +// A file that turns out not to be a gzip member is not the shim's business. Put +// the two bytes back and hand it to zlib, which is then in the state it would +// have been in without the shim at all: it buffers the file at whatever size +// its caller asks for and reads it correctly with no help. That covers an empty +// file and a file too short to hold a header too. The one descriptor whose +// bytes cannot go back is one that cannot be seeked, and there the shim keeps +// them and copies the file through itself. +static void GzPeekAtOpen(GzipFile* gz) { if (gz->mode != FileMode::READ) { return; } // Where the file is now is both zlib's state->start and the position to put // the descriptor back to afterwards. A descriptor that cannot be seeked - // cannot be put back, so zlib's look cannot be borrowed for it and the shim - // runs its own test instead. -1 stays in start, which is what gzseek and - // gzrewind refuse on. + // cannot be put back; -1 stays in start, which is what gzseek and gzrewind + // refuse on. gz->start = lseek(gz->fd, 0, SEEK_CUR); - if (gz->start == static_cast(-1)) { - GzDecideOwnershipAtOpen(gz); - return; - } - // Settled before the look, not after it: see GzUncompressAcceleratorSelected. - if (!GzUncompressAcceleratorSelected() || gz->path == ZLIB) { - gz->path = ZLIB; + GzDecideOwnershipAtOpen(gz); + if (!gz->shim_must_peek) { + // Already zlib's, and zlib is still untouched -- no accelerator configured + // for uncompressing, or a mode string the constructor found unoffloadable. + // There is nothing to test and nothing to put back. return; } - if (orig_gzdirect == nullptr) { + if (gz->start == static_cast(-1)) { + // A pipe, where the peek stays lazy, and deliberately so: zlib performs no + // I/O of its own until the first read, and a read from inside gzdopen is a + // behaviour change with teeth. A single-threaded program that wraps a + // pipe's read end and only then writes to it would deadlock, and a + // non-blocking descriptor would latch EAGAIN before the application had + // asked for anything. The bytes cannot be put back either way, so nothing + // is gained by taking them early. return; } - // Shrink the look before it happens. gz_look sizes both of its buffers from - // `want` and reads `want` bytes to judge the header by, and `want` is - // settable through public gzbuffer -- which zlib refuses once its buffers - // exist, so this must come before the gzdirect below and zlib.h says so. zlib - // does a genuine look either way and sets its own how/direct; no private - // state is touched. - // - // Measured on this host, per file open for reading at the same time: 31,776 - // bytes of zlib buffers and inflate state at the default, 8,736 at 512, and a - // 512-byte read of the descriptor instead of 8,192. Smaller sizes save little - // more -- 7,232 bytes at zlib's floor of 8 -- and cost a great deal on the - // one path that still goes through zlib's buffer: a transparent file read a - // byte at a time is 21x slower at 8 and 2x slower at 512. - // - // It matters that this is below the test above. zlib inflating a whole file - // through an 8-byte input buffer is 85x slower, and that is exactly the file - // the test above has already sent to zlib. - if (orig_gzbuffer != nullptr) { - orig_gzbuffer(file, 512); - } + GzEnsurePeeked(gz); - if (orig_gzdirect(file) != 0) { - // Not a gzip member. zlib owns it from here. - gz->path = ZLIB; + if (lseek(gz->fd, gz->start, SEEK_SET) == static_cast(-1)) { + // Seekable one syscall ago and not now. The shim is holding the bytes it + // read, so it can still read this file correctly from here -- which is + // exactly the pipe's situation, so say so the way a pipe says it and let + // gzseek and gzrewind refuse on the same marker. Handing the file to zlib + // instead would start it two bytes in. + gz->start = static_cast(-1); return; } - // The shim reads it, so the header bytes zlib consumed have to come back. - // zlib's copy of them is now stale, and serving them again on top of a - // rewound descriptor would duplicate that much of the file and then fail the - // checksum - // -- so from here the descriptor belongs to the shim alone. - if (lseek(gz->fd, gz->start, SEEK_SET) == static_cast(-1)) { - gz->path = ZLIB; + if (!gz->transparent_read) { + // A gzip member, and the descriptor is back at its first byte, so the peek + // must not also be served out of gz->peek: the read path seeds io_buf from + // it, and inflate would be handed the header twice. Clearing it also leaves + // the file byte-for-byte in the state it was never peeked in, which is what + // gzrewind and the held-bytes arithmetic in gzseek assume. + gz->peek_len = 0; return; } - gz->shim_owns_reads = true; + + // Not a gzip member, and the bytes went back, so this is a better file for + // zlib than for the shim: zlib buffers it at its caller's size, where the + // shim would allocate its fixed 768 KiB to copy bytes through. + gz->peek_len = 0; + gz->path = ZLIB; + gz->shim_owns_reads = false; + gz->shim_must_peek = false; + // Cleared, not left as GzEnsurePeeked set it: the read dispatch tests + // transparent_read ahead of path, so a stale true would run the shim's + // copy-through on a file zlib now owns. + gz->transparent_read = false; + // A failed peek arrives here too -- it reads nothing, so it cannot have found + // a header. Its Z_ERRNO goes back with the bytes. bare zlib reads nothing at + // open and so reports that errno at the first read, from the same syscall, + // and zlib is about to make that syscall. Latching it here instead would fail + // a file zlib may well read successfully, and would report the error before + // the application had asked for a byte. This is also why the clearing lives + // here rather than inside GzEnsurePeeked: on a pipe the peek runs at that + // first read already, so there the latch is at the right moment and is kept. + GzSetError(gz, Z_OK, nullptr); } // Inspired by gz_open in gzlib.c @@ -2595,8 +2616,8 @@ gzFile ZEXPORT gzopen(const char* path, const char* mode) { } catch (...) { // Only the text of a later gzerror message is lost. } - GzLookAtOpen(file, gz.get()); - // Whatever pinned it -- the header look above, or a mode string the + GzPeekAtOpen(gz.get()); + // Whatever pinned it -- the header test above, or a mode string the // constructor found unoffloadable -- a file already on the zlib path at // open never has a byte of it pass through the shim, so zlib's own position // and error state stay authoritative for it. @@ -2649,8 +2670,8 @@ gzFile ZEXPORT gzdopen(int fd, const char* mode) { } catch (...) { // Only the text of a later gzerror message is lost. } - GzLookAtOpen(file, gz.get()); - // Whatever pinned it -- the header look above, or a mode string the + GzPeekAtOpen(gz.get()); + // Whatever pinned it -- the header test above, or a mode string the // constructor found unoffloadable -- a file already on the zlib path at // open never has a byte of it pass through the shim, so zlib's own position // and error state stay authoritative for it. @@ -3375,10 +3396,11 @@ int ZEXPORTVA gzprintf(gzFile file, const char* format, ...) { } // zlib's COPY mode: the file is not a gzip member, so there is nothing to -// decompress and the bytes are simply handed through. zlib does this itself for -// every file it looked at, which is why the shim has no need of it -- except -// for a descriptor that could not be rewound, where the shim did the looking -// and now holds bytes zlib will never see. +// decompress and the bytes are simply handed through. Reached only by a +// descriptor that could not be seeked. Every other plain file is handed to zlib +// at open, which does this itself and better -- at the buffer size its caller +// asked for. Here the header bytes cannot be put back, so zlib would start the +// file two bytes in and the shim has to be the reader. // // No push-back handling: the callers of GzreadOwnedFile drain it before they // get here, which is the same arrangement the accelerator path relies on. @@ -4110,24 +4132,29 @@ int ZEXPORT gzeof(gzFile file) { return gz->read_past_end; } -// Intercepted for one case only: a descriptor the shim had to run the header -// test on itself, because it could not be rewound. There zlib has not looked -// and cannot answer -- worse, an application gzdirect would make zlib look -// right then, pulling up to 8 KB out of the pipe into zlib's private buffer and -// punching a hole in the front of the shim's input. +// Intercepted for the files the shim reads, and delegated for the rest, which +// is the same ownership line every function in this section is drawn on. +// +// zlib cannot be asked about a file the shim reads, and not merely because it +// does not know the answer. Asking is what makes zlib look, and the look +// allocates a buffer and pulls up to 8 KB off the descriptor -- out of the +// front of the shim's own input, and out of a pipe irretrievably. The shim has +// the answer already, from its own two-byte test, so it costs nothing to give. // -// Everything else delegates, which is the whole point of the shim_must_peek -// gate. A seekable file already has zlib's own cached answer, and it is the -// true one in all four cases (see GzLookAtOpen); a write-mode file keeps -// whatever zlib reports for "wT". Neither is touched here. +// A zlib-owned file delegates because zlib's answer is the true one there: it +// is reading the file, so its look is work that had to happen anyway, and its +// buffer is the one the application's gzbuffer sized. A write-mode file +// delegates because zlib's gzdirect guards its look with mode == GZ_READ, so +// for a writer it is a pure read of "was there a T in the mode string" -- state +// zlib holds and the shim has no reason to duplicate. // -// The gate is shim_must_peek and not shim_peeked because the peek is lazy: on a -// file the shim owns the test may not have run yet, and this call is one of the -// two things that makes it run. zlib's gzdirect looks for the same reason, so -// the I/O this does is I/O zlib would also have done. +// The peek may still be to come when this is called: on a descriptor that could +// not be seeked it is deliberately deferred to the first read, and this call is +// the other thing that makes it happen. zlib's gzdirect looks for the same +// reason, so the I/O is I/O zlib would also have done, at the same moment. int ZEXPORT gzdirect(gzFile file) { auto gz = gzip_files.Get(file); - if (gz == nullptr || !gz->shim_must_peek) { + if (gz == nullptr || gz->zlib_owns_file || gz->mode != FileMode::READ) { return orig_gzdirect != nullptr ? orig_gzdirect(file) : 0; } GzEnsurePeeked(gz.get()); @@ -4176,7 +4203,7 @@ static int GzRewindOwned(GzipFile* gz) { if (gz->mode != FileMode::READ || !GzReadableAfterError(gz)) { return -1; } - // A pipe cannot be rewound, and GzLookAtOpen leaves start at -1 to say so. + // A pipe cannot be rewound, and GzPeekAtOpen leaves start at -1 to say so. if (gz->start == static_cast(-1)) { return -1; } @@ -4256,15 +4283,19 @@ static z_off64_t GzSeekOwned(GzipFile* gz, z_off64_t offset, int whence) { // after the first read how is COPY, so the lseek is attempted, and a pipe // refuses it // - // transparent_read carries that distinction on its own, because the header - // test is lazy: it is only ever set by GzEnsurePeeked, so it cannot be true - // before the test has run, and the test runs at exactly the moments zlib's - // look does -- the first read, or an application gzdirect. So a false here - // means "zlib is still in LOOK" and the lazy path below is the matching one. - // It is deliberately not io_started, which is now a different event: gzdirect - // moves zlib to COPY without any application-visible I/O, and gating on - // io_started would take the lazy path there and disagree. Measured rather - // than assumed -- the conformance suite's O11/O12 pipe seeks turned this up. + // Only one kind of file reaches this branch: a pipe. A seekable plain file is + // handed to zlib at open, so gzseek delegates before getting here, and a gzip + // file is compressed and has to be inflated through. On a pipe the header + // test is still lazy, and transparent_read carries the LOOK/COPY distinction + // on its own because of it: the flag is only ever set by GzEnsurePeeked, so + // it cannot be true before the test has run, and the test runs at exactly the + // moments zlib's look does -- the first read, or an application gzdirect. So + // a false here means "zlib would still be in LOOK" and the lazy path below is + // the matching one. It is deliberately not io_started, which is a different + // event: gzdirect moves zlib to COPY without any application-visible I/O, and + // gating on io_started would take the lazy path there and disagree. Measured + // rather than assumed -- the conformance suite's O11/O12 pipe seeks turned + // this up. if (gz->transparent_read && gz->pos + offset >= 0) { const off_t held = static_cast(gz->peek_len) + static_cast(gz->pushback.size()); @@ -4423,35 +4454,35 @@ void ZEXPORT gzclearerr(gzFile file) { // zlib's gzbuffer accepts a size only before any reading or writing has begun, // because that is when it would still be allocating (gzbuffer, gzlib.c:299: -// "make sure we haven't already allocated memory"). Without interception the -// shim gets that backwards twice over: +// "make sure we haven't already allocated memory"). // -// - before the open-time header look existed, zlib never allocated at all on a -// file the shim read, so its size stayed 0 and gzbuffer accepted every call, -// including the ones zlib itself would have refused; -// - the header look does make zlib allocate, at open, so zlib's gzbuffer went -// the other way and started refusing the *first* call as well -- confirmed -// in bare zlib with no shim loaded: gzbuffer returns 0 after gzopen and -1 -// after a gzdirect on the same file. +// A file zlib owns delegates, and gets full fidelity: the shim leaves zlib +// unallocated at open, so zlib's own accept-then-refuse is intact and the size +// is genuinely applied. That is what GzPeekAtOpen's two-byte test buys -- the +// header test the shim used to borrow from zlib made zlib allocate at open, so +// zlib's gzbuffer then refused the *first* call as well, and delegating would +// have answered -1 where bare zlib answers 0. // -// So the refusals have to be replicated against the shim's own state, which is -// what io_started tracks. +// A file the shim reads or writes cannot delegate, because zlib is not the one +// buffering it: zlib would stay unallocated forever and accept every call, +// including the ones it would itself have refused. So the refusals are +// replicated against the shim's own state, which is what io_started tracks. // -// Judgment call, worth a reviewer's attention: the size itself is accepted and -// then not applied. The shim's buffers are a fixed 256 KiB of uncompressed and -// 512 KiB of compressed data, and the accelerator paths are sized around that -// split, so plumbing an arbitrary size through it is a change to the read and -// write paths rather than to this function. Ignoring it is a performance -// difference and not a correctness one for anything smaller than the shim's own -// buffers -- which is every default and most requests. The alternative, pinning -// any file whose caller calls gzbuffer to plain zlib, would take acceleration -// away from exactly the callers trying to tune for speed. +// For those files the size is accepted and then not applied. The shim's buffers +// are a fixed 256 KiB of uncompressed and 512 KiB of compressed data, and the +// accelerator paths are sized around that split, so plumbing an arbitrary size +// through is a change to the read and write paths rather than to this function. +// Ignoring it is a performance difference and not a correctness one for +// anything smaller than the shim's own buffers -- which is every default and +// most requests. The alternative, pinning any file whose caller calls gzbuffer +// to plain zlib, would take acceleration away from exactly the callers trying +// to tune for speed. int ZEXPORT gzbuffer(gzFile file, unsigned size) { Log(LogLevel::LOG_INFO, "gzbuffer Line ", __LINE__, ", file ", static_cast(file), ", size ", size, "\n"); auto gz = gzip_files.Get(file); - if (gz == nullptr) { + if (gz == nullptr || gz->zlib_owns_file) { return orig_gzbuffer != nullptr ? orig_gzbuffer(file, size) : -1; }