From e2e65149538be4642a967c76706e3fe56f858b45 Mon Sep 17 00:00:00 2001 From: xermicus Date: Wed, 8 Jul 2026 16:31:01 +0200 Subject: [PATCH] Fix/ny msize dse expansion (#565) Bug found by @jubnzv: `--newyork`: dead-store elimination could drop a store whose memory expansion an intervening `msize()` observes, so `msize()` read a stale (unexpanded) value. --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + crates/integration/contracts/MsizeDseBug.yul | 24 ++++++++ crates/integration/src/tests.rs | 22 +++++++ crates/newyork/src/mem_opt.rs | 65 ++++++++++++++++++++ 4 files changed, 112 insertions(+) create mode 100644 crates/integration/contracts/MsizeDseBug.yul diff --git a/CHANGELOG.md b/CHANGELOG.md index f36b47c..67c3f54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Supported `polkadot-sdk` rev: `2604.2.0` - Restored the top-level `revive_version` field in `--standard-json` output and the `resolc_version` field in `--combined-json` output, both of which stopped being populated in `v0.4.0`. [#557](https://github.com/paritytech/revive/pull/557) - ICE triggered by `--disable-solc-optimizer --newyork where inlined loops could emit an unterminated basic block in the inlined entrypoint. [#561](https://github.com/paritytech/revive/pull/561) +- `--newyork`: dead-store elimination could drop a store whose memory expansion an intervening `msize()` observes, so `msize()` read a stale (unexpanded) value. [#565](https://github.com/paritytech/revive/pull/565) - `--newyork`: dead-store elimination could drop a store that a later overlapping unaligned `mload` reads, returning zeroed memory instead of the stored bytes. - `--newyork`: an unaligned full-word `mload`/`mstore` overlapping a word kept native (little-endian) by the heap optimization returned byte-swapped bytes. - `--newyork`: a `calldatacopy`/`codecopy`/`returndatacopy` to a dynamic destination that could overwrite the free-memory-pointer word truncated a subsequent `mload(0x40)` to `0`. [#564](https://github.com/paritytech/revive/pull/564) diff --git a/crates/integration/contracts/MsizeDseBug.yul b/crates/integration/contracts/MsizeDseBug.yul new file mode 100644 index 0000000..78eb292 --- /dev/null +++ b/crates/integration/contracts/MsizeDseBug.yul @@ -0,0 +1,24 @@ +/// Soundness PoC (newyork dead-store elimination vs `msize`): a store whose +/// memory expansion an intervening `msize()` observes must not be eliminated as +/// dead by a later overwrite. +/// +/// `mstore(288, 1)` writes `[288, 320)`, expanding active memory to `0x140`. +/// `msize()` between the two stores must observe that expansion (`0x140`). The +/// dead-store pass dropped the first store because `mstore(288, 2)` overwrites +/// the same offset — but that moved the expansion to *after* the `msize()`, so +/// `msize()` read the smaller (unexpanded) value `0`. EVM (and the stock +/// pipeline) return `0x140`. The fix clears the dead-store candidate set when an +/// `msize()` is evaluated, since it observes the cumulative memory expansion of +/// all prior stores. +object "MsizeDseBug" { + code { datacopy(0, dataoffset("MsizeDseBug_deployed"), datasize("MsizeDseBug_deployed")) return(0, datasize("MsizeDseBug_deployed")) } + object "MsizeDseBug_deployed" { + code { + mstore(288, 1) + let r := msize() + mstore(288, 2) + mstore(0, r) + return(0, 32) + } + } +} diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index be95e06..ec668cb 100644 --- a/crates/integration/src/tests.rs +++ b/crates/integration/src/tests.rs @@ -4769,6 +4769,28 @@ fn array_bounds_probe() { } } +/// Regression (newyork dead-store elimination vs `msize`): a store whose memory +/// expansion an intervening `msize()` observes must not be eliminated as dead. +/// `mstore(288, 1); let r := msize(); mstore(288, 2)` — the dead-store pass +/// dropped the first store as overwritten by the second, moving the memory +/// expansion to after the `msize()`, so `r` read the smaller value `0` instead of +/// `0x140`. The fix clears the dead-store candidate set at an `msize()`, which +/// observes the cumulative expansion of all prior stores. Compared newyork-PVM +/// vs solc-EVM. +#[test] +fn msize_observes_dead_store_expansion() { + let mut actions = instantiate_yul("contracts/MsizeDseBug.yul", "MsizeDseBug"); + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value: 0, + gas_limit: Some(GAS_LIMIT), + storage_deposit_limit: None, + data: vec![], + }); + run_differential(actions); +} + /// Regression (newyork FMP range proof): a `calldatacopy` whose *dynamic* /// destination can land on the free-memory-pointer word `[0x40, 0x60)` corrupts /// the FMP, but only *static* copy destinations flagged `fmp_could_be_unbounded`. diff --git a/crates/newyork/src/mem_opt.rs b/crates/newyork/src/mem_opt.rs index 2b8647d..782051f 100644 --- a/crates/newyork/src/mem_opt.rs +++ b/crates/newyork/src/mem_opt.rs @@ -667,6 +667,11 @@ impl MemoryOptimizer { Expression::MappingSLoad { key, slot } } + Expression::MSize => { + self.pending_stores.clear(); + Expression::MSize + } + other => other, } } @@ -1473,6 +1478,66 @@ mod tests { } } + /// A store whose memory expansion an intervening `msize()` observes must not + /// be eliminated as dead by a later store to the same offset. + /// + /// Builds `mstore(288, 1); let r := msize(); mstore(288, 2)`: the `msize()` + /// observes the first store's expansion, so that store is not dead. + #[test] + fn msize_prevents_dead_store_elimination() { + use crate::ir::{MemoryRegion, Object}; + + fn literal(id: u32, value: u64) -> Statement { + Statement::Let { + bindings: vec![ValueId(id)], + value: Expression::Literal { + value: BigUint::from(value), + value_type: Type::Int(BitWidth::I256), + }, + } + } + fn int(id: u32) -> Value { + Value { + id: ValueId(id), + value_type: Type::Int(BitWidth::I256), + } + } + + let statements = vec![ + literal(1, 288), + literal(2, 1), + Statement::MStore { + offset: int(1), + value: int(2), + region: MemoryRegion::Unknown, + }, + Statement::Let { + bindings: vec![ValueId(3)], + value: Expression::MSize, + }, + literal(4, 2), + Statement::MStore { + offset: int(1), + value: int(4), + region: MemoryRegion::Unknown, + }, + ]; + + let mut object = Object { + name: "test".to_string(), + code: Block { statements }, + functions: BTreeMap::new(), + subobjects: vec![], + data: BTreeMap::new(), + }; + + let statistics = MemoryOptimizer::new().optimize_object(&mut object); + assert_eq!( + statistics.stores_eliminated, 0, + "the first store's expansion is observed by the intervening msize()" + ); + } + /// A store read back by an intervening unaligned *overlapping* load must not /// be eliminated as dead by a later same-offset store. ///