Fix parsing EOF version in header

[chfast]

Fixes the bug introduced in #947 where reading the version byte is missing the bounds check.

This bug is hard to detect when std::string is used as the test container. Found by a fuzzer.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.30%. Comparing base (26d5609) to head (1d357c2).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #957   +/-   ##
=======================================
  Coverage   94.29%   94.30%           
=======================================
  Files         143      143           
  Lines       16138    16142    +4     
=======================================
+ Hits        15218    15222    +4     
  Misses        920      920           

Flag	Coverage Δ
eof_execution_spec_tests	16.93% <14.28%> (-0.01%)	⬇️
ethereum_tests	26.90% <14.28%> (-0.01%)	⬇️
ethereum_tests_silkpre	18.68% <0.00%> (-0.01%)	⬇️
execution_spec_tests	17.90% <0.00%> (-0.01%)	⬇️
unittests	89.73% <100.00%> (+<0.01%)	⬆️

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

Files	Coverage Δ
lib/evmone/eof.cpp	85.66% <100.00%> (ø)
test/unittests/eof_validation.cpp	93.22% <100.00%> (+0.23%)	⬆️

[gumb0]

Can we have a test for this?

[chfast]

Can we have a test for this?

In theory we have: https://github.com/ethereum/evmone/blob/master/test/unittests/eof_validation_test.cpp#L38

[pdobacz]

I'd love to know more what's going on here (or have a test or some other guarantee this never materializes). How did the fuzzer manage to run into this?

[chfast]

I'd love to know more what's going on here (or have a test or some other guarantee this never materializes). How did the fuzzer manage to run into this?

I think I know how to fix the tests so I will submit it separately. When you use vector or string as the container the out-of-bounds read is not always detected because of the container's additional capacity: this memory is actually allocated and it is merely the out-of-bounds by the vector/string logic. In the string case, there is always the terminating null byte allocated in the container. Normally, address sanitizer misses this unless the container has special annotations for address sanitizer. Availability of these annotations depends on the stdlib vendor and version. We may not have it in CI. See https://github.com/google/sanitizers/wiki/AddressSanitizerContainerOverflow.

[pdobacz]

Thanks for the explanation, this is pure magic... Though how did the fuzzer run into this?

[chfast]

Though how did the fuzzer run into this?

It allocates the input buffer of exact size.

[chfast]

Do not merge yet.

[chfast]

This is the older PR where we use libc++ with address sanitizer. There you can see some container-overflows reported in the evmone-statetest tool but not in the EOF unit tests... https://app.circleci.com/pipelines/github/ethereum/evmone/7076/workflows/96de07f1-e206-4496-b7f8-fe92a055b350/jobs/103322/tests

[chfast]

This helps: #958.