Add NDEBUG-safe boundary checks in SnappyIOVecReader::Advance#237
Open
SongT-50 wants to merge 1 commit into
Open
Add NDEBUG-safe boundary checks in SnappyIOVecReader::Advance#237SongT-50 wants to merge 1 commit into
SongT-50 wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
SongT-50
force-pushed
the
fix-iovec-advance-ndebug-boundary
branch
from
May 1, 2026 08:14
de1899c to
4be8f6f
Compare
Author
|
Hi — a gentle ping on this PR. I know maintainers are busy, so no rush at all. If there's anything I can do to help it move forward (rebase, extra tests, or clarifying the boundary-check rationale), I'm happy to. Thanks for maintaining snappy! |
The current Advance() relies on a debug-only assert to enforce the invariant
total_size_remaining_ >= curr_size_remaining_. Under NDEBUG (most production
builds) the assert is stripped, leaving the inner do-while loop unguarded
against caller-misuse of the raw API RawCompressFromIOVec(iov,
uncompressed_length, ...) when uncompressed_length does not match the actual
sum of iov_len values.
Two defensive checks are added (interim, NDEBUG-safe):
1. If total_size_remaining_ < curr_size_remaining_ on entry, saturate to
no-data state and bail out rather than underflowing total_size_remaining_
into a large unsigned value below.
2. Cap consecutive zero-length iovec walks at kMaxEmptyWalks (1024). A run
of zero-length trailing entries with nonzero claimed total_size would
otherwise spin past the array end via the do-while loop.
The default safe wrapper CompressFromIOVec(iov, iov_cnt, ...) already
computes the total itself before calling the raw API and is unaffected by
this change. The patch is backward compatible (API signature unchanged).
A more permanent API-level fix is to add a safe overload of
RawCompressFromIOVec that takes iov_cnt explicitly and deprecate the current
3-arg form. Happy to prepare a follow-up PR for that direction if
maintainers prefer.
Findings cross-verified through independent code auditing (multi-model code
review pipeline).
SongT-50
force-pushed
the
fix-iovec-advance-ndebug-boundary
branch
from
July 6, 2026 15:02
4be8f6f to
87425bc
Compare
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
1 participant
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.
Summary
Under
NDEBUG(release builds)SnappyIOVecReader::Advance()(snappy.cc:1937) loses itsonly bounds guard — a debug-only
assert(total_size_remaining_ >= curr_size_remaining_).When a caller of the raw API
RawCompressFromIOVec(iov, uncompressed_length, ...)passes anuncompressed_lengthlarger than the actual sum ofiov_len,Advance()walkscurr_iov_past the end of the iovec array (OOB read / DoS). A run of zero-length trailing entries
triggers the same walk.
The safe public wrapper
CompressFromIOVec(iov, iov_cnt, ...)computes the total itself andis unaffected — only direct raw-API callers are exposed.
Fix
Two NDEBUG-safe guards in
Advance()(noassertreliance; API signature unchanged; normalinputs take the same path):
total_size_remaining_ < curr_size_remaining_(prevents the unsigned underflow that leads to the past-end walk).
kMaxEmptyWalks;1024is a placeholder — maintainersmay pick a value that fits the project's convention).
Test
Three edge cases (normal /
total_size > sum(iov_len)/ zero-length trailing) build clean under-fsanitize=address,undefined; a standalone repro is available on request, and I'm happy to adda
snappy_unittestcase.A permanent API-level fix (add an
iov_cntoverload and deprecate the 3-arg raw API) is thecleaner direction — happy to follow up if you prefer that instead of / in addition to this interim guard.