Fix integer overflow in heap_opt range walk on top-of-memory access (… by kvpanch · Pull Request #582 · paritytech/revive

kvpanch · 2026-07-28T14:06:04Z

The word-stepping loops in taint_range, mark_escaping_range, and mark_escaping_and_tainted_range walked a static range with while word < end { word += 32 }. When an access sits near the top of the address space, end = address.saturating_add(size) saturates at u64::MAX, and the final word += 32 steps past u64::MAX, panicking with "attempt to add with overflow".

Under --newyork this is reachable from valid Yul: folding add(mul(MAX_U256, 1), 0x41) to 0x40 lets an mstore corrupt the free-memory-pointer word with u64::MAX; mem_opt then forwards mload(0x40) to that literal, turning a later mstore(fmp, _) into a static unaligned store at u64::MAX that taints its covered words.

Iterate the already-bounded num_words count with a saturating step so the walk covers the same words without overflowing.

…581) The word-stepping loops in `taint_range`, `mark_escaping_range`, and `mark_escaping_and_tainted_range` walked a static range with `while word < end { word += 32 }`. When an access sits near the top of the address space, `end = address.saturating_add(size)` saturates at `u64::MAX`, and the final `word += 32` steps past `u64::MAX`, panicking with "attempt to add with overflow". Under `--newyork` this is reachable from valid Yul: folding `add(mul(MAX_U256, 1), 0x41)` to `0x40` lets an mstore corrupt the free-memory-pointer word with `u64::MAX`; mem_opt then forwards `mload(0x40)` to that literal, turning a later `mstore(fmp, _)` into a static unaligned store at `u64::MAX` that taints its covered words. Iterate the already-bounded `num_words` count with a saturating step so the walk covers the same words without overflowing.

xermicus

Could you please add the MRE from the bug report as integration test?

xermicus · 2026-08-05T06:30:16Z

                let range = end.saturating_sub(first_word);
                let num_words =
                    range.saturating_add(BYTE_LENGTH_WORD as u64 - 1) / BYTE_LENGTH_WORD as u64;
+                // Records each covered word; same range walk and corner cases as `taint_range`.


Please remove this and make a doc-comment instead

xermicus · 2026-08-05T06:30:49Z

                    .saturating_sub(first_word)
                    .saturating_add(BYTE_LENGTH_WORD as u64 - 1)
                    / BYTE_LENGTH_WORD as u64;
+                // `[address, address + size)` spans `num_words` 32-byte words starting at


Same here, please remove and make a proper doc comment instead.

elle-j

Could you add a changelog entry as well, referencing this PR? 👍

elle-j · 2026-08-11T09:46:22Z

                        self.escaping_regions.insert(word);
                        self.tainted_regions.insert(word);
-                        word += BYTE_LENGTH_WORD as u64;
+                        word = word.saturating_add(BYTE_LENGTH_WORD as u64);


From seeing how similar the updated sections are, we could consider adding e.g. a fn range_words() helper to extract the identical logic (let end = .., let first_word = .., let num_words = .., if num_words > MAX_RANGE_WORDS { .., etc.), having it return the range of words (an iterator over the words).

Other than easier maintenance and the stepping being handled in one place, I think the small differences at each call site would also be a bit clearer then (e.g. how they all handle if num_words > MAX_RANGE_WORDS being true or false a bit differently. The helper could return None when the range is wider than the max for instance). Let me know what you think.

elle-j · 2026-08-11T09:51:45Z

                let range = end.saturating_sub(first_word);
                let num_words =
                    range.saturating_add(BYTE_LENGTH_WORD as u64 - 1) / BYTE_LENGTH_WORD as u64;
+                // Records each covered word; same range walk and corner cases as `taint_range`.


If by the "corner cases" you mean e.g. the size == 0 case then at all locations they do handle this somewhat differently, so I'd drop the "same" wording here.

elle-j · 2026-08-11T09:56:32Z

+                //   unaligned store through a corrupted free-memory pointer). Stepping the
+                //   bounded `num_words` count with a saturating add keeps the final increment
+                //   from overflowing `u64` — a plain `word += 32` panics here.
+                // - `size == 0` is excluded by the match guard (an empty range taints nothing).


This last comment line needs an update. The (Some(address), _) arm adds a tainted region for size == 0.

elle-j · 2026-08-11T10:02:39Z

+    /// `taint_range` taints exactly the word-aligned words a static range covers,
+    /// including the partial leading word when the start is unaligned.
+    #[test]
+    fn taint_range_covers_spanned_words() {


Currently only the taint_range() is tested. Could consider adding similar ones for the other updates.

elle-j

Nice, thanks!

elle-j · 2026-08-11T15:07:29Z

+/// must not overflow the word walk. `add(mul(MAX_U256, 1), 0x41)` folds to `0x40`, corrupting the
+/// free-memory-pointer word with `u64::MAX`; `mem_opt` then forwards a store to that literal and
+/// the walk panicked with "attempt to add with overflow". Compiling is the regression; the call
+/// pins runtime behaviour against solc-EVM.


Nit:

Suggested change

/// pins runtime behaviour against solc-EVM.

/// pins runtime behavior against solc-EVM.

elle-j · 2026-08-11T15:11:49Z

+
+    /// An access at the top of the address space must not overflow the word walk.
+    #[test]
+    fn taint_range_top_of_memory_does_not_overflow() {


We could add an assertion on the analysis.has_dynamic_access in this test as well.

elle-j · 2026-08-11T15:12:42Z

+
+    /// The overflowing range reaches this caller through the same helper.
+    #[test]
+    fn mark_escaping_range_top_of_memory_does_not_overflow() {


We could add an assertion on the analysis.has_dynamic_escapes in this test as well.

elle-j · 2026-08-11T15:13:00Z

+
+    /// The overflowing range through the both-sets walk.
+    #[test]
+    fn mark_escaping_and_tainted_range_top_of_memory_does_not_overflow() {


We could add an assertion on the analysis.has_dynamic_escapes in this test as well.

elle-j · 2026-08-11T15:15:37Z

+## Unreleased
+


Suggested change

## Unreleased

## Unreleased

Supported `polkadot-sdk` rev: `2604.2.0`

kvpanch requested review from elle-j and xermicus July 28, 2026 14:06

xermicus reviewed Aug 5, 2026

View reviewed changes

elle-j reviewed Aug 11, 2026

View reviewed changes

kvpanch added 2 commits August 11, 2026 10:06

addressed comments

d67c5ba

Merge remote-tracking branch 'origin/main' into gh_581

1dabf41

elle-j approved these changes Aug 11, 2026

View reviewed changes

	/// pins runtime behaviour against solc-EVM.
	/// pins runtime behavior against solc-EVM.

Uh oh!

Conversation

kvpanch commented Jul 28, 2026

Uh oh!

xermicus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elle-j left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elle-j left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants