Fix/ny dse overlapping load (#563)

Another bug found by @jubnzv:

`--newyork`: dead-store elimination could drop a store that a later
overlapping unaligned `mload` reads, returning zeroed memory instead of
the stored bytes.

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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

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)
+    }
+  }
+}

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

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};