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: storage corruption from re-entrancy locks #2379

Conversation

iamdefinitelyahuman
Copy link
Contributor

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

image

@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2021

Codecov Report

Merging #2379 (37d62d6) into master (d2f0a96) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
vyper/parser/global_context.py 68.65% <100.00%> (ø)
vyper/cli/vyper_compile.py 75.00% <0.00%> (-1.15%) ⬇️
vyper/functions/functions.py 88.36% <0.00%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2f0a96...37d62d6. Read the comment docs.

Copy link
Member

@charles-cooper charles-cooper left a 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

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!

@iamdefinitelyahuman iamdefinitelyahuman merged commit 60f8767 into vyperlang:master Jul 8, 2021
@iamdefinitelyahuman iamdefinitelyahuman deleted the fix/reentrancy-corruption branch July 8, 2021 01:11
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.

None yet

3 participants