Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Jun 19, 2024

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jun 18, 2024

What I did

panic on variant of #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

@@ -40,6 +40,7 @@
UnimplementedException,
tag_exceptions,
)
from vyper.semantics.analysis.utils import get_expr_writes

Check notice

Code scanning / CodeQL

Cyclic import Note

Import of module
vyper.semantics.analysis.utils
begins an import cycle.
@charles-cooper charles-cooper marked this pull request as ready for review June 18, 2024 17:15
@charles-cooper charles-cooper changed the title subscript overlap panic checker fix[codegen]: subscript overlap checker Jun 18, 2024
@charles-cooper charles-cooper changed the title fix[codegen]: subscript overlap checker fix[codegen]: panic on potential subscript eval order issue Jun 18, 2024
Copy link

codecov bot commented Jun 19, 2024

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 ⚠️
vyper/codegen/ir_node.py 25.00% 6 Missing ⚠️
vyper/codegen/expr.py 60.00% 2 Missing ⚠️

❗ 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
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cyberthirst
Copy link
Collaborator

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"

charles-cooper and others added 4 commits June 19, 2024 08:52
---------

Co-authored-by: cyberthirst <cyberthirst.eth@gmail.com>
`potential_overlap()` doesn't actually work for our purposes here, we
need a modification.
"""
c = get_contract(code)
# tricky to get this right, for now we just panic instead of generating code
assert c.foo() == [b"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be wrapped in [ ], no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm true, but if we ever modify codegen so it passes get_contract(), the test needs to be fixed anyways

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but anyway, updated -- a397c73

return 0
"""
c = get_contract(code)
assert c.foo() == [b"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if len(left.referenced_variables & right.variable_writes) > 0:
return True

if len(left.referenced_variables) > 0 and right.contains_risky_call:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably overly conservative as contains_risky_call checks against staticcall

@charles-cooper charles-cooper enabled auto-merge (squash) June 19, 2024 19:26
@charles-cooper charles-cooper merged commit 4594f8b into vyperlang:master Jun 19, 2024
160 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants