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

EOF: Assert against overflow in header.data_offset #891

Merged
merged 1 commit into from
May 24, 2024
Merged

Conversation

pdobacz
Copy link
Collaborator

@pdobacz pdobacz commented May 15, 2024

While working on #878 I noticed the offset calculation in validate_header can overflow the uint16, if the declared sizes of container sections are large. Currently there is no upper limit on container size in validation (there's only the indirect limit imposed by max_initcode_size), so there can be a large container, which has a subcontainer of size 0xffff, and will have it's data_offset not fit the uint16.

We have 2 tests which happened to run into this, but the problem was masked (the data_offset was just really tiny, despite the container had a huge subcontainer, but it was never used, and the check for stray data wasn't affected by the overflow). I adjust them so (IMO) they still test what they're supposed to test, but not overflow.

@pdobacz pdobacz requested a review from gumb0 May 15, 2024 16:30
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@7f65121). Learn more about missing BASE report.

Current head 93d42c5 differs from pull request most recent head 282f575

Please upload reports for the commit 282f575 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #891   +/-   ##
=========================================
  Coverage          ?   98.49%           
=========================================
  Files             ?      130           
  Lines             ?    15630           
  Branches          ?        0           
=========================================
  Hits              ?    15394           
  Misses            ?      236           
  Partials          ?        0           
Flag Coverage Δ
ethereum-tests 27.95% <9.09%> (?)
ethereum-tests-silkpre 19.69% <0.00%> (?)
execution-spec-tests 19.05% <0.00%> (?)
unittests 94.34% <100.00%> (?)

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

Files Coverage Δ
lib/evmone/eof.cpp 87.33% <100.00%> (ø)
test/unittests/eof_validation_test.cpp 100.00% <100.00%> (ø)
...est/unittests/state_transition_eof_create_test.cpp 100.00% <100.00%> (ø)

@pdobacz pdobacz self-assigned this May 22, 2024
lib/evmone/eof.cpp Outdated Show resolved Hide resolved
@chfast chfast changed the title Assert against overflow in header.data_offset EOF: Assert against overflow in header.data_offset May 24, 2024
@chfast chfast added the EOF label May 24, 2024
@chfast chfast enabled auto-merge (squash) May 24, 2024 17:04
@chfast chfast merged commit 633f696 into master May 24, 2024
22 checks passed
@chfast chfast deleted the overflow branch May 24, 2024 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants