Remove UBSan suppressions for CTxMemPool* #17791

1. 
   hebasto commented at 8:33 pm on December 22, 2019: member

   In CTxMemPoolEntry class the type of four data members has been changed from uint64_t to int64_t. The type of getters, uint64_t, remains unchanged.

   In CTxMemPool::UpdateAncestorsOf() the only expression was needed to be reworked (the result looks a bit ugly, I’m still open for suggestions).

2. fanquake added the label Mempool on Dec 22, 2019
3. in src/txmempool.cpp:221 in ed1a916ed4 outdated

   216@@ -217,10 +217,11 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors
   217         UpdateChild(piter, it, add);
   218     }
   219     const int64_t updateCount = (add ? 1 : -1);
   220-    const int64_t updateSize = updateCount * it->GetTxSize();
   221+    const int64_t updateSize = it->GetTxSize();
   222+    assert(updateSize > 0);
   

   ---

   ---

   

   promag commented at 9:08 pm on December 22, 2019:

   Why are you adding this? GetTxSize returns size_t and asserting != 0 is new behavior. I think this could be dropped.

   ---

   

   hebasto commented at 10:31 pm on December 22, 2019:

   GetTxSize returns size_t and asserting != 0 is new behavior.

   Fixed.

   I think this could be dropped.

   Let’s keep assert for safety ;)

4. 
   promag commented at 9:09 pm on December 22, 2019: member

   Concept ACK.
5. 
   practicalswift commented at 9:27 pm on December 22, 2019: contributor

   Concept ACK
6. hebasto force-pushed on Dec 22, 2019
7. 
   DrahtBot commented at 11:13 pm on December 22, 2019: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21000 (fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer by practicalswift)

   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.

8. 
   practicalswift commented at 11:20 pm on December 22, 2019: contributor

   Nit regarding the commit message “Fix UB in CTxMemPool*”:

   Unsigned integer wraparound and implicit integer sign change are not UB, but they are often unintentional - that is why UBSan is flagging them :)

   Perhaps the PR title (“Remove UBSan suppressions for CTxMemPool*”) could be used as commit message instead?

   -fsanitize=implicit-integer-sign-change: Implicit conversion between integer types, if that changes the sign of the value. That is, if the the original value was negative and the new value is positive (or zero), or the original value was positive, and the new value is negative. Issues caught by this sanitizer are not undefined behavior, but are often unintentional.

   -fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where the result of an unsigned integer computation cannot be represented in its type. Unlike signed integer overflow, this is not undefined behavior, but it is often unintentional.

9. Remove UBSan suppressions for CTxMemPool* 52a45c393c
10. hebasto force-pushed on Dec 22, 2019
11. 
    hebasto commented at 11:25 pm on December 22, 2019: member

    @practicalswift

    Nit regarding the commit message “Fix UB in CTxMemPool*”…

    Agree with every word ;) Fixed.

12. 
    practicalswift commented at 7:19 am on December 23, 2019: contributor

    ACK 52a45c393c7cf4309b754d42c240e9652f825bbe – diff looks correct
13. in src/txmempool.cpp:221 in 52a45c393c

    216@@ -217,10 +217,11 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors
    217         UpdateChild(piter, it, add);
    218     }
    219     const int64_t updateCount = (add ? 1 : -1);
    220-    const int64_t updateSize = updateCount * it->GetTxSize();
    221+    const int64_t updateSize = it->GetTxSize();
    222+    assert(updateSize >= 0);
    

    ---

    ---

    

    Empact commented at 8:38 pm on February 20, 2020:

    nit: this is now the size of the tx, not the update

    Maybe assert against the it->GetTxSize directly? Would mean just adding one line here. If you’re concerned about the repeated calls, something like this: https://github.com/Empact/bitcoin/commit/0018dd13b6799d7dbaafc8cdce66c8a0689083bd would make them trivial.

14. 
    Empact commented at 8:48 pm on February 20, 2020: member

    Concept ACK
15. 
    DrahtBot commented at 9:08 am on January 26, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

16. DrahtBot added the label Needs rebase on Jan 26, 2021
17. hebasto closed this on Aug 24, 2021

    ---

18. DrahtBot locked this on Aug 24, 2022

Contributors
hebasto promag practicalswift DrahtBot Empact

Labels
Mempool Needs rebase