mirror of
https://github.com/pezkuwichain/revive.git
synced 2026-10-20 18:56:17 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
1 parent
9adfd9db24
commit
e2e6514953
4 files changed
+112
No files matched your search
@@ -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)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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`.
|
||||
|
||||
@@ -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.
|
||||
///
|
||||
|
||||
Reference in new issue
Block a user