-
Notifications
You must be signed in to change notification settings - Fork 329
Disallow unreachable code sections #721
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
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #721 +/- ##
=======================================
Coverage 97.90% 97.91%
=======================================
Files 110 110
Lines 10573 10644 +71
=======================================
+ Hits 10352 10422 +70
- Misses 221 222 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Please rebase and take into account sections targeted by JUMPF. |
|
Also please add a validation unit test with unreachable sections error. |
ff5e515 to
db57f5e
Compare
|
Note also you can now build an EOF bytecode with multiple section using |
170d99c to
727c180
Compare
In new tests I'm using that approach: https://github.com/ethereum/evmone/blob/7f1487c33e3f8974e7013140100665465a3c8a24/test/unittests/eof_validation_test.cpp#L1202-L1204 However, in some other parts, I'm generating the code "manually" in order to generate many code sections where each section calls the next one: https://github.com/ethereum/evmone/blob/7f1487c33e3f8974e7013140100665465a3c8a24/test/unittests/eof_validation_test.cpp#L1278-L1281 Not sure if there is a way to do something like that by using the |
test/unittests/eof_test.cpp
Outdated
| std::string section_types_256; | ||
| for (int i = 0; i < 256; ++i) | ||
| { | ||
| section_size_256 += "0003"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found a more concise way of making these sequences, I think I'll make a separate PR with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it in #796, please rebase now and use similar approach here.
| "040000 00" + 0x400 * bytecode{"00800000"} + code_sections_1024; | ||
| EXPECT_EQ(validate_eof(valid), EOFValidationError::success); | ||
|
|
||
| const auto invalid = "EF0001 011002" + bytecode{"020401"} + 0x401 * bytecode{"0001"} + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would make ssense to use code_sections_1024 in invalid case, too, just adding one more after them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added another variable code_sections_1025 in order that section 1024 is calling section 1025 (so we don't have unreachable code sections).
chfast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues I reported were fixed.
4807f11 to
6bd75ff
Compare
| std::string code_sections_256_err_254 = "E50001E50002"; | ||
| std::string cs_calling_next; | ||
| for (int i = 0; i < 252; ++i) | ||
| cs_calling_next += "E5" + hex(big_endian(static_cast<uint16_t>(i + 1))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this does what you intended, maybe loop index is wrong, e.g. code_sections_256_err_254 seems to end up with a sequence jumpf{1} jumpf{2} jumpf{1} jumpf{2} jumpf{3} ...
I think intention was to have jumpf{1} jumpf{2} jumpf{3} jumpf{4} ...
Also, if you want to use bytecode here, it should be possible with something like:
auto code_sections_256_err_001 = eof_bytecode(bytecode{JUMPF} + "0001").code(bytecode{JUMPF} + "0002", 0, 0x80, 0);
for (int i = 2; i < 254; ++i)
code_sections_256_err_001.code(bytecode{JUMPF} + big_endian(static_cast<uint16_t>(i + 1)), 0, 0x80, 0); (Also would be nice to have helpers in bytecode.hpp for callf / jumpf to not write bytecode{JUMPF} every time, but that's probably better for separate PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an error in the loop indices, it is fixed and it is now using bytecode.
c3eef35 to
89434de
Compare
|
Looks fine to me now, could you squash the commits? |
89434de to
da47d23
Compare
Commits squashed |
da47d23 to
b9ee992
Compare
This PR disallows EOF unreachable code sections.
Pending: