fix(debugger): saturate memory-pane offset/len arithmetic - #16575
Open
gomesalexandre wants to merge 3 commits into
Open
fix(debugger): saturate memory-pane offset/len arithmetic#16575gomesalexandre wants to merge 3 commits into
gomesalexandre wants to merge 3 commits into
Conversation
The read-region end (offset + len) and write-region end (write_offset + write_size) used raw addition on values that come from saturating_to() on a raw U256, so usize::MAX is a legitimate input - the unfixed half of foundry-rs#6472. PR foundry-rs#6474 fixed the U256::to() panic by switching to saturating_to(), which moved the failure one frame downstream into this still-unguarded arithmetic. In a debug build this is 'attempt to add with overflow'; in release it wraps, so the highlight for the exact byte access being diagnosed silently disappears and the wrapped value can poison the overlap check below. Fix: saturating_add for both end computations, matching the class of guard already used elsewhere in this file.
Extract offset+len saturation into a const fn helper (saturating_region_end) and unit-test it directly for both the read-region and write-region cases. The existing UI-level regression test can only exercise the read-region branch: write_offset/write_size are only populated when active_buffer == Memory, and there's no way to construct a non-empty step.memory in this test module since RecordedMemory's content constructor is crate-private to revm-inspectors - so the write branch was previously untested and would still pass CI if its saturating_add were accidentally reverted. Caught by Codex adversarial review.
gomesalexandre
marked this pull request as ready for review
September 2, 2026 19:35
gomesalexandre
requested review from
0xrusowsky,
DaniPopes,
figtracer,
grandizzy,
mablr,
mattsse and
stevencartavia
as code owners
September 2, 2026 19:35
Contributor
✅ Changelog foundThe deterministic check will validate the changed entry. |
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.
What
The debugger's memory pane computes a read-region end (
offset + len) and a write-region end (write_offset + write_size) using raw addition. Both operands come fromget_buffer_accesses, which internally callssaturating_to()on a rawU256stack value — sousize::MAXis a legitimate input (e.g. a stack value produced bynot(0)in inline assembly), not just a theoretical one.This is the unfixed half of #6472. #6474 fixed the original
U256::to()panic by switching tosaturating_to(), which moved the failure one frame downstream into this still-unguarded arithmetic:attempt to add with overflowpanic.32 + usize::MAX == 31— the highlight for the exact byte access being diagnosed silently disappears, and the wrapped value can poison the overlap-detection logic below it.Fix
saturating_addfor both end computations, matching the class of guard already used elsewhere in this codebase. Extracted into a smallconst fnhelper (saturating_region_end) so both call sites share one implementation and it's directly unit-testable.Testing
draw_buffer_does_not_overflow_on_saturated_stack_offsets: a real UI-level regression test — a step with an all-U256::MAXstack, routed through the debugger's actualdraw_bufferrendering path. Confirmed red before the fix (panics withattempt to add with overflow), green after.saturating_region_end_handles_normal_and_saturated_inputs: direct unit tests on the extracted helper, covering both the read-region and write-region cases. This closes a real gap a synchronous Codex adversarial review caught — the UI-level test can only reach the read-region branch (write_offset/write_sizeare only populated whenactive_buffer == Memory, and there's no way to construct a non-emptystep.memoryin this test module sinceRecordedMemory's content constructor is crate-private to revm-inspectors), so the write-regionsaturating_addwas previously untested and would still pass CI if accidentally reverted.foundry-debuggercrate suite: 124/124 passing.cargo clippy -p foundry-debugger --lib -- -D warnings: clean (also fixed amissing_const_for_fnlint on the new helper).cargo fmt --check: clean.No merge, no self-approve.