Merge pull request #2345 from iamdefinitelyahuman/fix-parse-sequence

Fix: memory corruption from nested internal function calls

Author: fubuloubu

tests/parser/types/test_lists.py

@@ -286,3 +286,79 @@ def test_values(arr: int128[2][3], s: String[10]) -> (int128[2][3], String[10]):
 
     c = get_contract(code)
     assert c.test_values([[1, 2], [3, 4], [5, 6]], "abcdef") == [[[1, 2], [3, 4], [5, 6]], "abcdef"]
+
+
+def test_nested_index_of_returned_array(get_contract):
+    code = """
+@internal
+def inner() -> (int128, int128):
+    return 1,2
+
+@external
+def outer() -> int128[2]:
+    return [333, self.inner()[0]]
+    """
+
+    c = get_contract(code)
+    assert c.outer() == [333, 1]
+
+
+def test_nested_calls_inside_arrays(get_contract):
+    code = """
+@internal
+def _foo(a: uint256, b: uint256[2]) -> (uint256, uint256, uint256, uint256, uint256):
+    return 1, a, b[0], b[1], 5
+
+@internal
+def _foo2() -> uint256:
+    a: uint256[10] = [6,7,8,9,10,11,12,13,15,16]
+    return 4
+
+@external
+def foo() -> (uint256, uint256, uint256, uint256, uint256):
+    return self._foo(2, [3, self._foo2()])
+    """
+
+    c = get_contract(code)
+    assert c.foo() == [1, 2, 3, 4, 5]
+
+
+def test_nested_calls_inside_arrays_with_index_access(get_contract):
+    code = """
+@internal
+def _foo(a: uint256[2], b: uint256[2]) -> (uint256, uint256, uint256, uint256, uint256):
+    return a[1]-b[0], 2, a[0]-b[1], 8-b[1], 5
+
+@internal
+def _foo2() -> (uint256, uint256):
+    a: uint256[10] = [6,7,8,9,10,11,12,13,15,16]
+    return a[6], 4
+
+@external
+def foo() -> (uint256, uint256, uint256, uint256, uint256):
+    return self._foo([7, self._foo2()[0]], [11, self._foo2()[1]])
+    """
+
+    c = get_contract(code)
+    assert c.foo() == [1, 2, 3, 4, 5]
+
+
+def test_so_many_things_you_should_never_do(get_contract):
+    code = """
+@internal
+def _foo(a: uint256[2], b: uint256[2]) -> uint256[5]:
+    return [a[1]-b[0], 2, a[0]-b[1], 8-b[1], 5]
+
+@internal
+def _foo2() -> (uint256, uint256):
+    b: uint256[2] = [5, 8]
+    a: uint256[10] = [6,7,8,9,10,11,12,13,self._foo([44,b[0]],b)[4],16]
+    return a[6], 4
+
+@external
+def foo() -> (uint256, uint256[3], uint256[2]):
+    x: uint256[3] = [1, 14-self._foo2()[0], self._foo([7,self._foo2()[0]], [11,self._foo2()[1]])[2]]
+    return 666, x, [88, self._foo2()[0]]
+    """
+    c = get_contract(code)
+    assert c.foo() == [666, [1, 2, 3], [88, 12]]

vyper/parser/expr.py

@@ -949,28 +949,25 @@ def parse_Call(self):
             return external_call.make_external_call(self.expr, self.context)
 
     def parse_List(self):
-        if not len(self.expr.elements):
-            return
-
-        def get_out_type(lll_node):
-            if isinstance(lll_node, ListType):
-                return get_out_type(lll_node.subtype)
-            return lll_node.typ
+        call_lll, multi_lll = parse_sequence(self.expr, self.expr.elements, self.context)
+        out_type = next((i.typ for i in multi_lll if not i.typ.is_literal), multi_lll[0].typ)
+        typ = ListType(out_type, len(self.expr.elements), is_literal=True)
+        multi_lll = LLLnode.from_list(["multi"] + multi_lll, typ=typ, pos=getpos(self.expr))
+        if not call_lll:
+            return multi_lll
 
-        lll_node = []
-        out_type = None
+        lll_node = ["seq_unchecked"] + call_lll + [multi_lll]
+        return LLLnode.from_list(lll_node, typ=typ, pos=getpos(self.expr))
 
-        for elt in self.expr.elements:
-            current_lll_node = Expr(elt, self.context).lll_node
-            if not out_type or not current_lll_node.typ.is_literal:
-                # prefer to use a non-literal type here, because literals can be ambiguous
-                # this should be removed altogether as we refactor types out of parser
-                out_type = current_lll_node.typ
-            lll_node.append(current_lll_node)
+    def parse_Tuple(self):
+        call_lll, multi_lll = parse_sequence(self.expr, self.expr.elements, self.context)
+        typ = TupleType([x.typ for x in multi_lll], is_literal=True)
+        multi_lll = LLLnode.from_list(["multi"] + multi_lll, typ=typ, pos=getpos(self.expr))
+        if not call_lll:
+            return multi_lll
 
-        return LLLnode.from_list(
-            ["multi"] + lll_node, typ=ListType(out_type, len(lll_node)), pos=getpos(self.expr),
-        )
+        lll_node = ["seq_unchecked"] + call_lll + [multi_lll]
+        return LLLnode.from_list(lll_node, typ=typ, pos=getpos(self.expr))
 
     @staticmethod
     def struct_literals(expr, name, context):
@@ -990,39 +987,6 @@ def struct_literals(expr, name, context):
             pos=getpos(expr),
         )
 
-    def parse_Tuple(self):
-        if not len(self.expr.elements):
-            return
-        call_lll = []
-        multi_lll = []
-        for node in self.expr.elements:
-            if isinstance(node, vy_ast.Call):
-                # for calls inside the tuple, we perform the call prior to building the tuple and
-                # assign it's result to memory - otherwise there is potential for memory corruption
-                lll_node = Expr(node, self.context).lll_node
-                target = LLLnode.from_list(
-                    self.context.new_internal_variable(lll_node.typ),
-                    typ=lll_node.typ,
-                    location="memory",
-                    pos=getpos(self.expr),
-                )
-                call_lll.append(make_setter(target, lll_node, "memory", pos=getpos(self.expr)))
-                multi_lll.append(
-                    LLLnode.from_list(
-                        target, typ=lll_node.typ, pos=getpos(self.expr), location="memory"
-                    ),
-                )
-            else:
-                multi_lll.append(Expr(node, self.context).lll_node)
-
-        typ = TupleType([x.typ for x in multi_lll], is_literal=True)
-        multi_lll = LLLnode.from_list(["multi"] + multi_lll, typ=typ, pos=getpos(self.expr))
-        if not call_lll:
-            return multi_lll
-
-        lll_node = ["seq_unchecked"] + call_lll + [multi_lll]
-        return LLLnode.from_list(lll_node, typ=typ, pos=getpos(self.expr))
-
     # Parse an expression that results in a value
     @classmethod
     def parse_value_expr(cls, expr, context):
@@ -1035,3 +999,63 @@ def parse_variable_location(cls, expr, context):
         if not o.location:
             raise StructureException("Looking for a variable location, instead got a value", expr)
         return o
+
+
+def parse_sequence(base_node, elements, context):
+    """
+    Generate an LLL node from a sequence of Vyper AST nodes, such as values inside a
+    list/tuple or arguments inside a call.
+
+    Arguments
+    ---------
+    base_node : VyperNode
+        Parent node which contains the sequence being parsed.
+    elements : List[VyperNode]
+        A list of nodes within the sequence.
+    context : Context
+        Currently active local context.
+
+    Returns
+    -------
+    List[LLLNode]
+        LLL nodes that must execute prior to generating the actual sequence in order to
+        avoid memory corruption issues. This list may be empty, depending on the values
+        within `elements`.
+    List[LLLNode]
+        LLL nodes which collectively represent `elements`.
+    """
+    init_lll = []
+    sequence_lll = []
+    for node in elements:
+        if isinstance(node, vy_ast.List):
+            # for nested lists, ensure the init LLL is also processed before the values
+            init, seq = parse_sequence(node, node.elements, context)
+            init_lll.extend(init)
+            out_type = next((i.typ for i in seq if not i.typ.is_literal), seq[0].typ)
+            typ = ListType(out_type, len(node.elements), is_literal=True)
+            multi_lll = LLLnode.from_list(["multi"] + seq, typ=typ, pos=getpos(node))
+            sequence_lll.append(multi_lll)
+            continue
+
+        lll_node = Expr(node, context).lll_node
+        if isinstance(node, vy_ast.Call) or (
+            isinstance(node, vy_ast.Subscript) and isinstance(node.value, vy_ast.Call)
+        ):
+            # nodes which potentially create their own internal memory variables, and so must
+            # be parsed prior to generating the final sequence to avoid memory corruption
+            target = LLLnode.from_list(
+                context.new_internal_variable(lll_node.typ),
+                typ=lll_node.typ,
+                location="memory",
+                pos=getpos(base_node),
+            )
+            init_lll.append(make_setter(target, lll_node, "memory", pos=getpos(base_node)))
+            sequence_lll.append(
+                LLLnode.from_list(
+                    target, typ=lll_node.typ, pos=getpos(base_node), location="memory"
+                ),
+            )
+        else:
+            sequence_lll.append(lll_node)
+
+    return init_lll, sequence_lll

vyper/parser/parser_utils.py

@@ -534,8 +534,14 @@ def make_setter(left, right, location, pos, in_function_call=False):
             left = LLLnode.from_list(["sha3_32", left], typ=left.typ, location="storage_prehashed")
             left_token.location = "storage_prehashed"
         # If the right side is a literal
-        if right.value == "multi":
-            subs = []
+        if right.value in ["multi", "seq_unchecked"] and right.typ.is_literal:
+            if right.value == "seq_unchecked":
+                # when the LLL is `seq_unchecked`, this is a literal where one or
+                # more values must be pre-processed to avoid memory corruption
+                subs = right.args[:-1]
+                right = right.args[-1]
+            else:
+                subs = []
             for i in range(left.typ.count):
                 lhs_setter = _make_array_index_setter(left, left_token, pos, location, i)
                 subs.append(make_setter(lhs_setter, right.args[i], location, pos=pos,))

vyper/parser/self_call.py

@@ -1,14 +1,13 @@
 import itertools
 
-from vyper import ast as vy_ast
 from vyper.codegen.abi import abi_decode
 from vyper.exceptions import (
     StateAccessViolation,
     StructureException,
     TypeCheckFailure,
 )
 from vyper.parser.lll_node import LLLnode
-from vyper.parser.parser_utils import getpos, make_setter, pack_arguments
+from vyper.parser.parser_utils import getpos, pack_arguments
 from vyper.signatures.function_signature import FunctionSignature
 from vyper.types import (
     BaseType,
@@ -57,37 +56,15 @@ def make_call(stmt_expr, context):
     # (x) pop return values
     # (x) pop local variables
 
-    pre_init = []
     pop_local_vars = []
     push_local_vars = []
     pop_return_values = []
     push_args = []
-
-    from vyper.parser.expr import Expr
-
     method_name = stmt_expr.func.attr
 
-    expr_args = []
-    for arg in stmt_expr.args:
-        lll_node = Expr(arg, context).lll_node
-        if isinstance(arg, vy_ast.Call):
-            # if the argument is a function call, perform the call seperately and
-            # assign it's result to memory, then reference the memory location when
-            # building this call. otherwise there is potential for memory corruption
-            target = LLLnode.from_list(
-                context.new_internal_variable(lll_node.typ),
-                typ=lll_node.typ,
-                location="memory",
-                pos=getpos(arg),
-            )
-            setter = make_setter(target, lll_node, "memory", pos=getpos(arg))
-            expr_args.append(
-                LLLnode.from_list(target, typ=lll_node.typ, pos=getpos(arg), location="memory")
-            )
-            pre_init.append(setter)
-        else:
-            expr_args.append(lll_node)
+    from vyper.parser.expr import parse_sequence
 
+    pre_init, expr_args = parse_sequence(stmt_expr, stmt_expr.args, context)
     sig = FunctionSignature.lookup_sig(context.sigs, method_name, expr_args, stmt_expr, context,)
 
     if context.is_constant() and sig.mutability not in ("view", "pure"):