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.

Sign up for GitHub

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

Jump to bottom

Check stack height for OP_CALLF validation #619

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Check stack height for OP_CALLF validation #619

merged 1 commit into from
Aug 24, 2023

Conversation

rodiazet
Copy link
Collaborator

@rodiazet rodiazet commented Apr 19, 2023 •
edited
Loading

  • Add validation rule.
  • Add unit tests.
  • Fix already existing unit test.

@codecov
Copy link

codecov bot commented Apr 19, 2023 •
edited
Loading

Codecov Report

Merging #619 (0547115) into master (c9afe1f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #619      +/-   ##
==========================================
+ Coverage   97.55%   97.56%   +0.01%     
==========================================
  Files          88       88              
  Lines        8287     8338      +51     
==========================================
+ Hits         8084     8135      +51     
  Misses        203      203
Flag Coverage Δ
blockchaintests 63.05% <0.00%> (-0.15%) ⬇️
statetests 74.12% <60.00%> (-0.06%) ⬇️
statetests-silkpre 22.98% <0.00%> (-0.13%) ⬇️
unittests 95.38% <100.00%> (+0.05%) ⬆️

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

Files Changed Coverage Δ
lib/evmone/eof.hpp 100.00% <ø> (ø)
lib/evmone/eof.cpp 83.85% <100.00%> (+0.16%) ⬆️
test/unittests/eof_validation_test.cpp 100.00% <100.00%> (ø)
test/unittests/evm_eof_function_test.cpp 100.00% <100.00%> (ø)

@rodiazet rodiazet requested a review from gumb0 April 19, 2023 11:13
@rodiazet rodiazet force-pushed the callf-stack-height-validation branch from ddd195c to a43448e Compare April 19, 2023 12:07
@chfast chfast added the EOF label Apr 23, 2023
gumb0
gumb0 reviewed Apr 24, 2023
View reviewed changes
lib/evmone/eof.cpp Outdated Show resolved Hide resolved
@gumb0 gumb0 force-pushed the callf-stack-height-validation branch 2 times, most recently from 2a0e34b to c7d1224 Compare July 20, 2023 08:42
@gumb0
Copy link
Member

gumb0 commented Jul 21, 2023

State tests currently seem to have a bug ethereum/tests#1238 (comment)

@gumb0
Copy link
Member

gumb0 commented Jul 24, 2023

Similar validation-time check should be added to JUMPF (but JUMPF is not merged yet: #644)

This was referenced Jul 24, 2023
Merged
Merged
@gumb0 gumb0 force-pushed the callf-stack-height-validation branch from c7d1224 to 960de10 Compare July 31, 2023 14:35
@gumb0 gumb0 marked this pull request as draft July 31, 2023 15:13
@gumb0 gumb0 force-pushed the callf-stack-height-validation branch 4 times, most recently from ff73384 to 168c5ba Compare August 3, 2023 13:19
gumb0
gumb0 reviewed Aug 3, 2023
View reviewed changes
lib/evmone/eof.cpp
if (opcode == OP_CALLF)
{
const auto fid = read_uint16_be(&code[i + 1]);

stack_height_required = static_cast<int8_t>(code_types[fid].inputs);

if (stack_height + code_types[fid].max_stack_height - stack_height_required >
Copy link
Member

Choose a reason for hiding this comment

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

In case of stack underflow this expression can be negative, I think it's ok, it's signed and won't underflow

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we have an unit test covering this?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't found any tests for stack underflow validation at all. I will add some at least for CALLF.

Copy link
Member

@gumb0 gumb0 Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I was wrong, it cannot be negative, because callee's max_stack_height >= inputs

@gumb0 gumb0 requested review from axic and chfast August 3, 2023 13:25
@gumb0 gumb0 marked this pull request as ready for review August 3, 2023 13:42
@gumb0 gumb0 mentioned this pull request Aug 19, 2023
Merged
@gumb0 gumb0 force-pushed the callf-stack-height-validation branch from 168c5ba to d890cf6 Compare August 19, 2023 10:23
@gumb0
Copy link
Member

gumb0 commented Aug 19, 2023

Rebased.

@rodiazet rodiazet force-pushed the callf-stack-height-validation branch from d890cf6 to 34773dc Compare August 22, 2023 09:05
@gumb0 gumb0 force-pushed the callf-stack-height-validation branch 3 times, most recently from cefad46 to f4749a3 Compare August 22, 2023 10:08
chfast
chfast approved these changes Aug 23, 2023
View reviewed changes
lib/evmone/eof.cpp
@@ -341,18 +342,23 @@ std::variant<EOFValidationError, int32_t> validate_max_stack_height(
auto stack_height_required = instr::traits[opcode].stack_height_required;
auto stack_height_change = instr::traits[opcode].stack_height_change;

auto stack_height = stack_heights[i];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto stack_height = stack_heights[i];
const auto stack_height = stack_heights[i];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's modified in line 365

test/unittests/eof_validation_test.cpp Outdated
{
{
auto code = eof1_bytecode(
512 * push(1) + OP_CALLF + bytecode{"0x0000"_hex} + 510 * OP_POP + OP_RETURN, 512);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
512 * push(1) + OP_CALLF + bytecode{"0x0000"_hex} + 510 * OP_POP + OP_RETURN, 512);
512 * push(1) + OP_CALLF + "0000" + 510 * OP_POP + OP_RETURN, 512);

bytecode can consume strings and will interpret them as hex.

@rodiazet rodiazet force-pushed the callf-stack-height-validation branch from f4749a3 to 450d32d Compare August 24, 2023 07:59
@rodiazet @gumb0
Check stack height for OP_CALLF
0547115 
Co-authored-by: Andrei Maiboroda <[email protected]>
@rodiazet rodiazet force-pushed the callf-stack-height-validation branch from 450d32d to 0547115 Compare August 24, 2023 08:01
@rodiazet rodiazet merged commit ebeb258 into master Aug 24, 2023
3 checks passed
@rodiazet rodiazet deleted the callf-stack-height-validation branch August 24, 2023 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gumb0 gumb0 gumb0 left review comments

@chfast chfast chfast approved these changes

@axic axic Awaiting requested review from axic

Assignees
No one assigned
Labels
EOF
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rodiazet @gumb0 @chfast