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: memory allocation in certain builtins using msize #3610

Merged
merged 16 commits into from
Sep 21, 2023

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Sep 19, 2023

What I did

fix for #3609

How I did it

How to verify it

Commit message

in certain builtins which use `msize` to allocate a buffer for their
arguments (specifically, `raw_call()`, `create_copy_of()` and
`create_from_blueprint()`), corruption of the buffer can occur when
`msize` is not properly initialized. (this usually happens when there
are no variables which are held in memory in the outer external
function). what can happen is that some arguments can be evaluated after
`msize` is evaluated, leading to overwriting the memory region for the
argument buffer with other arguments. specifically, combined with the
condition that `msize` is underinitialized, this can happen with:

- the buffer for the initcode of `create_copy_of()` and
  `create_from_blueprint()` can be overwritten when the `salt=` or
  `value=` arguments write to memory

- the buffer for the `data` argument (when `msg.data` is provided,
  prompting the use of `msize`) of `raw_call()` can be overwritten when
  the `to`, `gas=` or `value=` arguments write to memory

this commit fixes the issue by using a variant of `cache_when_complex()`
to ensure that the relevant arguments are evaluated before `msize` is
evaluated.

this is a patch for GHSA-c647-pxm2-c52w.

summarized changelog:
* fix raw_call
* test: raw_call with msg.data buffer clean memory
* force memory effects in some clean_mem tests
* add tests for clean memory in create_* functions
* add scope_multi abstraction
* refactor raw_call to use scope_multi
* add fixes for create_* memory cleanliness
* update optimizer tests -- callvalue is now considered constant
* move salt back into scope_multi
* add a note on reads in cache_when_complex

---------

Co-authored-by: Tanguy Rocher <[email protected]>

Description for the changelog

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Merging #3610 (8450c37) into master (1711569) will decrease coverage by 0.08%.
Report is 1 commits behind head on master.
The diff coverage is 72.58%.

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

@@            Coverage Diff             @@
##           master    #3610      +/-   ##
==========================================
- Coverage   89.12%   89.04%   -0.08%     
==========================================
  Files          86       86              
  Lines       11407    11450      +43     
  Branches     2595     2607      +12     
==========================================
+ Hits        10166    10196      +30     
- Misses        819      832      +13     
  Partials      422      422              
Files Changed Coverage Δ
vyper/codegen/ir_node.py 88.08% <57.50%> (-4.05%) ⬇️
vyper/builtins/functions.py 90.55% <100.00%> (+0.02%) ⬆️

... 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 marked this pull request as ready for review September 19, 2023 16:49
@charles-cooper charles-cooper requested review from fubuloubu and removed request for fubuloubu September 19, 2023 16:49
@charles-cooper charles-cooper marked this pull request as draft September 19, 2023 16:49
@charles-cooper charles-cooper marked this pull request as ready for review September 19, 2023 19:21
@charles-cooper charles-cooper changed the title fix: raw_call allocator fix: memory allocation in certain builtins using msize Sep 19, 2023
@trocher
Copy link
Contributor

trocher commented Sep 21, 2023

LGTM

@charles-cooper charles-cooper merged commit 79303fc into vyperlang:master Sep 21, 2023
82 checks passed
@charles-cooper charles-cooper deleted the fix/msize-usages branch September 21, 2023 14:51
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.

4 participants