deflate/inflate: record terminal state after Z_STREAM_END - #71
Open
asonje wants to merge 5 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds terminal-state tracking so accelerated streams preserve zlib-compatible behavior after Z_STREAM_END.
Changes:
- Tracks terminal state across calls, resets, and stream copies.
- Intercepts copy and
inflateReset2APIs. - Adds ISA-L cloning helpers, documentation, and regression tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
zlib_accel.cpp |
Implements terminal-state and copy/reset handling. |
tests/zlib_accel_test.cpp |
Adds regression coverage. |
README.md |
Documents behavior and limitations. |
igzip.h |
Declares ISA-L state helpers. |
igzip.cpp |
Implements inflate cloning and window comparison. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
978
to
979
| PrintDeflateBlockHeader(LogLevel::LOG_INFO, strm->next_in, strm->avail_in, | ||
| inflate_settings->window_bits); |
Comment on lines
+871
to
+874
| if (IgzipOwnsDeflateStream(deflate_settings)) { | ||
| Log(LogLevel::LOG_INFO, "deflateCopy Line ", __LINE__, | ||
| " rejected, ISA-L holds live state for source stream\n"); | ||
| return Z_STREAM_ERROR; |
Comment on lines
+1308
to
+1313
| // inflateReset2() is the only zlib entry point that changes windowBits on a | ||
| // live stream, so it is the only one that can restart a finished stream without | ||
| // going through inflateReset(). It has to be intercepted for two reasons: the | ||
| // recorded window_bits would otherwise go stale and path selection would keep | ||
| // deciding on the format the stream was initialized with, and a stream left | ||
| // marked as ended would keep returning Z_STREAM_END forever. |
An offloaded stream never feeds zlib's own deflate or inflate state, and nothing else recorded that the stream had ended, so every call after Z_STREAM_END was dispatched from scratch. Measured on 64 KiB at level 6: QAT and IAA appended bytes zlib would not (+8 is a complete empty zlib stream after a finished one, +2 a second header), all from orig_deflate, since a non-Z_FINISH second call is not offloadable and path selection hands it to a zlib state still at INIT_STATE; IGZIP consumed the whole 64 KiB, emitted nothing, and reported success. Record the completion per stream and answer later calls from that state, above path selection and above the zlib fall-through. The gate runs zlib's own parameter checks in zlib's own order, which is not the obvious order and was measured against a no-shim oracle: avail_out == 0 outranks the terminal state on deflate but not on inflate, a NULL next_out or a NULL next_in with input pending outranks it on both, and inflate accepts any flush value where deflate rejects everything but Z_FINISH. All 22 rows now match plain zlib on every path. inflateReset2 has to be intercepted for this to be safe: it is the one API that legitimately restarts a finished stream without inflateReset, so a flag left set there would wedge the stream at Z_STREAM_END forever. Intercepting it also fixes a pre-existing bug, a stale window_bits and a stale ISA-L format after a window or format change, since isal_inflate_reset() preserves crc_flag and hist_bits and the stream must therefore be discarded rather than reset. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib's deflate() and inflate() reject a null next_out, and a null next_in with avail_in != 0, before they look at anything else. The shim reads both before that point: inflate() logs the incoming deflate block header, probes the zlib header for the FDICT bit and asks IsIAADecompressible() about the input, and both entry points hand next_in/next_out straight to a vendor library once a backend is selected. An application that calls inflate() with next_in == NULL and avail_in != 0 -- a Z_STREAM_ERROR without the shim -- segfaults with it, at the default log level, on a stream that has not ended. Delegate such a call to zlib from both entry points, below the terminal-state gate and above path selection. zlib's parameter checks touch no stream state, so a delegated call is indistinguishable from an unshimmed one. PrintDeflateBlockHeader() is also fixed at the source, since its guard covered a short buffer but not a null one, and it is reachable from any caller. The terminal-state rows already cover both pointers, but only on a finished stream, where the gate answers them before any of these reads. The new cases cover a live stream: five of them crash the suite without this change. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
deflateReset() is deflateResetKeep() plus lm_init(), and inflateReset() is inflateResetKeep() plus a discarded window. Both Keep forms are declared in zlib.h among the functions zlib does not document and both are exported by libz, so an application can restart a stream without going through the reset entry points this shim intercepts. With the terminal state left set, every later call on such a stream answers from the gate: Z_BUF_ERROR on deflate and Z_STREAM_END with no output on inflate, forever. That is the wedge the design rejected z_stream::reserved to avoid, so intercept both. The shim-side state work each reset performs is now one helper per direction, shared by deflateReset, deflateResetKeep, inflateReset, inflateResetKeep and inflateReset2. inflateResetKeep also pins the stream to ZLIB. Neither reset frees the window, so retaining its contents is the only reason to call the Keep form: the caller is saying the next stream may reference the previous stream's bytes. No backend can see that history -- IGZIP would decode a lookback against whatever its own window holds, and can consume input and emit output before reaching an invalid distance, so a later fall-through to zlib would decode with a window missing those bytes. Pinning is what inflateSetDictionary() does for the same reason, and a later inflateReset lifts it. deflateResetKeep needs no pin: the LZ77 window it keeps only affects how zlib would encode the next stream, while an offloaded stream emits a self-contained one that any decoder accepts. deflateReset and inflateReset now forward to zlib first and do their state work on Z_OK, matching deflateParams and inflateReset2. zlib builds each Reset on its ResetKeep, so on a libz whose internal calls are interposable the Keep wrapper runs nested inside the outer one; acting last is what keeps inflateResetKeep's pin from leaking into every ordinary inflateReset. Six cases in TerminalStateRegressionTest, one per direction per backend; all six fail without this. The inflate ones run with use_zlib_uncompress=0, so the stream after inflateResetKeep only decodes if the pin itself reaches zlib. Their payloads come from a new deterministic generator rather than GenerateBlock(). GenerateRandomString() draws from std::rand(), which nothing in the suite seeds and every test shares, so six tests drawing 64 KiB each shift the payload of every parameterized case that runs afterwards. That is not hypothetical: with GenerateBlock() the suite went 51093/44852/6240/1, the one failure being case 25841 (IGZIP compress, QAT uncompress, incompressible, 256 KiB, gzip, chunking on), whose new payload QATzip refuses with QZ_FAIL so the shim falls back to zlib and the case's execution-path assertion fails. Removing any one earlier rand-drawing test made it pass again, which is what identifies the sequence rather than these tests as the cause. Seeding locally keeps the rest of the suite on the payloads it was measured with: same 51082 baseline results plus these cases, and the same set of QAT refusals. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
deflateCopy() refused any source stream ISA-L owned, which is right while the stream is under way -- isal_zstream::level_buf points into its own allocation, so a copy would share the source's pending block -- but wrong once the stream has finished. ISA-L is at ZSTATE_END with all its output delivered at that point, so there is nothing left to duplicate, and the terminal state the copy inherits answers every deflate() made on it. That is exactly the handling QAT and IAA already get, so IGZIP was the odd path out for no state reason. The guard now also requires that the source has not reached Z_STREAM_END. The copy lands with path IGZIP and no ISA-L stream, which every consumer already handles: deflate() answers from the terminal-state gate above path selection, deflateEnd() releases nothing, deflateReset() clears both path and flag so the next stream re-selects, and deflateParams()' ISA-L discard is null-guarded. No ISA-L state is shared, so the no-sharing invariant holds. IGZIPRefusesFinishedDeflateCopy becomes IGZIPFinishedDeflateCopyRefusesInput, calling the same helper the QAT and IAA cases use; IGZIPRefusesMidstreamDeflateCopy still covers the live stream. Net test count is unchanged. Also strips the measurements out of the comments this branch adds, the pass already made over the stream-copy comments: byte deltas and buffer sizes are pinned to one library version and one host and go stale where nobody re-runs them, so the comments state the mechanism and the figures stay in the commit messages, which are dated. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
asonje
force-pushed
the
pr-terminal-state
branch
from
August 19, 2026 19:02
c29d4d0 to
ca3368d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
zlib_accel.cpp:1465
inflateResetKeep()cannot preserve history by pinning an already offloaded stream to zlib. The zlib inflate state never received the previous stream, so its retained window is empty; a following stream produced withdeflateResetKeep()may reference prior-stream bytes andorig_inflate()will fail even though this wrapper returnedZ_OK. The new test only replays a self-contained stream. Please either reject this operation for non-ZLIB paths before mutating state, or retain and restore the actual decoded history.
const int ret = orig_inflateResetKeep(strm);
if (ret == Z_OK) {
auto inflate_settings = inflate_stream_settings.Get(strm);
ResetInflateStreamState(inflate_settings);
SetInflatePath(inflate_settings, ZLIB);
zlib_accel.cpp:1140
- This direct delegation similarly omits
INFLATE_ZLIB_COUNTandINFLATE_ERROR_COUNT. A null-pointer rejection therefore disappears from the backend/error statistics even though the call is counted and returns a negative zlib error. Record the delegated call and result before returning.
if (strm->next_out == nullptr ||
(strm->next_in == nullptr && strm->avail_in != 0)) {
return orig_inflate != nullptr ? orig_inflate(strm, flush)
: Z_VERSION_ERROR;
zlib_accel.cpp:702
- This direct delegation bypasses statistics accounting. With
ENABLE_STATISTICS, a rejected null-pointer call incrementsDEFLATE_COUNTbut neitherDEFLATE_ZLIB_COUNTnorDEFLATE_ERROR_COUNT, although zlib returnsZ_STREAM_ERROR; the normal delegation below records both. Capture the result and update those counters before returning.
This issue also appears on line 1137 of the same file.
if (strm->next_out == nullptr ||
(strm->avail_in != 0 && strm->next_in == nullptr)) {
return orig_deflate != nullptr ? orig_deflate(strm, flush)
: Z_VERSION_ERROR;
The null-pointer delegations added with the terminal-state gate returned
zlib's verdict without recording anything, so a call that reached
orig_deflate()/orig_inflate() and was rejected there stayed invisible to
an ENABLE_STATISTICS build -- unlike the gate immediately below them, which
records DEFLATE_ERROR_COUNT, and unlike the normal zlib fall-through, which
records both counters. Record *_ZLIB_COUNT and *_ERROR_COUNT on both.
inflateResetKeep pins the stream to zlib because the window it retains is a
preset dictionary in all but name. What the pin cannot do is put that
history into zlib's window: an offloaded stream never fed it, so a next
stream that really does reference the previous stream's bytes fails with
Z_DATA_ERROR where unaccelerated zlib decodes it. That much is inherent to
offloading -- the bytes exist only in the output the accelerator already
returned to the caller, and whether a later stream will reference them is
unknowable while the previous one is still being decoded. What the pin buys
is the failure mode: a zlib data error on the stream that needs the history,
rather than a backend resolving the lookback against an unrelated window and
reporting success. Rejecting the reset outright would be worse still, failing
the common history-independent restart to report the rare case earlier.
Recorded in the README and at the interception.
{IGZIP,QAT,IAA}InflateResetKeepHistoryDependentStreamFails pins it. The
pair is built with a preset dictionary on a raw stream, where zlib sets the
encoder's window without an FDICT bit, so the second stream is ordinary
deflate whose distances reach behind its own first byte -- what a window
retained across inflateResetKeep is supposed to supply. The test asserts
the dependence itself (65 bytes for 8 KiB of input, undecodable standalone
with "invalid distance too far back"), then that the all-zlib path decodes
it byte for byte after inflateResetKeep, then that it comes back
Z_DATA_ERROR once the previous stream was offloaded.
Suite with all three backends: 51096 / 44855 / 6241 / 0. The IAA case
skips -- data zlib produced with a 32 KiB window is not IAA-decodable, so
its first stream falls back to zlib and feeds zlib's window, which is the
case the all-zlib half already covers.
Signed-off-by: Olasoji <olasoji.denloye@intel.com>
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.
An offloaded stream never feeds zlib's own deflate or inflate state, and nothing else recorded that the stream had ended, so every call after Z_STREAM_END was dispatched from scratch. This PR records the completion per stream and answer later calls from that state, above path selection and above the zlib fall-through correctly.