Add blog post describing the returndatacopy bug and fix.#198
Add blog post describing the returndatacopy bug and fix.#198
returndatacopy bug and fix.#198Conversation
✅ Deploy Preview for solidity-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
returndatacopy bug and fix.
|
|
||
| ## Summary | ||
|
|
||
| The Yul optimizer's `UnusedStoreEliminator` pass could incorrectly remove `returndatacopy(...)` operations when the length argument was supplied from a `returndatasize()` value that had become stale due to an intervening call opcode. Because `returndatacopy` reverts on out-of-bounds access, removing it in this case silently suppresses a revert that should have occurred, allowing execution to continue past a point where it should have stopped. |
There was a problem hiding this comment.
| The Yul optimizer's `UnusedStoreEliminator` pass could incorrectly remove `returndatacopy(...)` operations when the length argument was supplied from a `returndatasize()` value that had become stale due to an intervening call opcode. Because `returndatacopy` reverts on out-of-bounds access, removing it in this case silently suppresses a revert that should have occurred, allowing execution to continue past a point where it should have stopped. | |
| The Yul optimizer's `UnusedStoreEliminator` step could incorrectly remove `returndatacopy()` operations when the length argument was supplied from a `returndatasize()` value that had become stale due to an intervening call opcode. Because `returndatacopy` reverts on out-of-bounds access, removing it in this case silently suppresses a revert that should have occurred, allowing execution to continue past a point where it should have stopped. |
There was a problem hiding this comment.
By the way, I think it's a good convention to have a line break after every sentence. Makes diffs clearer and smaller, but also does not require an annoying rewrap after each change like a fixed line width does.
|
|
||
| The Yul optimizer's `UnusedStoreEliminator` pass could incorrectly remove `returndatacopy(...)` operations when the length argument was supplied from a `returndatasize()` value that had become stale due to an intervening call opcode. Because `returndatacopy` reverts on out-of-bounds access, removing it in this case silently suppresses a revert that should have occurred, allowing execution to continue past a point where it should have stopped. | ||
|
|
||
| The bug only affects **handwritten Yul or inline assembly** code. High-level Solidity code was never affected. The severity is rated **very low**. |
There was a problem hiding this comment.
| The bug only affects **handwritten Yul or inline assembly** code. High-level Solidity code was never affected. The severity is rated **very low**. | |
| The bug only affects **handwritten Yul or inline assembly** code. High-level Solidity code never produces the affected pattern. The severity is rated **very low**. |
|
|
||
| ## Technical Background | ||
|
|
||
| The `UnusedStoreEliminator` is an optimizer step in the Yul pipeline that removes write operations (to memory or storage) whose results are provably never read. Eligible operations include `mstore`, `mstore8`, `codecopy`, `calldatacopy`, `extcodecopy`, and — since version 0.8.13 — `returndatacopy`. |
There was a problem hiding this comment.
| The `UnusedStoreEliminator` is an optimizer step in the Yul pipeline that removes write operations (to memory or storage) whose results are provably never read. Eligible operations include `mstore`, `mstore8`, `codecopy`, `calldatacopy`, `extcodecopy`, and — since version 0.8.13 — `returndatacopy`. | |
| The `UnusedStoreEliminator` is an optimizer step in the Yul pipeline that removes write operations (to memory or storage) whose results are provably never read. Eligible memory operations include `mstore`, `mstore8`, `codecopy`, `calldatacopy`, `extcodecopy`, and — since version 0.8.13 — `returndatacopy`. |
Otherwise the list should include sstore as well.
|
|
||
| The `UnusedStoreEliminator` is an optimizer step in the Yul pipeline that removes write operations (to memory or storage) whose results are provably never read. Eligible operations include `mstore`, `mstore8`, `codecopy`, `calldatacopy`, `extcodecopy`, and — since version 0.8.13 — `returndatacopy`. | ||
|
|
||
| The `returndatacopy` instruction is special among memory-copying opcodes: unlike `calldatacopy` or `codecopy`, it **reverts** when accessing bytes beyond the bounds of the return data buffer. Since reverts are a side effect the optimizer must preserve, a `returndatacopy` can only be removed when it is certain that it will never access out-of-bounds memory. |
There was a problem hiding this comment.
| The `returndatacopy` instruction is special among memory-copying opcodes: unlike `calldatacopy` or `codecopy`, it **reverts** when accessing bytes beyond the bounds of the return data buffer. Since reverts are a side effect the optimizer must preserve, a `returndatacopy` can only be removed when it is certain that it will never access out-of-bounds memory. | |
| The `returndatacopy` instruction has an odd quirk that other memory-copying opcodes don't have: unlike `calldatacopy` or `codecopy`, it **reverts** when accessing bytes beyond the bounds of the input data buffer. Since reverts are a side effect the optimizer must preserve, a `returndatacopy` can only be removed when it is certain that it will never access out-of-bounds memory. |
Saying that it's "special" makes it sound like this behavior is justified. This should be worded more strongly. In fact, this is a good opportunity to emphasize how annoying this behavior is for compilers (and that EOF would have fixed it).
|
|
||
| The `returndatacopy` instruction is special among memory-copying opcodes: unlike `calldatacopy` or `codecopy`, it **reverts** when accessing bytes beyond the bounds of the return data buffer. Since reverts are a side effect the optimizer must preserve, a `returndatacopy` can only be removed when it is certain that it will never access out-of-bounds memory. | ||
|
|
||
| The only pattern where this can be guaranteed statically is `returndatacopy(0, 0, returndatasize())` — copying the entire return data buffer from the beginning. This is also the exact pattern emitted by the Solidity code generator for handling return data from external calls. Starting in 0.8.13, the optimizer was special-cased to recognize and remove only this pattern. |
There was a problem hiding this comment.
It's not the only one. Just the only one we cared about due to the fact that it's emitted by the codegen.
| The only pattern where this can be guaranteed statically is `returndatacopy(0, 0, returndatasize())` — copying the entire return data buffer from the beginning. This is also the exact pattern emitted by the Solidity code generator for handling return data from external calls. Starting in 0.8.13, the optimizer was special-cased to recognize and remove only this pattern. | |
| One of the patterns where this can be guaranteed statically is `returndatacopy(0, 0, returndatasize())` — copying the entire return data buffer from the beginning. This is also the exact pattern emitted by the Solidity code generator for handling return data from external calls. Starting in 0.8.13, the optimizer was special-cased to recognize and remove only this pattern. |
| - The affected code pattern must appear in **inline assembly or handwritten Yul**. The Solidity code generator itself never produces code interleaving multiple external calls with access to a previous call's return data, so high-level Solidity code is not affected. | ||
| - The memory region written by `returndatacopy` must **not be read afterward**. The `UnusedStoreEliminator` only removes a write operation when it can prove the written memory is never subsequently read. If any later instruction reads from the same memory region, the store is kept regardless of the pattern match. | ||
|
|
||
| **Effect:** A `returndatacopy` that would have reverted due to an out-of-bounds access is silently removed, causing execution to continue when it should have stopped. The memory region targeted by the copy is never written; the program proceeds with stale memory contents and no revert occurs. The practical impact depends on how that memory is used afterward, but given the specific conditions required to trigger the bug, real-world impact is expected to be minimal. |
There was a problem hiding this comment.
This focuses a bit too much on the mechanics of what happens when the bug is triggered.
That bit is obvious - revert does not happen so code that should not be executed is executed. Not much more needs to be said.
On the other hand it does not get deep enough into the practical impact, i.e. how likely this is to impact anyone in the first place. The important thing to mention is that we don't think anyone is likely to intentionally write assembly using returndatacopy() with a stale returndatasize() value. We'd not expect other code generators that emit Yul to produce such code either. It's more likely to be a mistake than something intentional. This severely limits the real-world impact.
Then, even if someone did that, they may still not even be aware that the instruction is supposed to revert on out-of-bounds access (that's not a usual property of other similar instructions), so removing the revert may not be harmful.
And even then, the removal of the revert is the only effect. The bug would not make the compiler remove returndatacopy() whose result is used, so the bug would never result in the wrong value being returned by the call.
There was a problem hiding this comment.
Damn, having written this like this makes me question if this even deserves very low severity. Back when we decided it's a security issue we were assuming that the optimization is actually effective sometimes. But as we go deeper into it it's starting to look that this is less and less likely to affect anyone.
| ## Workaround | ||
|
|
||
| If you are using a version between 0.8.13 and 0.8.34 and your code uses `returndatacopy` in inline assembly or Yul following an external call, avoid storing the result of `returndatasize()` in a variable before the call and using it as the length argument afterward. Instead, call `returndatasize()` fresh at the point of `returndatacopy`: |
There was a problem hiding this comment.
I'm not sure it makes sense to include such a workaround. Like, if your returndatasize() is not stale, you're safe. And if it's not, the workaround will break it, but why are you trying to use a stale returndatasize() in the first place?
| ## Timeline | ||
|
|
||
| | Date | Event | | ||
| |------|-------| | ||
| | 2022-05-17 | Optimization first introduced in 0.8.13 (partial fix for out-of-bounds revert preservation) | | ||
| | 2026-01-17 | Bug reported by Carl Andersen from Spearbit | | ||
| | 2026-04-17 | Fix committed in PR #16508 | | ||
| | 2026-XX-XX | Fixed in Solidity 0.8.35 | |
There was a problem hiding this comment.
We don't normally include the timeline in such posts. And I don't think it's that important.
|
|
||
| ## The Fix | ||
|
|
||
| The fix, implemented in [PR #16508](https://github.com/argotorg/solidity/pull/16508), takes the conservative approach of **removing the `returndatacopy` optimization entirely** from the `UnusedStoreEliminator`. The `RETURNDATACOPY` instruction is now unconditionally blacklisted from removal alongside `MCOPY`. |
There was a problem hiding this comment.
This needs some detail on why we thought removing the optimization is a good idea.
| layout: post | ||
| published: true | ||
| title: 'UnusedStoreEliminator Stale Return Data Size Bug' | ||
| date: '2026-04-21' |
There was a problem hiding this comment.
Just a heads up that date needs to be updated.
Add blog post to describing
returndatacopyincorrect removing bug and the fix implemented in the PR