vyperlang / vyper

Pythonic Smart Contract Language for the EVM
https://vyperlang.org
Other
4.85k stars 791 forks source link

fix[codegen]: panic on potential subscript eval order issue #4159

Closed charles-cooper closed 3 months ago

charles-cooper commented 3 months ago

What I did

panic on variant of https://github.com/vyperlang/vyper/pull/4157

How I did it

How to verify it

Commit message

subscript expressions have an evaluation order issue when
evaluation of the index (i.e. `node.index`) modifies the parent
(i.e. `node.value`). because the evaluation of the parent is
interleaved with evaluation of the index, it can result in "invalid"
reads where the length check occurs before evaluation of the index, but
the data read occurs afterwards. if evaluation of the index results in
modification of the container size for instance, the data read from the
container can happen on a dangling reference.

another variant of this issue would be accessing
`self.nested_array.pop().append(...)`; however, this currently happens
to be blocked by a panic in the frontend.

this commit conservatively blocks compilation if the preconditions for
the interleaved evaluation are detected. POC tests that the appropriate
panics are generated are included as well.

---------

Co-authored-by: trocher <trooocher@proton.me>
Co-authored-by: Hubert Ritzdorf <hubert.ritzdorf@chainsecurity.com>
Co-authored-by: cyberthirst <cyberthirst.eth@gmail.com>

Description for the changelog

Cute Animal Picture

image

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.

Project coverage is 46.22%. Comparing base (3d9c537) to head (286fb90). Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
vyper/codegen/core.py 10.00% 9 Missing :warning:
vyper/codegen/ir_node.py 25.00% 6 Missing :warning:
vyper/codegen/expr.py 60.00% 2 Missing :warning:

:exclamation: There is a different number of reports uploaded between BASE (3d9c537) and HEAD (286fb90). Click for more details.

HEAD has 139 uploads less than BASE | Flag | BASE (3d9c537) | HEAD (286fb90) | |------|------|------| ||140|1|
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #4159 +/- ## =========================================== - Coverage 91.32% 46.22% -45.10% =========================================== Files 109 109 Lines 15573 15606 +33 Branches 3420 3432 +12 =========================================== - Hits 14222 7214 -7008 - Misses 923 7838 +6915 - Partials 428 554 +126 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cyberthirst commented 3 months ago

passes

def test_array_index_overlap_extcall(get_contract):
    code = """

interface Bar:
    def bar() -> uint256: payable    

a: public(DynArray[DynArray[Bytes[96], 5], 5])

@external
def foo() -> Bytes[96]:
    self.a.append([b'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx'])
    return self.a[0][extcall Bar(self).bar()]

@external
def bar() -> uint256:
    self.a[0] = [b'yyy']
    self.a.pop()
    return 0
    """
    c = get_contract(code)
    assert c.foo() == b"yyy"