Skip to content

fix(debugger): saturate memory-pane offset/len arithmetic - #16575

Open
gomesalexandre wants to merge 3 commits into
foundry-rs:masterfrom
gomesalexandre:fix/debugger-memory-pane-saturating-add
Open

fix(debugger): saturate memory-pane offset/len arithmetic#16575
gomesalexandre wants to merge 3 commits into
foundry-rs:masterfrom
gomesalexandre:fix/debugger-memory-pane-saturating-add

Conversation

@gomesalexandre

Copy link
Copy Markdown
Contributor

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 from get_buffer_accesses, which internally calls saturating_to() on a raw U256 stack value — so usize::MAX is a legitimate input (e.g. a stack value produced by not(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 to saturating_to(), which moved the failure one frame downstream into this still-unguarded arithmetic:

  • Debug build: attempt to add with overflow panic.
  • Release build: wraps, so 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_add for both end computations, matching the class of guard already used elsewhere in this codebase. Extracted into a small const fn helper (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::MAX stack, routed through the debugger's actual draw_buffer rendering path. Confirmed red before the fix (panics with attempt 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_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-region saturating_add was previously untested and would still pass CI if accidentally reverted.
  • Full foundry-debugger crate suite: 124/124 passing.
  • cargo clippy -p foundry-debugger --lib -- -D warnings: clean (also fixed a missing_const_for_fn lint on the new helper).
  • cargo fmt --check: clean.

No merge, no self-approve.

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.
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

✅ Changelog found

The deterministic check will validate the changed entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants