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: block memory allocation overflow #3639

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

charles-cooper
Copy link
Member

What I did

fix #3637

How I did it

this fixes potential overflow bugs in pointer calculation by blocking memory allocation above a certain size. the size limit is set at 2**64, which is the size of addressable memory on physical machines.

practically, for EVM use cases, we could limit at a much smaller number (like 2**24), but we want to allow for "exotic" targets which may allow much more addressable memory.

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

this fixes potential overflow bugs in pointer calculation by blocking
memory allocation above a certain size. the size limit is set at
`2**64`, which is the size of addressable memory on physical machines.

practically, for EVM use cases, we could limit at a much smaller number
(like `2**24`), but we want to allow for "exotic" targets which may
allow much more addressable memory.
if self.size_of_mem >= 2**64:
# this should not be caught
raise MemoryAllocationException(
"Tried to allocate {self.size_of_mem} bytes! (limit is 2**32 bytes)"
Copy link
Member

Choose a reason for hiding this comment

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

probably should make it a constant and use it here as an f-string (and in the if statement)

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, right, i wanted to do it that way in the error message because 2**32 is easier to read than 4294967296.

Copy link
Member

Choose a reason for hiding this comment

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

oh, right, i wanted to do it that way in the error message because 2**32 is easier to read than 4294967296.

do the constant as a string and then parse it to int lol

Copy link
Member Author

Choose a reason for hiding this comment

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

i think eval() is a bit overkill here :P unless you are thinking there is a cleaner way of parsing the string?

Copy link
Member

Choose a reason for hiding this comment

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

or do log2

Copy link
Member Author

@charles-cooper charles-cooper Oct 4, 2023

Choose a reason for hiding this comment

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

i don't know about that. that results in an error message like

vyper.exceptions.MemoryAllocationException: Tried to allocate 18446744073709551712 bytes! (limit is 2**64.0 bytes)

and since floating point math is not very precise, we get the same error message for instance with _ALLOCATION_LIMIT = 2**64 - 1. so there is still the problem of drift between the error message and the value.

Copy link
Member

Choose a reason for hiding this comment

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

In [5]: MEM_LIMIT = 2**64

In [6]: f"2**{math.log2(MEM_LIMIT):2.0f}"
Out[6]: '2**64'

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2023

Codecov Report

Merging #3639 (9296ff4) into master (9136169) will decrease coverage by 0.05%.
Report is 2 commits behind head on master.
The diff coverage is 60.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #3639      +/-   ##
==========================================
- Coverage   89.13%   89.09%   -0.05%     
==========================================
  Files          86       86              
  Lines       11463    11467       +4     
  Branches     2606     2607       +1     
==========================================
- Hits        10218    10216       -2     
- Misses        826      831       +5     
- Partials      419      420       +1     
Files Coverage Δ
vyper/exceptions.py 97.34% <100.00%> (+0.02%) ⬆️
vyper/codegen/memory_allocator.py 58.46% <50.00%> (-1.22%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@charles-cooper charles-cooper merged commit 68da04b into vyperlang:master Oct 5, 2023
82 checks passed
@charles-cooper charles-cooper deleted the fix/overflow branch October 5, 2023 16:04
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.

Incorrect Integer Overflow Behavior
3 participants