Unsigned integer overflow is well defined by the language and in some cases even useful or necessary. However, I think that it should be avoided in interpreter, as it makes the code harder to read and requires the whole file to be suppressed in the sanitizer. This puts more burden on reviewers to check that any changes to interpreter that involve unsigned integer overflow are sane.
This patch involves a few changes:
- Evaluate the addition in 64-bit “space”. Previously, the first argument was
size_t(unsigned, 32-bit or 64-bit, depending on platform) and the second wasint(32-bit on all supported platforms). Thus the addition was done in 32-bit or 64-bit “unsigned space”. Now the addition is done in 64-bit “signed space” on all platforms. This is safe because signed integer overflow (UB) isn’t expected here with 64-bit integers. - Clarify that the value passed to the “stack macros” always fits in an
int64_t. This is done with the C++11 syntaxint64_t{i}, which fails to compile ifineeds to be narrowed to fit intoint64_t. - Explicitly convert the result of the addition to
size_t. This isn’t needed, because the called function already converts the value (see https://en.cppreference.com/w/cpp/container/vector/operator_at), however I have a slight preference for the explicit cast. (Happy to remove if reviewers prefer without)
The patch does not change the bitcoind binary on my 64-bit system with clang++ -O2. However, it does change with gcc.
