mempool: Fix unsigned integer wraparounds in the mempool code #14223

pull practicalswift wants to merge 0 commits into bitcoin:master from practicalswift:unsigned-integer-wraparounds-in-mempool-code changing 0 files +0 −0

practicalswift commented at 7:13 AM on September 15, 2018: contributor

Fix unsigned integer wraparounds in the mempool code.

Before:

txmempool.cpp:219:44: runtime error: unsigned integer overflow: 18446744073709551615 * 155 cannot be represented in type 'unsigned long' !=
txmempool.cpp:308:26: runtime error: unsigned integer overflow: 223 + 18446744073709551532 cannot be represented in type 'unsigned long'
txmempool.cpp:311:27: runtime error: unsigned integer overflow: 2 + 18446744073709551615 cannot be represented in type 'unsigned long'
txmempool.cpp:317:24: runtime error: unsigned integer overflow: 2056 + 18446744073709549694 cannot be represented in type 'unsigned long'
txmempool.cpp:320:25: runtime error: unsigned integer overflow: 2 + 18446744073709551615 cannot be represented in type 'unsigned long' !=

After:

practicalswift commented at 7:21 AM on September 15, 2018: contributor

Our resident integer expert @arvidn of libtorrent fame might want to review? :-)
gmaxwell commented at 7:24 AM on September 15, 2018: contributor

nSizeWithDescendants = (uint64_t)((int64_t)nSizeWithDescendants + modifySize); assert(int64_t(nSizeWithDescendants) > 0);

This code ends up looking pretty deeply confused. The fix at 219 looks okay to me, the asserts should probably changed to typical overflow checking before the operations they're checking, rather than a whirlwind of casts. :)
fanquake added the label Mempool on Sep 15, 2018
DrahtBot commented at 9:11 AM on September 15, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #13804 (Transaction Pool Layer by MarcoFalke)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
in src/txmempool.cpp:308 in eb0dd77fbf outdated
```
 304 | @@ -305,19 +305,19 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
 305 |  
 306 |  void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount)
 307 |  {
 308 | -    nSizeWithDescendants += modifySize;
 309 | +    nSizeWithDescendants = (uint64_t)((int64_t)nSizeWithDescendants + modifySize);
```
arvidn commented at 4:52 PM on September 15, 2018:

could you explain what you're doing here? I get the impression that you intend to make it so that (unintended) unsigned wraparound do not happen. I don't think it's obvious how these casts help with that. Is modifySize legitimately negative sometimes? and is nSizeWithDescendants legitimately near INT64_MAX as a result?
practicalswift commented at 5:54 PM on September 15, 2018:

@arvidn Yes modifySize can be legitimately negative. Actually that condition and the resulting wraparound is trigged just by running the ordinary test suite:

$ ./configure --with-sanitizers=integer && \ make check && \ test/functional/test_runner.py … txmempool.cpp:308:26: runtime error: unsigned integer overflow: 223 + 18446744073709551532 cannot be represented in type 'unsigned long' …

Closing this PR for now: I'll go with @gmaxwell's suggestion with typical overflow checking instead of explicit casts to avoid this.
practicalswift closed this on Sep 15, 2018
practicalswift force-pushed on Sep 15, 2018
practicalswift commented at 5:50 PM on September 15, 2018: contributor

@gmaxwell Yes agree it looks weird a bit weird together with the old assert(…). Your suggestion to go with typical overflow checking is better. I don't know the rationale behind that assert(…): I'll have to dig in the repo and GitHub history too see. Closing this PR for now :-) I'll revisit when I have prepared a better solution which also gets rid of that old assert.
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels