From c18d29b861c16e0ddbc08c9a4559df49b1a156a2 Mon Sep 17 00:00:00 2001 From: xermicus Date: Tue, 7 Jul 2026 19:38:21 +0200 Subject: [PATCH] Fix/ny unaligned mload native byteorder (#562) Another bug found by @jubnzv: `--newyork`: an unaligned full-word `mload`/`mstore` overlapping a word kept native (little-endian) by the heap optimization returned byte-swapped bytes. --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + .../contracts/UnalignedMloadNativeBug.sol | 24 +++++++++++++++++ crates/integration/src/cases.rs | 12 +++++++++ crates/integration/src/tests.rs | 23 ++++++++++++++++ crates/newyork/src/heap_opt.rs | 27 ++++++++++++++++--- 5 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 crates/integration/contracts/UnalignedMloadNativeBug.sol diff --git a/CHANGELOG.md b/CHANGELOG.md index eb15c75..401bf12 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`: 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/UnalignedMloadNativeBug.sol b/crates/integration/contracts/UnalignedMloadNativeBug.sol new file mode 100644 index 0000000..b564909 --- /dev/null +++ b/crates/integration/contracts/UnalignedMloadNativeBug.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.28; + +/// Soundness PoC (newyork heap optimization): a full-word `mload` at a +/// non-word-aligned offset that overlaps a word the heap analysis classified +/// as native (little-endian, byte-swap elided) reads that word's bytes in the +/// wrong order. +/// +/// `mstore(0x20, PAT)` is word-aligned and does not escape, so newyork keeps +/// it little-endian (native mode, no byte-swap). `mload(0x08)` reads +/// `[0x08, 0x28)`, overlapping the native word at `0x20`, but is itself +/// unaligned so it lowers to a big-endian (byte-swapped) read. The two +/// accesses disagree on byte order for the shared bytes, so the load returns +/// byte-swapped garbage. The fix taints every word an unaligned full-word +/// access covers, forcing those words big-endian for all accesses. +contract UnalignedMload { + function f() external pure returns (bytes32 r) { + assembly { + mstore(0x20, 0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20) + r := mload(0x08) + } + } +} diff --git a/crates/integration/src/cases.rs b/crates/integration/src/cases.rs index 8f684af..74310f7 100644 --- a/crates/integration/src/cases.rs +++ b/crates/integration/src/cases.rs @@ -317,6 +317,18 @@ case!( unaligned_mstore_bug, ); +sol!( + contract UnalignedMload { + function f() external pure returns (bytes32); + } +); +case!( + "UnalignedMloadNativeBug.sol", + UnalignedMload, + fCall, + unaligned_mload_native_bug, +); + sol!( contract ConstReturnOverflowBug { function bug() external pure returns (uint256); diff --git a/crates/integration/src/tests.rs b/crates/integration/src/tests.rs index 000c905..0308885 100644 --- a/crates/integration/src/tests.rs +++ b/crates/integration/src/tests.rs @@ -469,6 +469,29 @@ fn unaligned_mstore_forwarding() { run_differential(actions); } +/// Regression (newyork heap optimization): a full-word `mload` at a +/// non-word-aligned offset that overlaps a word classified native +/// (little-endian, byte-swap elided) returned byte-swapped garbage. In the +/// PoC `mstore(0x20, PAT)` is word-aligned and stays native, but `mload(0x08)` +/// reads `[0x08, 0x28)` — overlapping the native word `0x20` — and lowers to a +/// big-endian read, so the two accesses disagree on byte order for the shared +/// bytes. The heap analysis now taints every word an unaligned full-word +/// access covers, forcing them big-endian so all accesses agree. Compared +/// newyork-PVM vs solc-EVM. +#[test] +fn unaligned_mload_native_byte_order() { + let mut actions = instantiate("contracts/UnalignedMloadNativeBug.sol", "UnalignedMload"); + actions.push(Call { + origin: TestAddress::Alice, + dest: TestAddress::Instantiated(0), + value: 0, + gas_limit: None, + storage_deposit_limit: None, + data: Contract::unaligned_mload_native_bug().calldata, + }); + run_differential(actions); +} + /// Regression: `to_llvm.rs::get_or_create_return_block` (and revert / Stop /// siblings) build the constant offset/length via /// `context.xlen_type().const_int(const_offset, false)`, which silently diff --git a/crates/newyork/src/heap_opt.rs b/crates/newyork/src/heap_opt.rs index 32bd664..6aabb8e 100644 --- a/crates/newyork/src/heap_opt.rs +++ b/crates/newyork/src/heap_opt.rs @@ -268,10 +268,10 @@ impl HeapAnalysis { } else { self.has_dynamic_accesses = true; } - if *region == MemoryRegion::Unknown && !pattern.is_aligned() { + if !pattern.is_aligned() { if let Some(address) = static_offset { if address % BYTE_LENGTH_WORD as u64 != 0 { - self.tainted_regions.insert(word_align(address)); + self.taint_unaligned_access(address); } } } @@ -585,6 +585,21 @@ impl HeapAnalysis { } } + /// Taints every word a static, non-word-aligned full-word access (`mload`/`mstore`) covers. + /// + /// EVM memory is big-endian; the native-mode optimization keeps a word's bytes little-endian + /// (skipping the byte-swap) only when *every* access to that word is word-aligned. An unaligned + /// full-word access at `address` reads or writes the raw bytes of `[address, address + 32)`, + /// spanning the two words `word_align(address)` and `word_align(address) + 32`. If either word + /// were left a native candidate, an aligned native (little-endian) access to it and this + /// unaligned (big-endian) access would disagree on byte order for the overlapping bytes, + /// byte-swapping the result. Tainting both covered words forces them big-endian so all accesses + /// agree. `mstore(0x20, w); r := mload(0x08)` is the canonical trigger: the aligned store is + /// native but the unaligned load reads word `0x20` in the wrong order without this taint. + fn taint_unaligned_access(&mut self, address: u64) { + self.taint_range(Some(address), Some(BYTE_LENGTH_WORD as u64)); + } + /// Taints all word-aligned memory regions in a range. /// If the range is too large, treats it as a dynamic access instead. fn taint_range(&mut self, start: Option, len: Option) { @@ -783,8 +798,12 @@ impl HeapAnalysis { Expression::MLoad { offset, .. } => { let _ = self.classify_access(offset); self.track_variable_access(offset); - if self.extract_static_offset(offset).is_none() { - self.has_dynamic_accesses = true; + match self.extract_static_offset(offset) { + None => self.has_dynamic_accesses = true, + Some(address) if address % BYTE_LENGTH_WORD as u64 != 0 => { + self.taint_unaligned_access(address); + } + Some(_) => {} } } Expression::Keccak256 { offset, length } => {