diff --git a/CHANGELOG.md b/CHANGELOG.md index 401bf12..4724867 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 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. ## v1.3.0 diff --git a/crates/integration/contracts/DeadStoreOverlapBug.yul b/crates/integration/contracts/DeadStoreOverlapBug.yul new file mode 100644 index 0000000..c2826a3 --- /dev/null +++ b/crates/integration/contracts/DeadStoreOverlapBug.yul @@ -0,0 +1,23 @@ +/// Soundness PoC (newyork dead-store elimination): a store read back by an +/// intervening *unaligned overlapping* `mload` must not be eliminated as dead. +/// +/// `mem_opt` marked a pending store read only when a later `mload` used the +/// *exact same* static offset. Here `mstore(1, PAT)` writes `[1, 33)`, +/// `mload(8)` reads `[8, 40)` — overlapping `[1, 33)` but at a different offset, +/// so the store stayed a dead-store candidate — and the second `mstore(1, PAT)` +/// eliminated it. `r := mload(8)` then read zeroed memory. EVM (and the stock +/// pipeline) read the stored bytes: bytes 7..31 of PAT followed by 7 zeros. +/// The fix marks every pending store whose 32-byte range the load overlaps as +/// read. +object "DeadStoreOverlapBug" { + code { datacopy(0, dataoffset("DeadStoreOverlapBug_deployed"), datasize("DeadStoreOverlapBug_deployed")) return(0, datasize("DeadStoreOverlapBug_deployed")) } + object "DeadStoreOverlapBug_deployed" { + code { + mstore(1, 0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20) + let r := mload(8) + mstore(1, 0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20) + mstore(0, r) + return(0, 32) + } + } +} diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index 0308885..9c4c161 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): a store read back by an +/// intervening unaligned *overlapping* load must not be eliminated as dead. +/// `mem_opt` marked a pending store read only on an exact-offset load, so +/// `mstore(1, PAT); r := mload(8); mstore(1, PAT)` left the first store a +/// dead-store candidate — `mload(8)` reads `[8, 40)`, overlapping the store's +/// `[1, 33)` at a different offset — and the second store eliminated it, so `r` +/// read zeroed memory. The fix marks every pending store whose 32-byte range a +/// load overlaps as read. Compared newyork-PVM vs solc-EVM. +#[test] +fn dead_store_read_by_overlapping_load() { + let mut actions = instantiate_yul("contracts/DeadStoreOverlapBug.yul", "DeadStoreOverlapBug"); + 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 test for the FMP range-proof gap (PR #7 review finding): a copy /// with a static destination inside the free-memory-pointer word [0x40, 0x60) /// but a DYNAMIC length clobbers 0x40 with arbitrary bytes. `mload(0x40)` must diff --git a/crates/newyork/src/mem_opt.rs b/crates/newyork/src/mem_opt.rs index 523033d..2b8647d 100644 --- a/crates/newyork/src/mem_opt.rs +++ b/crates/newyork/src/mem_opt.rs @@ -592,6 +592,24 @@ impl MemoryOptimizer { } } + /// Marks every pending store that an `mload(load_offset)` reads as read, dropping it from the + /// dead-store candidate set. + /// + /// An `mload(load_offset)` reads the 32 bytes `[load_offset, load_offset + 32)`. A pending store + /// at `p` wrote `[p, p + 32)`; if those ranges overlap, the load observes at least one of the + /// store's bytes, so the store is *not* dead and must not be eliminated by a later overwrite. + /// Removing only the exact-offset entry (`load_offset == p`) missed an *unaligned* overlapping + /// load: `mstore(1, v); r := mload(8); mstore(1, v)` reads `[8, 40)` — overlapping the first + /// store's `[1, 33)` — yet the exact-offset check left it pending, so the second store to `1` + /// eliminated it as dead and `r` read zeroed memory. + fn mark_stores_read_by_load(&mut self, load_offset: u64) { + let load_end = load_offset.saturating_add(BYTE_LENGTH_WORD as u64); + self.pending_stores.retain(|&pending_offset, _| { + let pending_end = pending_offset.saturating_add(BYTE_LENGTH_WORD as u64); + pending_end <= load_offset || pending_offset >= load_end + }); + } + /// Tracks memory reads for dead store elimination and load-after-store forwarding. /// /// When a load follows a store to the same static offset, the stored value is @@ -604,7 +622,7 @@ impl MemoryOptimizer { if let Some(static_offset) = self.try_get_static_offset(&offset) { let word_offset = word_align(static_offset); - self.pending_stores.remove(&static_offset); + self.mark_stores_read_by_load(static_offset); if let Some(tracked) = self.memory_state.get_mut(&word_offset) { if tracked.offset == static_offset { @@ -1455,6 +1473,69 @@ mod tests { } } + /// A store read back by an intervening unaligned *overlapping* load must not + /// be eliminated as dead by a later same-offset store. + /// + /// Builds `mstore(1, v); r := mload(8); mstore(1, v)`: the `mload(8)` reads + /// `[8, 40)`, overlapping the first store's `[1, 33)`, so that store is not dead. + #[test] + fn overlapping_load_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, 1), + literal(2, 0xdead), + Statement::MStore { + offset: int(1), + value: int(2), + region: MemoryRegion::Scratch, + }, + literal(3, 8), + Statement::Let { + bindings: vec![ValueId(4)], + value: Expression::MLoad { + offset: int(3), + region: MemoryRegion::Scratch, + }, + }, + Statement::MStore { + offset: int(1), + value: int(2), + region: MemoryRegion::Scratch, + }, + ]; + + 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 is read by the overlapping mload(8) and must survive" + ); + } + #[test] fn test_constant_propagation_through_add() { use crate::ir::{MemoryRegion, Object};