Gz position and errors by asonje · Pull Request #74 · intel/zlib-accel

asonje · 2026-08-31T23:00:10Z

What this does

Completes the gz API surface. main intercepts 20 of the 32 gz entry points; the other 12 are left to zlib, which is reading a stream the shim has taken over. That is where every remaining behavior difference lives.

Scope

In scope — four commits:

51ecab0 — intercepts the position and error family (gztell, gztell64, gzoffset, gzoffset64, gzseek, gzseek64, gzrewind, gzerror, gzclearerr, gzbuffer), and asks zlib once at open whether a file is actually a gzip member, so plain files, empty files and files with trailing bytes stop being unreadable.
7cde606 — intercepts gzopen64 as a forwarder to gzopen, which is what zlib itself does. Its absence took every application built with -D_FILE_OFFSET_BITS=64 off the shim entirely.
a266406 — for a descriptor that cannot be rewound, the shim takes two header bytes of its own instead of borrowing zlib's look. This closes all seven pipe cases, including a non-gzip pipe being unreadable and gzdirect consuming up to 8 KB of a pipe just by being asked.
25206bc — calls gzbuffer(512) before the open-time look, cutting the look's read from 8192 bytes to 512 and its memory from 31,866 to 8,657 bytes per concurrently-open read file.

Behaviour changes worth stating

A read interrupted by a signal is now sticky. zlib does not retry EINTR; it latches the error and refuses later calls. Matching that removes an accidental leniency where a caller could previously retry and succeed.
Large-file builds are accelerated where before they were not, with everything that implies: different compressed bytes, a different gzoffset, and the accelerator's memory footprint.
A non-gzip pipe is copied through by the shim, so gzeof, gzerror and gztell answer from the shim for those files — zlib knows nothing about that descriptor.
gzoffset legitimately differs from zlib's and always will: the shim reads 512 KiB of compressed input at a time where zlib reads 8 KiB, so it really is further into the file. gztell, the number applications care about, is now correct where it previously returned 0.
gzbuffer sizes are accepted and then not applied; the shim's buffers stay a fixed 256 KiB / 512 KiB. For any smaller size this is a speed difference, not a correctness one. The alternative — dropping a file to plain zlib as soon as its caller calls gzbuffer — would take acceleration away from exactly the callers tuning for speed.

…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 <olasoji.denloye@intel.com>

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 <olasoji.denloye@intel.com>

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 <olasoji.denloye@intel.com>

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 <olasoji.denloye@intel.com>

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 <olasoji.denloye@intel.com>

Copilot

Pull request overview

Completes accelerated gzip API handling for positioning, errors, large-file entry points, and transparent streams.

Changes:

Adds seek, position, error, and buffer interceptors.
Handles gzip detection for regular files and pipes.
Adds extensive gzip conformance tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
`zlib_accel.cpp`	Implements the expanded gzip API behavior.
`tests/zlib_accel_test.cpp`	Tests positioning, errors, pipes, and header detection.

Suppressed comments (10)

zlib_accel.cpp:2370

gzdirect() returns 0 even when its internal header read fails; the failure is exposed through gzerror. This branch therefore treats an open-time EINTR/I/O error as gzip, rewinds, and gives the file to the shim with gz->err == Z_OK, so the next read retries and gzerror loses zlib's sticky error. Inspect zlib's error state immediately after this call and either preserve zlib ownership or mirror the error before rewinding.

  if (orig_gzdirect(file) != 0) {

zlib_accel.cpp:3423

A descriptor error can occur after this call has already copied buffered output into buf. Overwriting read_bytes with -1 then hides those delivered bytes, while the sticky latch rejects the next read. zlib instead returns the partial count and reports the error on the following call; preserve a positive read_bytes here and use -1 only when it is zero.

            GzSetError(gz, Z_ERRNO, strerror(errno));
            read_bytes = -1;
            goto gzread_end;

zlib_accel.cpp:3513

An inflate failure can happen after earlier iterations copied bytes into the caller's buffer, and inflate() may also have produced output before detecting a bad trailer. This error path later forces the return to -1, hiding all such output; because the error is now sticky, it can never be reported as a partial read. Preserve produced output and return its count once, deferring -1 to the next call as zlib does.

              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

zlib_accel.cpp:3800

The result of filling a pending seek gap is ignored. If writing the zeros fails (for example, disk full), the skip has already been cleared, but close can continue and return Z_OK, silently producing a shorter stream. Preserve this failure and return it after still closing the underlying handle, matching zlib's close behavior.

  if (gz->pending_skip > 0 && GzIsWriteMode(gz->mode)) {
    GzWriteZeros(file, gz.get());

zlib_accel.cpp:4224

io_started is not a reliable proxy for zlib buffer allocation: registered files whose first I/O delegates through gzgetc, gzgets, gzfread, gzputc, gzputs, gzfwrite, or gzprintf never set it, so this check accepts gzbuffer after I/O when zlib rejects it. Update every I/O wrapper that can bypass gzread/gzwrite (and allocation-triggering operations such as flush) before relying on this flag.

  if (gz->io_started) {
    return -1;

zlib_accel.cpp:3294

A zero-length read is an early no-op in zlib and leaves gzbuffer available. This unconditional assignment makes gzread(file, buf, 0) incorrectly cause every subsequent gzbuffer call to fail. Only set the flag for a nonzero request.

  // 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;

zlib_accel.cpp:4106

The plain gztell API must return -1 when the 64-bit position is not representable in z_off_t; this unchecked cast wraps/truncates it on narrow-z_off_t builds. Apply the same round-trip range check used by zlib.

  return static_cast<z_off_t>(gz->pos + gz->pending_skip);

zlib_accel.cpp:4141

An effective compressed offset larger than z_off_t is silently truncated here, whereas zlib's plain gzoffset returns -1 when the 64-bit result cannot be represented. Add a round-trip representability check before returning.

  return static_cast<z_off_t>(GzOffsetOwned(gz.get()));

zlib_accel.cpp:4147

When a shim-owned write stream later switches to path == ZLIB (for example after gzsetparams(..., 0, ...)), subsequent delegated writes latch failures only in zlib's state, while zlib_owns_file remains false. This function then reports the shim's stale Z_OK instead of that write error. Synchronize zlib errors into gz->err on every delegated operation, or merge the two states here.

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;

zlib_accel.cpp:3071

If zero filling delegates to zlib after this stream has switched to path == ZLIB, a failed gzwrite updates only zlib's error state; gz->err can still be Z_OK. Returning it here therefore reports a failed flush as success. Return the delegated error (or synchronize it into the shim latch) rather than assuming gz->err was set.

      GzWriteZeros(file, gz.get()) != 0) {
    return gz->err;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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 <olasoji.denloye@intel.com>

mulugetam · 2026-09-04T16:41:36Z

+// (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


This breaks the build for anyone compiling the shim with -D_FILE_OFFSET_BITS=64.

g++ -std=c++17 -fsyntax-only -D_FILE_OFFSET_BITS=64 -I. -Iconfig zlib_accel.cpp

yes it does and it should. The shim previously did not intercept the 64 bit variants of gzopen, gzseek e.t.c. it was correct but left a gap because an application that specifically asked for the 64 bit variants would not be accelerated. This is why we intercept both variants and why compiling the shim with -D_FILE_OFFSET_BITS=64 is incorrect because it would rename the functions into the 64 bit variants and cause a duplication and compile error.

i will modify the build so that it errors out when this flag is set with the reason

mulugetam · 2026-09-04T17:03:49Z

+
+  // 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;


Bug: writes into a file that is opened in read-only mode.

gzFile r = gzopen("demo.gz", "rb"); /* file holds "hello world" */ gzwrite(r, "XXXX", 4); /* zlib: 0. shim: 4 */ gzread(r, out, 31); /* zlib: 11 "hello world" */ /* shim: 15 "XXXXhello world" */

Return early if in read-only mode:

if (!GzIsWriteMode(gz->mode)) { return 0; }

mulugetam · 2026-09-04T17:37:11Z

+      static_cast<void*>(file), ", size ", size, "\n");
+
+  auto gz = gzip_files.Get(file);
+  if (gz == nullptr) {


Could you double check if a zlib-owned file should take the path as well? i.e. if (gz == nullptr || gz->zlib_owns_file).

Yes it should — but not until one other thing is fixed, and this comment is what exposed it.

For a seekable read-mode file with an accelerator configured, we eagerly call orig_gzdirect() at open to settle ownership: a gzip member means the shim owns the reads, anything else means zlib does. The side effect is that zlib's gz_look() runs, which allocates and prefetches up to 8 KB of the file into zlib's private buffer; all wasted when the shim ends up owning it. We call gzbuffer(file, 512) first to limit the damage (measured: 31,776 →
8,736 bytes per open read file).

The unintended consequence is the look itself, not the shrink. gz_look() allocating is exactly the condition zlib's gzbuffer() refuses on, so from open onwards zlib would refuse an application gzbuffer(); including the first call,
which bare zlib accepts. That is why delegating unconditionally today answers −1 where bare zlib answers 0 on a plain file, and why this function currently keeps its own io_started bookkeeping and drops the requested size instead.
The cost of dropping it is real: after gzbuffer(1 MB) on a plain file, bare zlib reads in 1048576/1040774/832-byte chunks and we read 8192/8191/832.

So the fix is to stop borrowing zlib's look: the shim does its own two-byte header test and seeks back, leaving zlib pristine. Then gzdirect answers from shim state for every file the shim owns, and this line becomes exactly what
you wrote.

… 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 <olasoji.denloye@intel.com>

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 <olasoji.denloye@intel.com>

…ib'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 <olasoji.denloye@intel.com>

asonje added 5 commits August 31, 2026 10:51

asonje requested a balanced review from Copilot September 1, 2026 17:34

Copilot started reviewing on behalf of asonje September 1, 2026 17:34 View session

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread zlib_accel.cpp

Comment thread zlib_accel.cpp

Comment thread zlib_accel.cpp Outdated

Comment thread tests/zlib_accel_test.cpp Outdated

Comment thread zlib_accel.cpp

Comment thread zlib_accel.cpp Outdated

asonje requested review from matt-welch and mulugetam September 1, 2026 22:30

mulugetam reviewed Sep 4, 2026

View reviewed changes

asonje added 3 commits September 4, 2026 13:32

Navigation Menu

Uh oh!

Gz position and errors - #74

Gz position and errors#74
asonje wants to merge 9 commits into
mainfrom
gz-position-and-errors

asonje commented Aug 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mulugetam Sep 4, 2026

Uh oh!

asonje Sep 4, 2026

Uh oh!

mulugetam Sep 4, 2026

Uh oh!

mulugetam Sep 4, 2026

Uh oh!

asonje Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

asonje commented Aug 31, 2026

What this does

Scope

Behaviour changes worth stating

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mulugetam Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

asonje Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

mulugetam Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

mulugetam Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

asonje Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants