Add NDEBUG-safe boundary checks in SnappyIOVecReader::Advance - #237
Add NDEBUG-safe boundary checks in SnappyIOVecReader::Advance#237SongT-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. |
de1899c to
4be8f6f
Compare
|
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).
4be8f6f to
87425bc
Compare
|
Hi @danilak-G — sorry to ping you directly. I saw you reviewed and merged #242 recently, so I hope it's alright to ask here. Status: CLA signed, check-changes passing, no conflicts. Short version: under NDEBUG the only bounds guard in Advance() is a debug-only assert, I'm not attached to the specific numbers — kMaxEmptyWalks is a placeholder and I'd happily No rush at all, and please ignore this if it's not a priority. Thanks for maintaining snappy. |
1 participant
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.