fix(arrow-avro): bound untrusted OCF block size and item counts#10237
Open
miniex wants to merge 4 commits into
Open
fix(arrow-avro): bound untrusted OCF block size and item counts#10237miniex wants to merge 4 commits into
miniex wants to merge 4 commits into
Conversation
github-actions
Bot
added
arrow
Contributor
|
run benchmarks avro_reader decoder |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/avro-untrusted-block-bounds (5907f95) to 7616e10 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/avro-untrusted-block-bounds (5907f95) to 7616e10 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Jefffrey
reviewed
Jul 1, 2026
A crafted OCF block size of i64::MAX hit Vec::reserve before any payload was read, aborting the process. Reserve only what the input backs. Closes apache#10234
A block count of i64::MAX spun the item loop forever for zero-byte items like null, and i64::MIN overflowed the negation. Reject counts above the bytes remaining and negate with unsigned_abs. Closes apache#10235
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
The old check rejected any block with more items than bytes remaining, wrongly refusing valid `array<null>` (zero-byte items). Cap the item total at `i32::MAX` instead; oversized counts still error before the loop.
miniex
force-pushed
the
fix/avro-untrusted-block-bounds
branch
from
605a62c to
83f3909
Compare
Jefffrey
reviewed
Jul 2, 2026
Comment on lines
+2317
to
+2328
| /// Decode `count` items, capping the running total at `i32::MAX` (the largest index | ||
| /// an Arrow list/map offset holds). Otherwise a crafted `i64::MAX` count of a zero-byte | ||
| /// item like `null` spins the loop forever (#10235); byte-consuming items self-terminate | ||
| /// on cursor exhaustion, so valid blocks (including `array<null>`) are unaffected. | ||
| #[inline] | ||
| fn process_block_items( | ||
| buf: &mut AvroCursor, | ||
| count: usize, | ||
| total: usize, | ||
| on_item: &mut impl FnMut(&mut AvroCursor) -> Result<(), AvroError>, | ||
| ) -> Result<usize, AvroError> { | ||
| let new_total = total.checked_add(count).filter(|&t| t <= i32::MAX as usize); |
Contributor
There was a problem hiding this comment.
i'm not sure about hard limiting to i32::MAX either considering we have LargeLists which have i64 offsets; i'll refer to my previous question, in wondering how other avro implementations deal with this issue?
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
3 participants
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.
Which issue does this PR close?
Rationale for this change
The Avro OCF reader trusts two length fields straight from the input, so a small crafted file can take down a process that parses untrusted Avro. A block size of
i64::MAXreachesVec::reservebefore any payload is read and aborts the process on a huge allocation (#10234). A block count ofi64::MAXspins the array/map item loop forever for a zero-byte item type likenull, andi64::MINoverflows the negative-count negation (#10235).What changes are included in this PR?
block.rs: reserve only what the current input buffer backs, and let the rest grow as data arrives.record.rs: reject a block item count larger than the bytes left to decode, and take the negative-count magnitude withunsigned_abs.cursor.rs: addAvroCursor::remaining(), used by that bound.A count past the remaining bytes can only describe items that are not there. Items that read input each need at least one byte, so only the zero-byte case is rejected and valid blocks keep working.
Are these changes tested?
Yes. The new tests hang or abort before this change and pass after. In
reader::block, ani64::MAXblock size stays bounded instead of aborting, a negative size errors, and a well-formed block still round-trips. Inreader::record,i64::MAXandi64::MINblock counts on anarray<null>now error instead of spinning the item loop.fmt --checkandclippyare clean.Are there any user-facing changes?
Malformed Avro OCF input that used to abort or hang now returns a clean
AvroError. There are no public API changes.I'm Korean, so sorry if any wording reads a little awkward.