-
-
Notifications
You must be signed in to change notification settings - Fork 793
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: storage corruption from re-entrancy locks #2379
Fix: storage corruption from re-entrancy locks #2379
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2379 +/- ##
==========================================
- Coverage 85.79% 85.74% -0.06%
==========================================
Files 91 91
Lines 8992 8995 +3
Branches 2143 2145 +2
==========================================
- Hits 7715 7713 -2
- Misses 785 788 +3
- Partials 492 494 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on offline conversation with @iamdefinitelyahuman it sounds like len(_globals) is not really used anymore for storage allocation; that's now handled here
vyper/vyper/context/validation/data_positions.py
Lines 20 to 28 in d2f0a96
def set_storage_slots(vyper_module: vy_ast.Module) -> None: | |
""" | |
Parse module-level Vyper AST to calculate the layout of storage variables. | |
""" | |
available_slot = 0 | |
for node in vyper_module.get_children(vy_ast.AnnAssign): | |
type_ = node.target._metadata["type"] | |
type_.set_position(StorageSlot(available_slot)) | |
available_slot += math.ceil(type_.size_in_bytes / 32) |
@iamdefinitelyahuman I think this change is fine so long as you comment that it's a kludge for not making the storage allocation info (from set_storage_slots) available to callers, AND add comments explaining that the other uses of len(_globals) are dead code. Thanks!
7e747f2
to
9380b47
Compare
9380b47
to
37d62d6
Compare
What I did
Fix an issue allowing corruption of storage by re-entrancy locks.
In #2308, the location of re-entrancy locks was moved to the first unused storage slot. This worked fine as long as all multiple-slot types were stored using a sha3, but after #2361 this created a potential overlap between the lock slot and regular storage when the contract contains at least one storage value that occupies multiple slots.
How I did it
In
vyper/parser/global_context.py
, when calculating the offset for re-entrancy locks, correctly consider the size of each type.How to verify it
Run the tests. I expanded a test that I'd added in #2361 that focuses on checking the layout of storage - it now also includes checks involving re-entrancy locks.
Cute Animal Picture