Cap EventCommandList's param_count to what the buffer actually holds - #2
Open
Sqweedward wants to merge 2 commits into
Open
Sqweedward wants to merge 2 commits into
Sqweedward wants to merge 2 commits into
Conversation
EventCommandRef::param_count previously stored the raw, unvalidated header value verbatim. ParamIter implements ExactSizeIterator off that count, so EventCommandList::read's parameters: cmd.params().collect() takes the Vec::with_capacity(size_hint) fast path and reserves for the declared count before reading a single parameter - a corrupted/ truncated file declaring e.g. u32::MAX parameters on a file of a few dozen bytes causes an attempted multi-gigabyte allocation and an abort, not a graceful parse error. The command-parsing loop already scans parameters one at a time and stops the moment varint() runs out of data, so it already knows the *actual* count - track that instead of the declared header value and report DiagKind::ShortChunk when they differ, the same diagnostic List::read_inline already uses for the equivalent "declared more than is present" case elsewhere in this crate. Fixes RPG-Maker-Translation-Tools#1. Confirmed against the fuzzer-found repro from that issue (minimized to 19 bytes) - no longer aborts. Also ran the reporter's own fuzz_database harness for ~150k iterations against this fix with no new crash. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The existing test only covered the all-or-nothing case (u32::MAX declared, zero bytes available). Add the more telling case: some parameters genuinely present, fewer than declared - confirms the fix keeps exactly what's there (values, count) rather than just zeroing out on any mismatch, and that the following next() call terminates cleanly on the now-exhausted buffer.
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.
Fixes #1.
EventCommandRef::param_countpreviously stored the raw, unvalidated header value verbatim.ParamIterimplementsExactSizeIteratoroff that count, soEventCommandList::read'sparameters: cmd.params().collect()takes theVec::with_capacity(size_hint)fast path and reserves for the declared count before reading a single parameter — a corrupted/truncated file declaring e.g.u32::MAXparameters on a file of a few dozen bytes causes an attempted multi-gigabyte allocation and an abort, not a graceful parse error.The command-parsing loop in
EventCommandIter::nextalready scans parameters one at a time and stops the momentvarint()runs out of data — it already knows the actual count. This just tracks that instead of the declared header value, and reportsDiagKind::ShortChunkwhen they differ (the same diagnosticList::read_inlinealready uses for the equivalent "declared more than is present" case elsewhere in this crate, so the fix follows an existing convention rather than inventing a new one).Verification
declared_param_count_is_capped_to_what_is_actually_present) constructing a command that declaresu32::MAXparameters with zero bytes actually available.cargo fuzz tmin) — no longer aborts.fuzz_databaseharness from the issue for ~150k iterations against this fix with no new crash.cargo test --workspace) passes unchanged.Happy to adjust the diagnostic choice or add more coverage if you'd like something different.