Pr gz api gaps - #72
Open
asonje wants to merge 11 commits into
Open
Conversation
gzclose was the only close entry point the shim intercepted, so an application that closed an accelerated file through gzclose_w or gzclose_r reached zlib directly: the data gzwrite had buffered was never flushed, the registry entry outlived the gzFile, and the bytes zlib's own close appends were left in the file instead of being truncated away again. A close that reported success could drop the tail of the file. Extract gzclose's body into GzCloseCommon() and give it the mode the entry point requires. zlib's gzclose_r/gzclose_w reject a file opened for the other direction without touching it, so the mode check happens before the flush, the close and the truncate -- a mismatched call must not act on a file zlib would have left alone. Append counts as a write, matching zlib, which folds append into its own write mode right after opening the file. Also export GetGzipFileExecutionPath() for tests: the gz entry points keep their own per-file state, which is not reachable through the deflate and inflate accessors, so without it a gz test cannot tell a case that exercised an accelerator from one that silently fell back to zlib. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
The level in a gzopen/gzdopen mode string was parsed and thrown away, and gzsetparams was not intercepted at all, so an accelerated gz file was compressed at the default level whatever the application asked for. Level 0 is the request with a contract to break: it asks for stored, uncompressed blocks, which no backend can emit. GetOpenFlags now parses the level digit, 'T' and '+' into a GzOpenParams struct. GzipFile records the level, Reset() keeps it -- as zlib's own resets do -- and passes it to the fallback deflate stream, and the IGZIP one-shot in gzwrite compresses at it. A file opened at level 0, or for a transparent write, is pinned to zlib at open time, since neither request is offloadable at all. gzsetparams is intercepted: it forwards to zlib first, so zlib decides the return value and records the level for a file that may later be handed back to it, then writes out the data buffered under the old level as a member of its own before recording the new one, and pins the file to zlib at level 0. The zlib fall-through in gzwrite now takes the pin as well, not only use_zlib_compress. A pinned file reaches zlib because the request was never offloadable, so a write that depends on the config would report failure with use_zlib_compress off; deflate() carries the same term at its own fall-through for the same reason. Two open-path fixes fall out of the same parse. The shim opens the file itself, before zlib validates the mode string, so a mode zlib refuses -- '+', or none of r/w/a -- has to be rejected before open(2): O_TRUNC would empty a file plain zlib leaves untouched, and O_CREAT would create one it never creates. And gzdopen no longer registers a NULL gzFile, which would key an entry that every gz entry point then finds in place of its unregistered-file handling. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib's gz* file API has several functions that just write data: gzputc writes a byte, gzputs a string, gzfwrite an array, gzprintf a formatted string. gzflush is the related promise that everything written so far has reached the file. This shim intercepted none of them, so each call went straight to real zlib. That corrupts the file. zlib compresses through the deflate stream it keeps for the file, and on a file this shim owns that stream has never seen any data -- the shim buffers what the application writes and compresses it itself, emitting one complete gzip member per buffer. So zlib writes its own gzip header and its own member into the middle of the shim's output. Bytes written with gzputs came back after bytes written later with gzwrite, and gzflush left behind a header with no data under it. All five are intercepted now, and none of them changes which engine a file uses. The four writers pass their bytes to the shim's own gzwrite, so they are buffered and offloaded exactly like a gzwrite of the same bytes. gzprintf formats the string here rather than handing the file back to zlib to format it, which would leave the rest of the file unaccelerated. gzflush writes the shim's buffer out as a complete gzip member, which is what makes those bytes readable, so no value of its flush argument needs anything further. A gzFile this shim has no entry for -- one the application never opened through gzopen/gzdopen, including the nullptr a failed open returns -- still goes straight to zlib, as does a file already on the zlib path. Return values follow zlib's: whole items for gzfwrite, a refused request when its size * nitems overflows, -1 or Z_STREAM_ERROR for a file that cannot be written. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib's gz* file API also has functions that read: gzgetc takes one byte, gzgets a line, gzfread an array, and gzungetc hands a byte back to be read again. As on the writing side, the shim intercepted none of them, so each call went to real zlib and read through the stream zlib keeps for the file. That returns the wrong bytes. When this shim reads a file it reads the compressed data itself, in large blocks, and serves decompressed bytes out of its own buffer, so the file offset sits far ahead of where zlib believes it is and zlib's own buffer is empty. gzgetc and the rest therefore started decoding from the middle of a compressed block. All of them read through the shim's own gzread now, so they see the same position and the same data a gzread would. gzgets stops at a newline or one byte short of the buffer, whichever comes first, and reports end of file as no string rather than an empty one. gzfread returns whole items and refuses a request whose size * nitems overflows. gzungetc stores the byte in the shim's per-file state and gzread hands it back before anything else -- one byte, which is what zlib guarantees, and a second push before that byte is read is refused, which zlib allows. gzeof counts a waiting byte as data, so a file that has one is not at its end. Two details come from zlib's own header. gzgetc is a macro that serves a byte straight out of zlib's buffer and only calls the function when that buffer is empty, which on a file the shim reads it always is, so the definition here sheds the macro first, as zlib's gzread.c does. gzgetc_ is the plain function zlib exports alongside that macro, and reaches the same code. A gzFile this shim has no entry for, or one already on the zlib path, still goes straight to zlib. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
… it does not The intercepted list still named only the six gz* functions the shim started with, so a reader could not tell which of zlib's gz* calls are safe to use on an accelerated file and which are not. List the twenty entry points that are intercepted now, and explain the design they follow. zlib keeps one continuous compressed stream per gzip file; zlib-accel instead compresses each buffer it fills into a complete gzip member, so a large file becomes a sequence of members. That is valid gzip, and it is why every call that moves bytes has to be served by zlib-accel rather than handed to zlib: zlib's stream for the file has seen none of the file's data, so a write through it inserts an unrelated member in the middle of the ones zlib-accel wrote, and a read through it decodes from the middle of a member. The same paragraph covers gzflush, gzungetc's one byte of push-back, the two directional close functions, and where the compression level comes from. Add a table of the gz* functions that are deliberately not intercepted - the position family, gzerror/gzclearerr, gzbuffer, gzdirect, and the *64 aliases - stating per row what calling it on an accelerated file does. Notably gzseek and gzrewind move the file descriptor zlib-accel is itself reading or writing, and an application built with -D_FILE_OFFSET_BITS=64 has its gzopen calls redirected to gzopen64, which is not exported, so those files are handled by zlib end to end. Documentation only; no functional change. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Expands gzip API interception to preserve correctness for accelerated multi-member gzip files.
Changes:
- Adds interception for close, parameter, flush, read, and write helpers.
- Tracks compression levels and read push-back state.
- Adds extensive tests and documents supported/unsupported APIs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
zlib_accel.h |
Exposes gzip execution-path inspection for tests. |
zlib_accel.cpp |
Implements expanded gzip API interception and state handling. |
tests/zlib_accel_test.cpp |
Tests new gzip operations and edge cases. |
README.md |
Documents gzip behavior and limitations. |
Suppressed comments (2)
zlib_accel.cpp:3032
gzfreaddelegates togzread, which returns its byte count as anint. A chunk betweenINT_MAX + 1andUINT_MAXcan therefore be read successfully and then convert to a negativeret, causing the loop to stop without accounting for bytes already consumed. Limit delegated reads toINT_MAX.
unsigned chunk =
remaining > UINT_MAX ? UINT_MAX : static_cast<unsigned>(remaining);
zlib_accel.cpp:3099
- Capturing
SEEK_CURis unsafe for append mode, which this common close path accepts. Opening an existing file with"ab"and closing it without writing leaves the descriptor offset at 0; zlib's close appends its empty member, and the subsequenttruncate(..., file_size)then erases the entire existing file. Read the actual end offset before invoking zlib's close.
// Capture file size and name before the close
off_t file_size = lseek(gz->fd, 0, SEEK_CUR);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…eturn gzwrite and gzread answer in an int, so a request they served in full can only be reported when the count fits in one. zlib refuses such a length outright -- gzwrite with 0, gzread with -1 -- and zlib-accel did not: the shim's gzwrite buffered the data and gzread read into the caller's buffer, then handed back a count that had wrapped negative. gzfwrite and gzfread made the same mistake one level up. Both split a z_size_t transfer into gzwrite/gzread calls capped at UINT_MAX, so a transfer above INT_MAX moved every byte to the file and was then reported as zero items, because the negative count from the oversized chunk reads as a failure and ends the loop. Cap the chunk at INT_MAX instead. Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com> Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib bounds gzungetc's push-back by the room left in its own output buffer, and guarantees that a push immediately after the file is opened accepts at least a full buffer's worth of characters. zlib-accel held a single byte and refused every push after it, which breaks a caller that relies on that guarantee -- three pushes after gzopen returned the first byte and then -1 twice, and the following read returned the file's own first bytes with the pushed ones lost. Hold them in a stack instead, and accept unconditionally: the guarantee is about a buffer size zlib-accel does not share, so a bound derived from it could only be a guess, and zlib documents a refusal as permitted rather than required. gzread serves the stack ahead of its own buffers, most recently pushed first, and gzeof counts it as data. A read whose length cannot be reported in an int pops nothing, so a refused read does not consume it. Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com> Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib's gzprintf formats into zlib's own buffer, and a result that does not fit is not written at all: the call returns 0 with the output silently discarded. zlib-accel formats the string itself and writes it whatever its length, so the limit is gone. Keep it that way -- the bound is a property of zlib's buffer rather than of the format request -- and say so, along with the related note that gzbuffer, which is what sets that bound, is not intercepted. Note one more divergence found while checking the int-fit refusals: zlib latches a failed gz* call and refuses every later call on the file, while an accelerated file has no error state to latch, since gzerror is not intercepted. Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com> Signed-off-by: Olasoji <olasoji.denloye@intel.com>
matt-welch
approved these changes
Aug 25, 2026
matt-welch
left a comment
matt-welch
left a comment
There was a problem hiding this comment.
Review: PR #72 gz: close the gz* API interception gaps
Reviewed at HEAD a337d07, base 13685bc.
Build and full test suite run on a clean worktree with USE_IGZIP=ON -DCMAKE_BUILD_TYPE=Debug. 12868/12869 tests pass; the one failure (ConfigLoaderTest.LoadValidConfig) is pre-existing on main and unrelated to this PR. ASAN build succeeds; the ASAN run aborts on a pre-existing new[]/free mismatch in test_utils.cpp::ZlibUncompress before reaching the gz tests.
The PR achieves what it sets out to do. Every entry point that moves bytes is now routed through the shim's own buffers rather than falling back to zlib's idle stream, and the new tests confirm the round-trip is correct for each path. The design is internally consistent. A few things worth looking at below.
Findings
None of the findings below are blockers. 1 is a backlog cleanup item; 2 and 3 are test coverage gaps; 4 is a performance note. Reproducers available on request.
1. gzflush returns Z_ERRNO for a failure that does not involve errno
FlushBufferedWrite (zlib_accel.cpp:2482) returns 1 when orig_deflate or orig_deflateReset is null, without touching errno. gzflush maps any non-zero return from FlushBufferedWrite to Z_ERRNO (zlib_accel.cpp:2654):
// zlib_accel.cpp:2653
if (FlushBufferedWrite(file, gz.get()) != 0) {
return Z_ERRNO; // errno is 0 or stale if the failure was a missing symbol
}Z_ERRNO is the right code when a write fails on the fd, and that is the common case. The symbol-missing path is essentially unreachable in a working build. But if it ever fires, a caller that inspects errno after seeing Z_ERRNO will read a stale or zero value.
The fix is to return Z_STREAM_ERROR from FlushBufferedWrite when the failure is the missing-symbol guard (line 2485), and have gzflush propagate that code unchanged. The current behavior is not going to cause problems in practice. Backlog item, not a blocker.
2. No test for the gzdopen null-registration fix
gzdopen now checks if (file == nullptr) return nullptr at line 2217 before calling gzip_files.Set. This prevents writing a null key into the registry when orig_gzdopen rejects the mode string.
The fix is correct. The existing GzFunctionsOnUnregisteredFile test exercises what happens when you pass a null handle to each function, which confirms the shim handles null handles safely. What it does not cover is the scenario where gzdopen on a bad mode creates the null entry in the first place and later calls find it. Adding a test that opens a file with a bad mode through gzdopen, then calls gzwrite(nullptr, ...) and confirms it returns 0 (unregistered-file result, not the bad entry) would lock this in.
A self-contained probe is in reviews/pr72/probe_gzdopen_null.cpp if that is useful.
3. gzclose_w accepts append-mode files but there is no test for it
GzCloseModeMatches at line 3103 falls through from FileMode::WRITE to FileMode::APPEND and accepts both for gzclose_w:
case FileMode::WRITE:
case FileMode::APPEND:
return mode == FileMode::WRITE || mode == FileMode::APPEND;This is correct: zlib folds append into write mode internally, so gzclose_w on an "ab" file is valid. The commit message explains the intent. The test suite covers gzclose_w on a write-mode file and the mismatch rejection, but not gzclose_w on an append-mode file. A short test that opens with "ab", writes, and closes via gzclose_w would confirm the append case does not return Z_STREAM_ERROR.
4. gzgets does one registry lookup per character on accelerated files
gzgets loops calling gzgetc (line 3037), which calls gzread(file, &ch, 1). Each gzread call does a gzip_files.Get lookup. Data is served from gz->data_buf in memory after the first fill, so there is no I/O per character, but the hash-table lookup and lock acquisition happen once per byte. For line-by-line reading of large files through an accelerated handle this adds up. zlib's gzgetc is a macro that reads directly from a pointer.
No action required now, but worth noting if gzgets throughput ever becomes a concern. The straightforward fix would be to read into a local buffer in gzgets using a single gzread call and scan for the newline locally.
Build artifacts
Both pre-existing test failures are in main and not attributable to this PR:
ConfigLoaderTest.LoadValidConfig: relative path todefault_confignot found from the test build directory.- ASAN:
new[]/freemismatch intest_utils.cpp::ZlibUncompress(line 61), allocation vianew char[], deallocation viaDestroyBlockwhich callsfree(). Should be fixed by changingDestroyBlockto usedelete[]for allocations fromZlibUncompress, or by switchingZlibUncompresstomalloc.
gzflush mapped every non-zero return from FlushBufferedWrite to Z_ERRNO, which tells the caller to go read errno. That is right for the failure that actually reaches it, a failed write, but FlushBufferedWrite also refuses to flush when a zlib symbol it needs is unresolved, and that guard never touches errno; a caller inspecting errno after Z_ERRNO would read a stale or zero value. Return Z_STREAM_ERROR from the guard and let gzflush pass it through unchanged. The other three callers are unaffected: gzwrite collapses any non-zero return to 0, gzsetparams and the shared close path collapse it to Z_STREAM_ERROR already. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib folds append into its own write mode right after opening the file, so gzclose_w accepts a file opened "ab" and the shim's directional check has to accept it too. The suite covered gzclose_w on a write-mode file and the rejection of a mismatched direction, but nothing anywhere opened a file for append, so neither the fall-through that permits it nor the shared close path on that mode was exercised -- and that path flushes the buffered tail and truncates the file back to the size it recorded before zlib's own close, both of which act on a file that already holds members. Write one member, reopen for append, write past the shim's buffer so a tail is left for the close to flush, then decode the whole file and compare both payloads. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
gzdopen does no mode validation of its own -- the descriptor is already open, so there is nothing to protect -- and takes zlib's answer, which for a refused mode is NULL. Registering that would key an entry by NULL, and that entry is what every gz* entry point finds when the application passes NULL, in place of the unregistered-file handling. The guard against it was untested: gzdopen appeared nowhere in the suite. Cover both of zlib's reasons for refusing a mode string, then confirm the null handle still gets the unregistered-file answers rather than an entry's. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
zlib_accel.cpp:3062
len == 1must returnbufcontaining an empty string even though no input byte is read (zlib.h explicitly calls out this case). Since the loop cannot incrementcopied, this branch incorrectly reports EOF and leaves the buffer untouched.
// Nothing read at all means end of file, which zlib reports as no string
// rather than an empty one.
if (copied == 0) {
return nullptr;
}
buf[copied] = '\0';
return buf;
Comment on lines
+2597
to
+2600
| // Forward first: zlib's own checks (write mode, no sticky error, not a | ||
| // transparent file) decide the return value, and zlib has to record the level | ||
| // too, since it is the one that compresses if this file is later handed back. | ||
| const int ret = orig_gzsetparams(file, level, strategy); |
Comment on lines
+2607
to
+2609
| if (FlushBufferedWrite(file, gz.get()) != 0) { | ||
| return Z_STREAM_ERROR; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The shim intercepted six of zlib's gz* functions. Every other gz* call went to zlib, which on an accelerated file works through a deflate/inflate stream that has seen none of the file's data so those calls returned the wrong bytes or corrupted the file. This raises the intercepted set to twenty.
The reason is structural: zlib keeps one continuous stream per gzip file, whereas zlib-accel compresses each buffer it fills into a complete gzip member, so a large file becomes a sequence of members. That is valid gzip, and it is why every call that moves bytes has to be served here rather than handed to zlib; a write through zlib's stream inserts an unrelated member in the middle of ours, and a read through it decodes from the middle of a member.
Five commits:
Nothing changes for a gzFile the shim has no entry for, or one already on the zlib path: those still go straight to zlib. Return values follow zlib's, including the size * nitems overflow refusals.
Deliberately out of scope: gzseek/gzrewind and the rest of the position family move the file descriptor the shim is itself reading or writing; gzopen64 is not exported, so an application built with -D_FILE_OFFSET_BITS=64 has its gzopen calls redirected there and is handled by zlib end to end.