Add ubsan suppressions for integer overflows in the getchaintxstats RPC.
getchainstatstx line "int nTxDiff = pindex->nChainTx - past_block.nChainTx" can
trigger ubsan integer overflows when assumeutxo snapshots are loaded, from
subtracting unsigned values and assigning the result to a signed int.
The overflow behavior probably exists in current code but is hard to trigger
because it would require calling getchainstatstx at the right time with
specific parameters as background blocks are being downloaded. But the overflow
behavior becomes easier to trigger in the upcoming commit removing fake
nChainTx values, so a suppression needs to be added before then for CI to pass.
getchainstatstx should probably be improved separately in another PR to not
need this suppression, and handle edge cases and missing nChainTx values more
carefully.
This is a refactor as long as no signed integer overflow appears. In
normal operation and absent bugs, signed integer overflow should never
happen in the touched code paths.
The main benefit of this refactor is to drop the file-wide ubsan
suppression unsigned-integer-overflow:txmempool.cpp.
For now, this only changes the internal private representation and the
publicly returned type remains uint64_t.
* The GCC suppression was fixed in gcc-11, which is available on all LTS
releases of Linux distros.
* The feerate suppression was likely fixed and does not trigger anymore.
If it was to trigger again, the underlying bug should be fixed instead
of suppressing it.
* The bench suppression does not trigger anymore.
Also, add comments to tsan suppressions on how to reproduce.
Xoroshiro128++ is a fast non-cryptographic random generator.
Reference implementation is available at https://prng.di.unimi.it/
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
fad84a2595 refactor: Fixup uint64_t-cast style in touched line (MarcoFalke)
fa041878de Fix implicit-integer-sign-change in bloom (MarcoFalke)
Pull request description:
Signed values don't really make sense when using `std::vector::operator[]`.
Fix that and remove the suppression.
ACKs for top commit:
PastaPastaPasta:
utACK fad84a2595
theStack:
Code-review ACK fad84a2595
Tree-SHA512: 7139dd9aa098c41e4af1b6e63dd80e71a92b0a98062d1676b01fe550ffa8e21a5f84a578afa7a536d70dad1b8a5017625e3a9e2dda6f864b452ec77b130ddf2a
fa2406a50a zmq: Fix implicit-integer-sign-change (MarcoFalke)
Pull request description:
uint256::begin() returns unsigned data, so there is no reason to make it signed.
Fix that and remove the sanitizer suppression.
ACKs for top commit:
hebasto:
ACK fa2406a50a
PastaPastaPasta:
utACK fa2406a50a, I have reviewed the code and think it makes sense
Tree-SHA512: 150ebcf3fdc3e0f60b6fd8e5fe638737b01e8a0863296bd545fb5ed17d33ab23b2ff94204996aa7b4617650b7383bd86ed2d2bf46746b410feae449de179a2bd
faa630aa15 test: Fix sanitizer suppresions in streams_tests (MarcoFalke)
Pull request description:
Two changes (that also make sense on their own) to remove the file-wide sanitizer suppression:
* `FindByte` no longer takes a `char`, but an `uint8_t`, after commit 196b459920.
* The `key` vector of unsigned chars can be removed and inlined as initializer-list. This avoids a bunch of verbose code like `clear()` and `push_back` of `char`s.
ACKs for top commit:
PastaPastaPasta:
utACK faa630aa15, I have reviewed the changes and agree it makes sense to merge
Tree-SHA512: 747b9d4676fad6d07f3955668639c93333625e69199ff4c499f01167de3875990d93db85e775a7f5b1b684575dceaec8aa000b4db15525fc47b699bac1c85e3d
fa832103aa Avoid integer sanitizer warnings in chain.o (MarcoFalke)
Pull request description:
The two changes make the code more self-documenting and also allow to remove 5 file-wide suppressions for the module
ACKs for top commit:
PastaPastaPasta:
utACK fa832103aa
jonatack:
ACK fa832103aa
Tree-SHA512: d32a06099c56eed9f69130a3209f989872acc593f849528acd7746ee6caa96688cc32de37e8e59ad5d25dcb8912e341f1a43e50642dadeff6ca7624d0873ad10
faa75fa193 Avoid unsigned integer overflow in bitcoin-tx (MarcoFalke)
Pull request description:
While `npos` means "largest unsigned value" and adding `1` to it yields `0`, it may be clearer to just assign `0` to it and only increment otherwise.
This also allows to remove a file-wide suppression for `unsigned-integer-overflow`.
ACKs for top commit:
hebasto:
ACK faa75fa193, I have reviewed the code and it looks OK, I agree it can be merged.
theStack:
Code-review ACK faa75fa193
Tree-SHA512: c24436641e5d801341c948b812c7f711d5dff70efdf04af00fd3221f4b81d93f25608dddaa36230ba81ca7ab0d18bdd957095d4561e22621e4d69017934f0a16
fa99e108e7 Fix implicit-integer-sign-change in arith_uint256 (MarcoFalke)
Pull request description:
This refactor doesn't change behaviour, but clarifies that the numbers being dealt with aren't supposed to be negative. This helps when reading the code and allows to remove a sanitizer suppression for the whole file.
ACKs for top commit:
PastaPastaPasta:
utACK fa99e108e7
shaavan:
ACK fa99e108e7
Tree-SHA512: f227e2fd22021e39f0445ec041f4a299d13477c23cef0fc06c53fb3313cbe550cec329336224a7e8775d9045b8009423052b394e83d42a1e40772085dfcdd471
Ryan Ofsky