grab bag of small UB fixes #2298
Conversation
| @@ -20,7 +24,11 @@ struct config { | |||
| // libtester disables the limits in all tests, except enforces the limits | |||
| // in the tests in unittests/eosvmoc_limits_tests.cpp. | |||
| std::optional<rlim_t> cpu_limit {20u}; | |||
| #if __has_feature(undefined_behavior_sanitizer) || __has_feature(address_sanitizer) | |||
| std::optional<rlim_t> vm_limit; // UBSAN & ASAN can add massive virtual memory usage; don't enforce vm limits when either of them are enabled | |||
There was a problem hiding this comment.
Don't need this for tests to pass: OC in unit tests always has vm_limit disabled now (except for the one modified test case), and python integration tests never use OC. But I'd like to use OC + ASAN/UBSAN with nodeos. Such usage is probably "developer enough" where simply disabling this limit is safe when configured that way; i.e. won't trap unsuspecting users. Alternatively could guard this disablement behind EOSVMOC_ENABLE_DEVELOPER_OPTIONS but ultimately I want to remove that option.
| @@ -40,7 +40,7 @@ memory::memory(uint64_t sliced_pages) { | |||
| uintptr_t* const intrinsic_jump_table = reinterpret_cast<uintptr_t* const>(zeropage_base - first_intrinsic_offset); | |||
| const intrinsic_map_t& intrinsics = get_intrinsic_map(); | |||
| for(const auto& intrinsic : intrinsics) | |||
| intrinsic_jump_table[-intrinsic.second.ordinal] = (uintptr_t)intrinsic.second.function_ptr; | |||
| intrinsic_jump_table[-(int)intrinsic.second.ordinal] = (uintptr_t)intrinsic.second.function_ptr; | |||
There was a problem hiding this comment.
This is an interesting one. The error here gets reported somewhat confusingly as addition of unsigned offset to 0x7aca262015f8 overflowed to 0x7aca26201570. I believe what is occurring is that ordinal is a size_t thus uint64_t, and negating it is still a uint64_t: now a really large uint64_t. But pointer arithmetic is defined on integer math; ptrdiff_t is a signed integer for example. So I think this is causing some sort of wrap around on a large signed integer. Casting ordinal to an int before the negation ensures it remains a small value int before pointer arithmetic. I'd be curious on other takes though.
There was a problem hiding this comment.
Should this use intptr_t (long int) instead of int?
There was a problem hiding this comment.
ordinal is the host function index over in,
so
int will always be plenty large enough (we won't have 231 host functions). My initial impression of intptr_t is that its purpose/connotation isn't really for indexes in to an array.
I do wonder if ordinal should just be an int instead; I don't think it'd be too hard to change it.
|
whoops, I pushed b47a64c here to the wrong branch; I wanted to have a separate PR for that. Sorry for force pushing it away... |
spoonincode
force-pushed
the
ub_grab_bag
branch
from
b47a64c
to
62e95b3
Compare
|
Note:start |
Brings in,
AntelopeIO/abieos#25
AntelopeIO/appbase#31
AntelopeIO/bn256#20
AntelopeIO/eos-vm#22
along with some other noted changes below.