Skip to content

fix(symbolic): reject oversized memory offsets - #16183

Open
stevencartavia wants to merge 15 commits into
masterfrom
steven/fix-symbolic-memory
Open

fix(symbolic): reject oversized memory offsets#16183
stevencartavia wants to merge 15 commits into
masterfrom
steven/fix-symbolic-memory

Conversation

@stevencartavia

Copy link
Copy Markdown
Member

Rejects unrepresentable offsets for symbolic MLOAD, MSTORE, and MSTORE8, matching concrete EVM failure behavior and preventing false passes.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Changelog found

The deterministic check will validate the changed entry.

mattsse
mattsse previously approved these changes Aug 14, 2026

@mattsse mattsse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread crates/evm/symbolic/src/executor/opcodes.rs
Comment thread crates/forge/tests/cli/test_cmd/symbolic_memory.rs
Comment thread crates/evm/symbolic/src/executor/opcodes.rs Outdated
Comment thread crates/evm/symbolic/src/executor/opcodes.rs
@stevencartavia
stevencartavia requested a review from mablr August 14, 2026 16:43
Comment thread crates/evm/symbolic/src/executor/opcodes.rs Outdated
Comment thread crates/evm/symbolic/src/executor/opcodes.rs Outdated
Comment thread crates/forge/tests/cli/test_cmd/symbolic_memory.rs Outdated
@stevencartavia
stevencartavia requested a review from mablr August 14, 2026 19:54
Comment thread crates/evm/symbolic/src/runtime/state.rs Outdated
Comment thread crates/evm/symbolic/src/runtime/state.rs Outdated
@stevencartavia
stevencartavia requested a review from mablr August 17, 2026 17:10
Comment thread crates/evm/symbolic/src/runtime/memory.rs Outdated
@stevencartavia
stevencartavia requested a review from mablr August 17, 2026 18:38
mablr
mablr previously approved these changes Aug 17, 2026

@figtracer figtracer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found two concrete/symbolic parity blockers and two hot-path representation costs on the current head. the full symbolic crate suite and the focused cli tests pass, but one added regression currently codifies a false proof.

Comment thread crates/evm/symbolic/src/runtime/state.rs Outdated
Comment thread crates/evm/symbolic/src/executor/create.rs Outdated
Comment thread crates/evm/symbolic/src/executor/calls.rs Outdated
Comment thread crates/evm/symbolic/src/runtime/memory.rs Outdated
mablr
mablr previously approved these changes Aug 18, 2026

@mablr mablr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see other blockers, sgtm

Comment thread crates/evm/symbolic/src/executor/opcodes.rs
…-memory

Amp-Thread-ID: https://ampcode.com/threads/T-01a01a8e-87d2-70ec-be7e-fed27bfe3644
Co-authored-by: Amp <amp@ampcode.com>

# Conflicts:
#	crates/evm/symbolic/src/runtime/memory.rs
}
}

pub(super) fn guard_memory_range<FEN: FoundryEvmNetwork>(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guard_memory_range only validates that the access fits the memory limit; it never grows logical_size. Read-only ops that route through it (KECCAK256, LOGn, MCOPY's src) leave memory size stale, so a later MSIZE won't reflect the expansion a real EVM would have performed for these reads.

Ok(true)
}

fn guard_fixed_memory_access<FEN: FoundryEvmNetwork>(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and guard_memory_range below duplicate ~25 lines of near-identical fork/clone/split-corpus-seed/push-to-worklist logic on the same (valid_sat, invalid_sat) match. Worth factoring into a shared helper so a future fix to the split idiom doesn't need to be applied twice.

(false, false) => Ok(Some(StepOutcome::AssumeRejected)),
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike guard_fixed_memory_access's local_size.as_const() fast path, this always issues two constraints_with_condition solver calls, even for fully concrete offset/size that are trivially in-bounds. Once size_word() becomes non-constant (any earlier symbolic-length write in the path), every guarded opcode here pays two SMT queries unconditionally.

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.

4 participants