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

advanced: Prevent invalid memory subscript index #339

Merged
merged 2 commits into from
Jun 2, 2021

Conversation

yperbasis
Copy link
Member

@yperbasis yperbasis commented Jun 1, 2021

triggered in MSVC debug build (Visual Studio 2019 version 16.10)

@chfast
Copy link
Member

chfast commented Jun 1, 2021

triggered in MSVC debug build

What version? Do you have CI log?

@axic
Copy link
Member

axic commented Jun 1, 2021

Do you have a unit test for this?

return evmc::make_result(
state.status, gas_left, &state.memory[state.output_offset], state.output_size);
return evmc::make_result(state.status, gas_left,
state.output_size != 0 ? &state.memory[state.output_offset] : nullptr, state.output_size);
Copy link
Member

Choose a reason for hiding this comment

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

This can be fixed with state.memory.data() + state.output_offset because evmc::make_result() will not use the pointer if output_size is 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #339 (498bb0a) into master (128d137) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 498bb0a differs from pull request most recent head ddbce46. Consider uploading reports for the commit ddbce46 to get more accurate results

@@           Coverage Diff           @@
##           master     #339   +/-   ##
=======================================
  Coverage   99.78%   99.78%           
=======================================
  Files          29       29           
  Lines        4108     4108           
=======================================
  Hits         4099     4099           
  Misses          9        9           
Flag Coverage Δ
consensus 91.19% <100.00%> (ø)
unittests 99.78% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
lib/evmone/execution.cpp 100.00% <100.00%> (ø)

@yperbasis
Copy link
Member Author

Do you have a unit test for this?

It was triggered by running Silkworm's core_test in Debug on Windows. Unfortunately, it didn't show in our Windows CI because it only has Release.

@axic
Copy link
Member

axic commented Jun 1, 2021

I suppose something like revert(100, 0) would work to trigger, assuming this is the beginning of the code, so memory was not expanded yet.

@chfast chfast force-pushed the win_out_of_range branch from 8819740 to dcee38a Compare June 1, 2021 21:23
@chfast chfast changed the title Prevent out of range access advanced: Prevent invalid memory subscript index Jun 1, 2021
@chfast chfast merged commit 3a2dbeb into ethereum:master Jun 2, 2021
@chfast chfast deleted the win_out_of_range branch June 2, 2021 06:25
@chfast chfast mentioned this pull request Jun 2, 2021
yperbasis added a commit to erigontech/silkworm that referenced this pull request Jun 2, 2021
yperbasis added a commit to erigontech/silkworm that referenced this pull request Jun 2, 2021
* Update evmone to prevent out of range access

* Switch back to upstream evm1 as ethereum/evmone/pull/339 is merged
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.

3 participants