Pr inflate midstream error - #75
Open
asonje wants to merge 7 commits into
Open
Conversation
When IGZIP rejected a chunk, inflate() pinned the stream to ZLIB and fell through to orig_deflate's counterpart. zlib's inflate state had never seen the earlier chunks -- no history, and next_in already advanced by the calls that succeeded -- so it parsed a byte from the middle of a deflate block as the first byte of a new stream. On zlib and gzip streams that misparse is rejected as a bad header, but the caller still lost the failing call's output; on raw deflate there is no header to reject, so the junk it emitted was counted as output, and at 2 of 58 input alignments the stream returned Z_STREAM_END on a decode that was 9 KB / 44 KB short. Measured against a no-shim oracle: 21 of 21 rows divergent across raw/zlib/gzip and every chunk size. Gate the pin on a new InflateSettings::bytes_consumed, set by the new AdvanceInflateStream helper that now owns the six manual field updates in the translation block. Once an offloaded call has consumed input there is no engine left that can finish the stream, so the failing call delivers what it decoded and returns Z_DATA_ERROR. A failure on the first call keeps today's fall-through: nothing was consumed, so zlib really does re-read the stream from its start -- the claim the comment at the fall-through used to make unconditionally, now corrected. strm->total_in was rejected as a free substitute for the flag because it is the application's field to reset. Three consequences. Z_NEED_DICT is untouched in practice: ISA-L reports it from the zlib header, i.e. on the first call, where the gate does not apply; the one way to reach it later is an FDICT bit fed a byte at a time, which is already broken today, so gating both error sites is no worse. The ISA-L state is no longer stranded by a pin, so inflate()'s path == ZLIB release is not involved and the state is freed at inflateEnd as usual. And the repeat call after the error had to be measured rather than assumed: ISA-L does not latch a data error the way zlib's BAD state does -- a second call on the failed state returned Z_OK with 58 bytes of junk -- so InflateSettings::data_error holds the stream at Z_DATA_ERROR, cleared by the resets and carried by SetFromCopy alongside bytes_consumed. inflateSync is now intercepted because it is the one call that legitimately resumes a stream elsewhere: it searches for a full-flush point and discards the decode state. On Z_OK or Z_DATA_ERROR it clears the latch for a stream that has consumed input, and pins that stream to ZLIB -- zlib performed the search and its state sits at the flush point, while ISA-L still holds the bits it read ahead of the failure and recovers only when the failure happened to leave it byte-aligned, which measurement confirmed is chunk-dependent luck. Z_BUF_ERROR and Z_STREAM_ERROR leave zlib's state untouched, so they leave the engine alone. Latched calls are counted as inflate_failed_stream_count, for the reason inflate_stream_end_count exists: no engine executes them, and the per-engine counters have to keep adding up to inflate_count. The mid-stream error itself is already counted under igzip. Adding the name to stat_names reflows the array to one entry per line, which is clang-format's choice at 21 elements. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
The compress-side twin of the inflate gate, and the worse shape of the two: a failed IGZIP call on a stream that has already emitted bytes used to fall through to orig_deflate, which holds none of that stream in zlib's deflate state and would therefore start a second stream -- a fresh header -- in the middle of the output and return Z_OK. Return Z_STREAM_ERROR instead. This is defensive: nothing reaches it today and no test covers it, which removing the guard confirms -- the full suite still passes. The reason it is unreachable is an ISA-L internal rather than a contract, which is why the guard is worth its lines: isal_deflate's two rejections are a flush value CompressIGZIP screens beforehand and a level/level_buf mismatch fixed at InitCompressIGZIP, which is never rebuilt while ISA-L owns the stream. A ZLIB-pinned stream cannot trip the guard, since IgzipOwnsDeflateStream requires path == IGZIP and a non-null ISA-L stream. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Seven cases in InflateMidstreamErrorRegressionTest, all inside USE_IGZIP since only a stateful engine can fail mid-stream. Each builds a stream whose valid Z_SYNC_FLUSH-terminated prefix is followed by a reserved block type (bfinal 1, btype 11b) and feeds it in chunks so the fault lands on a later inflate() call. Payloads come from GenerateSeededCompressibleBlock so the suite's shared rand() sequence is undisturbed. The raw case is the one that used to report success: it asserts the call never returns Z_STREAM_END, that total_out stops at the true length, and that every delivered byte matches the original. The zlib and gzip cases assert the return code is unchanged from today but the failing call's output now reaches the caller. The first-call case asserts the fall-through to zlib survives, which is what makes the bytes_consumed condition load-bearing in both directions -- dropping the condition fails exactly that test, and zeroing the output advance fails six of the seven. The QAT and IAA rows exercise the same corrupt stream through igzip_fallback, the deployed shape. The inflateSync case builds three independently decodable Z_FULL_FLUSH segments, smashes the middle one, and asserts the third decodes after the resync and that the stream is on ZLIB. Two zlib behaviours it has to work with: deflateEnd returns Z_DATA_ERROR when a stream is ended before Z_FINISH, and inflateSync only scans the input currently available, so the remainder has to be repositioned before the call. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Two paragraphs beside the terminal-state gate: what a mid-stream decode failure returns and why the stream is not handed to zlib to finish, including the first-call exception and that it applies to IGZIP and to QAT/IAA through igzip_fallback; and why inflateSync pins the stream to zlib. inflateSync joins the list of intercepted inflate functions. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate correctness and test-isolation issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents unsafe midstream fallback from IGZIP to zlib after corrupt inflate data.
Changes:
- Latches midstream inflate errors and preserves decoded output.
- Adds
inflateSyncrecovery and compression-side safeguards. - Adds regression tests, statistics, and documentation.
File summaries
| File | Review |
|---|---|
zlib_accel.cpp |
Two moderate issues: fragmented preset-dictionary streams can incorrectly latch Z_DATA_ERROR; interposed inflateSync() can fail to pin resumed streams to zlib. |
tests/zlib_accel_test.cpp |
Moderate issue: the fixture does not reliably restore process-global path settings, making tests order-dependent. |
statistics.h |
Adds the failed-stream statistic. |
statistics.cpp |
Registers the new statistic name. |
README.md |
Documents failure and synchronization behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| #ifdef USE_IGZIP | ||
| class InflateMidstreamErrorRegressionTest : public ::testing::Test {}; |
Comment on lines
+1465
to
+1466
| igzip_midstream_error = HandleMidStreamIGZIPInflateError( | ||
| strm, inflate_settings, input_len, output_len, &ret); |
Comment on lines
+1724
to
+1742
| const int ret = orig_inflateSync(strm); | ||
|
|
||
| // Z_OK is a completed search and Z_DATA_ERROR one that ran out of input | ||
| // looking; both leave zlib's state holding the outcome, and zlib answers a | ||
| // failed search with Z_STREAM_ERROR on every later inflate() call, which is | ||
| // its own state to report. Z_BUF_ERROR and Z_STREAM_ERROR are refusals that | ||
| // change nothing, so a stream that only got those keeps its engine. | ||
| if (ret != Z_OK && ret != Z_DATA_ERROR) { | ||
| return ret; | ||
| } | ||
|
|
||
| auto inflate_settings = inflate_stream_settings.Get(strm); | ||
| if (inflate_settings != nullptr && inflate_settings->bytes_consumed && | ||
| !inflate_settings->stream_end_reached) { | ||
| inflate_settings->data_error = false; | ||
| // Any ISA-L stream this puts out of reach is handed back by the next | ||
| // inflate(), the same way inflateResetKeep()'s pin releases it. | ||
| SetInflatePath(inflate_settings, ZLIB); | ||
| } |
The three helpers behind InflateMidstreamErrorRegressionTest restored igzip_fallback to a literal 0 rather than to the value the option held, and they left the eight per-engine path options -- plus the two that SetCompressPath/SetUncompressPath write unconditionally -- wherever the test had put them. An ASSERT_* failure skipped even the one restore they did make, since it returns from the helper. Snapshot the options in SetUp and put them back in TearDown, the way InflateFlushGateTest and GzipFileTest already do. TearDown runs whether or not an assertion fired, which is the point: a helper that dies half-way otherwise reconfigures every test that runs after it, and the suite's failures then depend on its order. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
FDICT lives in the second byte of a zlib header, so the probe that keeps dictionary streams off the accelerators needed two bytes in one call to see it. A caller that hands inflate() the first byte on its own defeated that, and the byte is not recoverable afterwards: ISA-L copies a partial header into its own buffer and reports the input consumed, so when the next call completes the header and asks for the dictionary, the bytes zlib would need to parse that header itself are gone from the caller's buffer. Measured against a no-shim oracle, feeding an FDICT stream one byte per call returned Z_DATA_ERROR with nothing decoded where zlib returns Z_NEED_DICT and then decodes the stream; chunks of two bytes and up already matched, before and after the mid-stream gate. Pin a single-byte call too, which is the last point where the decision can still be made. The cost is that a zlib-format stream whose first call happens to carry one byte stays on zlib even if it turns out to have no dictionary. An empty call is not pinned: it consumes nothing, so the next one still gets to look. Also add the missing next_in null check to the same condition -- the probe dereferenced next_in[1] on a call zlib itself answers with Z_STREAM_ERROR. Signed-off-by: Olasoji <olasoji.denloye@intel.com>
zlib builds inflateSync() on inflateReset(), so on a libz whose internal call is interposable that call lands in this file's inflateReset(), which clears bytes_consumed and data_error. Reading them after the original returned would then see a stream that had consumed nothing and skip the pin -- on the one entry point whose whole purpose is to pin. Snapshot the decision before calling the original instead. Ubuntu's libz binds that internal call in-object, so this cannot be reproduced here; the same trap cost #71 a wrong release site, which is why it is closed by construction rather than left to a test. 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.
When igzip hits corrupt data, the shim falls back to zlib. That works on the first call of a stream, but not later on: zlib never saw the earlier chunks, so it has no idea where in the stream it is and cannot pick the decode up. It reads the next byte as the start of a brand new stream. On raw deflate there is no header for it to reject, so it emits junk and counts it as output, and at some input alignments it even returns
Z_STREAM_END; success on a decode that is tens of kilobytes short.zlib in this situation would report a data error, so that is what the shim now does: the failing call hands back the bytes igzip did decode, returns
Z_DATA_ERROR, and every later call on that stream returns it too (zlib latches the error, igzip does not, so the shim keeps the flag). A failure on the first call still falls back to zlib, which really can read the stream from the beginning.The one call that legitimately restarts a stream elsewhere is
inflateSync, so it is now intercepted: it clears the error and pins the stream to zlib, which is the only engine that knows where the resync landed.The same fall-back exists on the compress side, where it would be worse; a second header written into the middle of the output, reported as success. It is not reachable today, but it is now guarded.
igzip-only, including QAT and IAA reaching it through
igzip_fallback. 7 new tests; suite, backends-off build and fuzzer all clean, QAT refusals unchanged.