cerc-io/solidity
16
0
Fork 0
mirror of https://github.com/ethereum/solidity synced 2023-10-03 13:03:40 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Merge pull request #14407 from ethereum/fix-inliner-argument-order-bug

Browse Source 
Fix ``FullInliner`` not preserving evaluation order of call arguments
This commit is contained in:
Kamil Śliwak 2023-07-17 18:32:50 +02:00 committed by GitHub
commit 755110aed4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
16 changed files with 317 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
Changelog.md
View File

@ -1,5 +1,9 @@
### 0.8.21 (unreleased)
Important Bugfixes:
* Yul Optimizer: Fix ``FullInliner`` step (``i``) not preserving the evaluation order of arguments passed into inlined functions in code that is not in expression-split form (i.e. when using a custom optimizer sequence in which the step not preceded by ``ExpressionSplitter`` (``x``)).
Language Features:
* Allow qualified access to events from other contracts.
* Relax restrictions on initialization of immutable variables. Reads and writes may now happen at any point at construction time outside of functions and modifiers. Explicit initialization is no longer mandatory.
@ -26,6 +30,7 @@ Bugfixes:
* SMTChecker: Fix internal error caused by using external identifier to encode member access to functions that take an internal function as a parameter.
* Standard JSON Interface: Fix an incomplete AST being returned when analysis is interrupted by certain kinds of fatal errors.
* Yul Optimizer: Ensure that the assignment of memory slots for variables moved to memory does not depend on AST IDs that may depend on whether additional files are included during compilation.
* Yul Optimizer: Fix ``FullInliner`` step not ignoring code that is not in expression-split form.
* Yul Optimizer: Fix optimized IR being unnecessarily passed through the Yul optimizer again before bytecode generation.
AST Changes:

13
docs/bugs.json
View File

@ -1,4 +1,17 @@
[
{
"uid": "SOL-2023-2",
"name": "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"summary": "Optimizer sequences containing FullInliner do not preserve the evaluation order of arguments of inlined function calls in code that is not in expression-split form.",
"description": "Function call arguments in Yul are evaluated right to left. This order matters when the argument expressions have side-effects, and changing it may change contract behavior. FullInliner is an optimizer step that can replace a function call with the body of that function. The transformation involves assigning argument expressions to temporary variables, which imposes an explicit evaluation order. FullInliner was written with the assumption that this order does not necessarily have to match usual argument evaluation order because the argument expressions have no side-effects. In most circumstances this assumption is true because the default optimization step sequence contains the ExpressionSplitter step. ExpressionSplitter ensures that the code is in *expression-split form*, which means that function calls cannot appear nested inside expressions, and all function call arguments have to be variables. The assumption is, however, not guaranteed to be true in general. Version 0.6.7 introduced a setting allowing users to specify an arbitrary optimization step sequence, making it possible for the FullInliner to actually encounter argument expressions with side-effects, which can result in behavior differences between optimized and unoptimized bytecode. Contracts compiled without optimization or with the default optimization sequence are not affected. To trigger the bug the user has to explicitly choose compiler settings that contain a sequence with FullInliner step not preceded by ExpressionSplitter.",
"link": "https://blog.soliditylang.org/2023/07/19/full-inliner-non-expression-split-argument-evaluation-order-bug/",
"introduced": "0.6.7",
"fixed": "0.8.21",
"severity": "low",
"conditions": {
"yulOptimizer": true
}
},
{
"uid": "SOL-2022-7",
"name": "StorageWriteRemovalBeforeConditionalTermination",

46
docs/bugs_by_version.json
View File

@ -1422,6 +1422,7 @@
},
"0.6.10": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1436,6 +1437,7 @@
},
"0.6.11": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1450,6 +1452,7 @@
},
"0.6.12": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1548,6 +1551,7 @@
},
"0.6.7": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"NestedCalldataArrayAbiReencodingSizeValidation",
@ -1564,6 +1568,7 @@
},
"0.6.8": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"NestedCalldataArrayAbiReencodingSizeValidation",
@ -1577,6 +1582,7 @@
},
"0.6.9": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1592,6 +1598,7 @@
},
"0.7.0": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1606,6 +1613,7 @@
},
"0.7.1": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1621,6 +1629,7 @@
},
"0.7.2": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1635,6 +1644,7 @@
},
"0.7.3": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1648,6 +1658,7 @@
},
"0.7.4": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1660,6 +1671,7 @@
},
"0.7.5": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1672,6 +1684,7 @@
},
"0.7.6": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1684,6 +1697,7 @@
},
"0.8.0": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1696,6 +1710,7 @@
},
"0.8.1": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1708,6 +1723,7 @@
},
"0.8.10": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1717,6 +1733,7 @@
},
"0.8.11": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1727,6 +1744,7 @@
},
"0.8.12": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1737,6 +1755,7 @@
},
"0.8.13": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"StorageWriteRemovalBeforeConditionalTermination",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
@ -1748,6 +1767,7 @@
},
"0.8.14": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"StorageWriteRemovalBeforeConditionalTermination",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
@ -1757,6 +1777,7 @@
},
"0.8.15": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"StorageWriteRemovalBeforeConditionalTermination",
"AbiReencodingHeadOverflowWithStaticArrayCleanup"
],
@ -1764,24 +1785,32 @@
},
"0.8.16": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"StorageWriteRemovalBeforeConditionalTermination"
],
"released": "2022-08-08"
},
"0.8.17": {
"bugs": [],
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder"
],
"released": "2022-09-08"
},
"0.8.18": {
"bugs": [],
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder"
],
"released": "2023-02-01"
},
"0.8.19": {
"bugs": [],
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder"
],
"released": "2023-02-22"
},
"0.8.2": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1793,11 +1822,14 @@
"released": "2021-03-02"
},
"0.8.20": {
"bugs": [],
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder"
],
"released": "2023-05-10"
},
"0.8.3": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1809,6 +1841,7 @@
},
"0.8.4": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1819,6 +1852,7 @@
},
"0.8.5": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1829,6 +1863,7 @@
},
"0.8.6": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1839,6 +1874,7 @@
},
"0.8.7": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1849,6 +1885,7 @@
},
"0.8.8": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",
@ -1860,6 +1897,7 @@
},
"0.8.9": {
"bugs": [
"FullInlinerNonExpressionSplitArgumentEvaluationOrder",
"AbiReencodingHeadOverflowWithStaticArrayCleanup",
"DirtyBytesArrayToStorage",
"DataLocationChangeInInternalOverride",

9
docs/internals/optimizer.rst
View File

@ -570,7 +570,7 @@ It is not applied to loop iteration-condition, because the loop control flow doe
this "outlining" of the inner expressions in all cases. We can sidestep this limitation by applying
:ref:`for-loop-condition-into-body` to move the iteration condition into loop body.
The final program should be in a form such that (with the exception of loop conditions)
The final program should be in an *expression-split form*, where (with the exception of loop conditions)
function calls cannot appear nested inside expressions
and all function call arguments have to be variables.
@ -1192,7 +1192,7 @@ This component can only be used on sources with unique names.
FullInliner
^^^^^^^^^^^
The Full Inliner replaces certain calls of certain functions
The FullInliner replaces certain calls of certain functions
by the function's body. This is not very helpful in most cases, because
it just increases the code size but does not have a benefit. Furthermore,
code is usually very expensive and we would often rather have shorter
@ -1216,6 +1216,11 @@ we can run the optimizer on this specialized function. If it
results in heavy gains, the specialized function is kept,
otherwise the original function is used instead.
FunctionHoister and ExpressionSplitter are recommended as prerequisites since they make the step
more efficient, but are not required for correctness.
In particular, function calls with other function calls as arguments are not inlined, but running
ExpressionSplitter beforehand ensures that there are no such calls in the input.
Cleanup
-------

12
libyul/optimiser/FullInliner.cpp
View File

@ -38,6 +38,8 @@
#include <libsolutil/Visitor.h>
#include <range/v3/action/remove.hpp>
#include <range/v3/view/reverse.hpp>
#include <range/v3/view/zip.hpp>
using namespace std;
using namespace solidity;
@ -187,6 +189,12 @@ bool FullInliner::shallInline(FunctionCall const& _funCall, YulString _callSite)
if (m_noInlineFunctions.count(_funCall.functionName.name) || recursive(*calledFunction))
return false;
// No inlining of calls where argument expressions may have side-effects.
// To avoid running into this, make sure that ExpressionSplitter runs before FullInliner.
for (auto const& argument: _funCall.arguments)
if (!holds_alternative<Literal>(argument) && !holds_alternative<Identifier>(argument))
return false;
// Inline really, really tiny functions
size_t size = m_functionSizes.at(calledFunction->name);
if (size <= 1)
@ -305,8 +313,8 @@ vector<Statement> InlineModifier::performInline(Statement& _statement, FunctionC
newStatements.emplace_back(std::move(varDecl));
};
for (size_t i = 0; i < _funCall.arguments.size(); ++i)
newVariable(function->parameters[i], &_funCall.arguments[i]);
for (auto&& [parameter, argument]: ranges::views::zip(function->parameters, _funCall.arguments) | ranges::views::reverse)
newVariable(parameter, &argument);
for (auto const& var: function->returnVariables)
newVariable(var, nullptr);

6
libyul/optimiser/FullInliner.h
View File

@ -44,10 +44,10 @@ class NameCollector;
* Optimiser component that modifies an AST in place, inlining functions.
* Expressions are expected to be split, i.e. the component will only inline
* function calls that are at the root of the expression and that only contains
* variables as arguments. More specifically, it will inline
* variables or literals as arguments. More specifically, it will inline
* - let x1, ..., xn := f(a1, ..., am)
* - x1, ..., xn := f(a1, ..., am)
* f(a1, ..., am)
* - f(a1, ..., am)
*
* The transform changes code of the form
*
@ -58,8 +58,8 @@ class NameCollector;
*
* function f(a, b) -> c { ... }
*
* let f_a := x
* let f_b := y
* let f_a := x
* let f_c
* code of f, with replacements: a -> f_a, b -> f_b, c -> f_c
* let z := f_c

6
test/libyul/YulOptimizerTestCommon.cpp
View File

@ -168,6 +168,12 @@ YulOptimizerTestCommon::YulOptimizerTestCommon(
FullInliner::run(*m_context, *m_ast);
ExpressionJoiner::run(*m_context, *m_ast);
}},
{"fullInlinerWithoutSplitter", [&]() {
disambiguate();
FunctionHoister::run(*m_context, *m_ast);
FunctionGrouper::run(*m_context, *m_ast);
FullInliner::run(*m_context, *m_ast);
}},
{"mainFunction", [&]() {
disambiguate();
FunctionGrouper::run(*m_context, *m_ast);

29
test/libyul/yulOptimizerTests/fullInliner/call_arguments_with_side_effects.yul Normal file
View File

@ -0,0 +1,29 @@
{
function fun_revert() -> ret { revert(0, 0) }
function fun_return() -> ret { return(0, 0) }
function empty(a, b) {}
// Evaluation order in Yul is right to left so fun_revert() should run first.
empty(fun_return(), fun_revert())
}
// ----
// step: fullInliner
//
// {
// {
// let ret_7 := 0
// revert(0, 0)
// let _1 := ret_7
// let ret_1_10 := 0
// return(0, 0)
// let _2 := ret_1_10
// let b_13 := _1
// let a_14 := _2
// }
// function fun_revert() -> ret
// { revert(0, 0) }
// function fun_return() -> ret_1
// { return(0, 0) }
// function empty(a, b)
// { }
// }

51
test/libyul/yulOptimizerTests/fullInliner/call_arguments_without_side_effects.yul Normal file
View File

@ -0,0 +1,51 @@
{
function empty(a, b, c) {}
// Constants
empty(111, 222, 333)
// Variables
let x := 111
let y := 222
let z := 333
empty(x, y, z)
// Calls
empty(mload(111), sload(222), calldataload(333))
// Mix
let a := 222
empty(111, a, mload(333))
}
// ----
// step: fullInliner
//
// {
// {
// let _1 := 333
// let _2 := 222
// let _3 := 111
// let c_13 := _1
// let b_14 := _2
// let a_15 := _3
// let x := 111
// let y := 222
// let c_16 := 333
// let b_17 := y
// let a_18 := x
// let _5 := calldataload(333)
// let _7 := sload(222)
// let _9 := mload(111)
// let c_19 := _5
// let b_20 := _7
// let a_21 := _9
// let a_1 := 222
// let _11 := mload(333)
// let _12 := 111
// let c_22 := _11
// let b_23 := a_1
// let a_24 := _12
// }
// function empty(a, b, c)
// { }
// }

9
test/libyul/yulOptimizerTests/fullInliner/move_up_rightwards_argument.yul
View File

@ -13,12 +13,13 @@
// let _2 := mload(5)
// let _4 := mload(4)
// let _6 := mload(3)
// let a_13 := mload(2)
// let _8 := mload(2)
// let c_13 := _4
// let b_14 := _6
// let c_15 := _4
// let a_15 := _8
// let x_16 := 0
// x_16 := add(a_13, b_14)
// x_16 := mul(x_16, c_15)
// x_16 := add(a_15, b_14)
// x_16 := mul(x_16, c_13)
// let _10 := add(x_16, _2)
// let y := add(mload(1), _10)
// }

9
test/libyul/yulOptimizerTests/fullInliner/multi_fun.yul
View File

@ -12,13 +12,14 @@
// let a_8 := 3
// let x_9 := 0
// x_9 := add(a_8, a_8)
// let b_10 := x_9
// let c_11 := _1
// let _3 := x_9
// let c_10 := _1
// let b_11 := _3
// let y_12 := 0
// let a_6_13 := b_10
// let a_6_13 := b_11
// let x_7_14 := 0
// x_7_14 := add(a_6_13, a_6_13)
// y_12 := mul(mload(c_11), x_7_14)
// y_12 := mul(mload(c_10), x_7_14)
// let y_1 := y_12
// }
// function f(a) -> x

22
test/libyul/yulOptimizerTests/fullInlinerWithoutSplitter/call_arguments_with_side_effects.yul Normal file
View File

@ -0,0 +1,22 @@
{
function fun_revert() -> ret { revert(0, 0) }
function fun_return() -> ret { return(0, 0) }
function empty(a, b) {}
// Evaluation order in Yul is right to left so fun_revert() should run first.
empty(fun_return(), fun_revert())
}
// ----
// step: fullInlinerWithoutSplitter
//
// {
// {
// empty(fun_return(), fun_revert())
// }
// function fun_revert() -> ret
// { revert(0, 0) }
// function fun_return() -> ret_1
// { return(0, 0) }
// function empty(a, b)
// { }
// }

40
test/libyul/yulOptimizerTests/fullInlinerWithoutSplitter/call_arguments_without_side_effects.yul Normal file
View File

@ -0,0 +1,40 @@
{
function empty(a, b, c) {}
// Constants
empty(111, 222, 333)
// Variables
let x := 111
let y := 222
let z := 333
empty(x, y, z)
// Calls
empty(mload(111), sload(222), calldataload(333))
// Mix
let a := 222
empty(111, a, mload(333))
}
// ----
// step: fullInlinerWithoutSplitter
//
// {
// {
// let c_1 := 333
// let b_2 := 222
// let a_3 := 111
// let x := 111
// let y := 222
// let z := 333
// let c_4 := z
// let b_5 := y
// let a_6 := x
// empty(mload(111), sload(222), calldataload(333))
// let a_1 := 222
// empty(111, a_1, mload(333))
// }
// function empty(a, b, c)
// { }
// }

19
test/libyul/yulOptimizerTests/fullInlinerWithoutSplitter/simple.sol Normal file
View File

@ -0,0 +1,19 @@
{
function f(a) -> x {
x := add(a, a)
}
let y := f(2)
}
// ----
// step: fullInlinerWithoutSplitter
//
// {
// {
// let a_1 := 2
// let x_2 := 0
// x_2 := add(a_1, a_1)
// let y := x_2
// }
// function f(a) -> x
// { x := add(a, a) }
// }

12
test/libyul/yulOptimizerTests/fullSuite/call_arguments_with_side_effects.yul Normal file
View File

@ -0,0 +1,12 @@
{
function fun_revert() -> ret { revert(0, 0) }
function fun_return() -> ret { return(0, 0) }
function empty(a, b) {}
// Evaluation order in Yul is always right to left so optimized code should reach the revert first.
empty(fun_return(), fun_revert())
}
// ----
// step: fullSuite
//
// { { revert(0, 0) } }

48
test/libyul/yulOptimizerTests/fullSuite/call_arguments_without_side_effects.yul Normal file
View File

@ -0,0 +1,48 @@
{
function empty(a, b, c) {
mstore(a, 1)
mstore(b, 1)
mstore(c, 1)
}
// Constants
empty(1, 2, 3)
// Variables
let x := 4
let y := 5
let z := 6
empty(x, y, z)
// Calls
empty(mload(7), sload(8), calldataload(9))
// Mix
let a := 12
empty(11, a, mload(13))
return(0, 32)
}
// ----
// step: fullSuite
//
// {
// {
// let _1 := 1
// mstore(_1, _1)
// mstore(2, _1)
// mstore(3, _1)
// mstore(4, _1)
// mstore(5, _1)
// mstore(6, _1)
// let _2 := sload(8)
// mstore(mload(7), _1)
// mstore(_2, _1)
// mstore(calldataload(9), _1)
// let _3 := mload(13)
// mstore(11, _1)
// mstore(12, _1)
// mstore(_3, _1)
// return(0, 32)
// }
// }