Merge pull request #14366 from ethereum/selectorSideEffects

Always generate code for .selector member access.
2023-10-03 13:03:40 +00:00 · 2023-07-18 19:14:30 +02:00 · 2023-07-18 19:14:30 +02:00 · 2a46e2fedc
commit 2a46e2fedc
parent 64427412c4 86602852be
6 changed files with 86 additions and 4 deletions
--- a/Changelog.md
+++ b/Changelog.md
@ -1,6 +1,7 @@
 ### 0.8.21 (unreleased)

 Important Bugfixes:
+ * Code Generator: Always generate code for the expression in ``<expression>.selector`` in the legacy code generation pipeline.
 * 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``)).


@ -8,6 +9,7 @@ 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.

+
 Compiler Features:
 * Commandline Interface: Add ``--ast-compact-json`` output in assembler mode.
 * Commandline Interface: Add ``--ir-ast-json`` and ``--ir-optimized-ast-json`` outputs for Solidity input, providing AST in compact JSON format for IR and optimized IR.
@ -36,6 +38,7 @@ Bugfixes:
 * 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:
 * AST: Add the ``experimentalSolidity`` field to the ``SourceUnit`` nodes, which indicate whether the experimental parsing mode has been enabled via ``pragma experimental solidity``.

--- a/docs/bugs.json
+++ b/docs/bugs.json
@ -12,6 +12,19 @@
            "yulOptimizer": true
        }
    },
+    {
+        "uid": "SOL-2023-1",
+        "name": "MissingSideEffectsOnSelectorAccess",
+        "summary": "Accessing the ``.selector`` member on complex expressions leaves the expression unevaluated in the legacy code generation.",
+        "description": "When accessing the ``.selector`` member on an expression with side-effects, like an assignment, a function call or a conditional, the expression would not be evaluated in the legacy code generation. This would happen in expressions where the functions used in the expression were all known at compilation time, regardless of whether the whole expression could be evaluated at compilation time or not. Note that the code generated by the IR pipeline was unaffected and would behave as expected.",
+        "link": "https://blog.soliditylang.org/2023/07/19/missing-side-effects-on-selector-access-bug/",
+        "introduced": "0.6.2",
+        "fixed": "0.8.21",
+        "severity": "low",
+        "conditions": {
+            "viaIR": false
+        }
+    },
    {
        "uid": "SOL-2022-7",
        "name": "StorageWriteRemovalBeforeConditionalTermination",
--- a/docs/bugs_by_version.json
+++ b/docs/bugs_by_version.json
@ -1423,6 +1423,7 @@
    "0.6.10": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1438,6 +1439,7 @@
    "0.6.11": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1453,6 +1455,7 @@
    "0.6.12": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1467,6 +1470,7 @@
    },
    "0.6.2": {
        "bugs": [
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1484,6 +1488,7 @@
    },
    "0.6.3": {
        "bugs": [
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1501,6 +1506,7 @@
    },
    "0.6.4": {
        "bugs": [
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1518,6 +1524,7 @@
    },
    "0.6.5": {
        "bugs": [
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1535,6 +1542,7 @@
    },
    "0.6.6": {
        "bugs": [
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1552,6 +1560,7 @@
    "0.6.7": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1569,6 +1578,7 @@
    "0.6.8": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "NestedCalldataArrayAbiReencodingSizeValidation",
@ -1583,6 +1593,7 @@
    "0.6.9": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1599,6 +1610,7 @@
    "0.7.0": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1614,6 +1626,7 @@
    "0.7.1": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1630,6 +1643,7 @@
    "0.7.2": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1645,6 +1659,7 @@
    "0.7.3": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1659,6 +1674,7 @@
    "0.7.4": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1672,6 +1688,7 @@
    "0.7.5": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1685,6 +1702,7 @@
    "0.7.6": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1698,6 +1716,7 @@
    "0.8.0": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1711,6 +1730,7 @@
    "0.8.1": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1724,6 +1744,7 @@
    "0.8.10": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1734,6 +1755,7 @@
    "0.8.11": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1745,6 +1767,7 @@
    "0.8.12": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1756,6 +1779,7 @@
    "0.8.13": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "StorageWriteRemovalBeforeConditionalTermination",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
@ -1768,6 +1792,7 @@
    "0.8.14": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "StorageWriteRemovalBeforeConditionalTermination",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
@ -1778,6 +1803,7 @@
    "0.8.15": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "StorageWriteRemovalBeforeConditionalTermination",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup"
        ],
@ -1786,31 +1812,36 @@
    "0.8.16": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "StorageWriteRemovalBeforeConditionalTermination"
        ],
        "released": "2022-08-08"
    },
    "0.8.17": {
        "bugs": [
-            "FullInlinerNonExpressionSplitArgumentEvaluationOrder"
+            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess"
        ],
        "released": "2022-09-08"
    },
    "0.8.18": {
        "bugs": [
-            "FullInlinerNonExpressionSplitArgumentEvaluationOrder"
+            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess"
        ],
        "released": "2023-02-01"
    },
    "0.8.19": {
        "bugs": [
-            "FullInlinerNonExpressionSplitArgumentEvaluationOrder"
+            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess"
        ],
        "released": "2023-02-22"
    },
    "0.8.2": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1823,13 +1854,15 @@
    },
    "0.8.20": {
        "bugs": [
-            "FullInlinerNonExpressionSplitArgumentEvaluationOrder"
+            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess"
        ],
        "released": "2023-05-10"
    },
    "0.8.3": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1842,6 +1875,7 @@
    "0.8.4": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1853,6 +1887,7 @@
    "0.8.5": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1864,6 +1899,7 @@
    "0.8.6": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1875,6 +1911,7 @@
    "0.8.7": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1886,6 +1923,7 @@
    "0.8.8": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
@ -1898,6 +1936,7 @@
    "0.8.9": {
        "bugs": [
            "FullInlinerNonExpressionSplitArgumentEvaluationOrder",
+            "MissingSideEffectsOnSelectorAccess",
            "AbiReencodingHeadOverflowWithStaticArrayCleanup",
            "DirtyBytesArrayToStorage",
            "DataLocationChangeInInternalOverride",
--- a/libsolidity/codegen/ExpressionCompiler.cpp
+++ b/libsolidity/codegen/ExpressionCompiler.cpp
@ -1659,6 +1659,10 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess)
 	{
 		if (functionType->hasDeclaration())
 		{
+			// Still visit the expression in case it has side effects.
+			_memberAccess.expression().accept(*this);
+			utils().popStackElement(*functionType);
+
 			if (functionType->kind() == FunctionType::Kind::Event)
 				m_context << u256(h256::Arith(util::keccak256(functionType->externalSignature())));
 			else
--- a/test/libsolidity/semanticTests/functionTypes/selector_assignment_expression.sol
+++ b/test/libsolidity/semanticTests/functionTypes/selector_assignment_expression.sol
@ -0,0 +1,10 @@
+contract C {
+    bool public z;
+    function f() public {
+        ((z = true) ? this.f : this.f).selector;
+    }
+}
+
+// ----
+// f()
+// z() -> true
--- a/test/libsolidity/semanticTests/functionTypes/selector_expression_side_effect.sol
+++ b/test/libsolidity/semanticTests/functionTypes/selector_expression_side_effect.sol
@ -0,0 +1,13 @@
+contract C {
+	uint x;
+	function f() public returns (uint256) {
+		h().f.selector;
+		return x;
+	}
+	function h() public returns (C) {
+		x = 42;
+		return this;
+	}
+}
+// ----
+// f() -> 42