diff options
| author | MacroFake <falke.marco@gmail.com> | 2022-06-27 08:25:13 +0200 |
|---|---|---|
| committer | MacroFake <falke.marco@gmail.com> | 2022-06-27 08:25:19 +0200 |
| commit | dde7205c576352a6052c1411d0fa8017e83d3ef4 (patch) | |
| tree | 89863ef994c69ef001d487249dac1ea89eb78603 /test | |
| parent | aaeb315ff0f7956449a92736160795f0140369e3 (diff) | |
| parent | fa07f84e316171d60dd9941fb8db37e0a0de6654 (diff) | |
| download | bitcoin-dde7205c576352a6052c1411d0fa8017e83d3ef4.tar.xz | |
Merge bitcoin/bitcoin#23418: Fix signed integer overflow in prioritisetransaction RPC
fa07f84e316171d60dd9941fb8db37e0a0de6654 Fix signed integer overflow in prioritisetransaction RPC (MarcoFalke)
fa52cf8e11b3af6e0a302d5d17aab6cea78899d5 refactor: Replace feeDelta by m_modified_fee (MarcoFalke)
Pull request description:
Signed integer overflow is UB in theory, but not in practice. Still,
it would be nice to avoid this UB to allow Bitcoin Core to be
compiled with sanitizers such as `-ftrapv` or ubsan.
It is impossible to predict when and if an overflow occurs, since
the overflow caused by a prioritisetransaction RPC might only be
later hit when descendant txs are added to the mempool.
Since it is impossible to predict reliably, leave it up to the user
to use the RPC endpoint responsibly, considering their mempool
limits and usage patterns.
Fixes: #20626
Fixes: #20383
Fixes: #19278
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34146 / https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47132
## Steps to reproduce
Build the code without the changes in this pull.
Make sure to pass the sanitizer flag:
```
./autogen.sh && ./configure --with-sanitizers=signed-integer-overflow && make clean && make -j $(nproc)
```
### Reproduce on RPC
```
./src/bitcoind -chain=regtest -noprinttoconsole &
./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789
./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789
|> txmempool.cpp:920:15: runtime error: signed integer overflow: 9123456789123456789 + 9123456789123456789 cannot be represented in type 'long int'
./src/bitcoin-cli -chain=regtest stop
```
### By fuzzing
```
wget https://github.com/bitcoin/bitcoin/files/8921302/clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt
FUZZ=validation_load_mempool ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt
|> txmempool.cpp:920:15: runtime error: signed integer overflow: 7214801925397553184 + 2314885530818453536 cannot be represented in type 'long int'
|> validation_load_mempool: succeeded against 1 files in 0s.
ACKs for top commit:
vasild:
ACK fa07f84e316171d60dd9941fb8db37e0a0de6654
dunxen:
ACK fa07f84
LarryRuane:
ACK fa07f84e316171d60dd9941fb8db37e0a0de6654
Tree-SHA512: 4a357950af55a49c9113da0a50c2e743c5b752f0514dd8d16cd92bfde2f77dd0ef56aa98452626df6f7f7a5b51d1227021f6bc94091201a179f0d488ee32a0df
Diffstat (limited to 'test')
| -rw-r--r-- | test/sanitizer_suppressions/ubsan | 4 |
1 files changed, 2 insertions, 2 deletions
diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index e6cfe5f81a..67ef512895 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -1,10 +1,10 @@ # -fsanitize=undefined suppressions # ================================= -# This would be `signed-integer-overflow:CTxMemPool::PrioritiseTransaction`, +# The suppressions would be `sanitize-type:ClassName::MethodName`, # however due to a bug in clang the symbolizer is disabled and thus no symbol # names can be used. # See https://github.com/google/sanitizers/issues/1364 -signed-integer-overflow:txmempool.cpp + # https://github.com/bitcoin/bitcoin/pull/21798#issuecomment-829180719 signed-integer-overflow:policy/feerate.cpp |