Avoid triggering undefined behaviour in base_uint<BITS>::bits() #14510

1. 
   practicalswift commented at 9:29 AM on October 18, 2018: contributor

   Avoid triggering undefined behaviour in base_uint<BITS>::bits().

   1 << 31 is undefined behaviour in C++11.

   Given the reasonable assumption of sizeof(int) * CHAR_BIT == 32.

2. Avoid triggering undefined behaviour in base_uint<BITS>::bits() 96f6dc9fc5
3. fanquake requested review from sipa on Oct 18, 2018
4. in src/arith_uint256.cpp:179 in 96f6dc9fc5

    175 | @@ -176,7 +176,7 @@ unsigned int base_uint<BITS>::bits() const
    176 |      for (int pos = WIDTH - 1; pos >= 0; pos--) {
    177 |          if (pn[pos]) {
    178 |              for (int nbits = 31; nbits > 0; nbits--) {
    179 | -                if (pn[pos] & 1 << nbits)
    180 | +                if (pn[pos] & 1U << nbits)
   

   ---

   ---

   

   laanwj commented at 2:43 PM on October 18, 2018:

   pn[...] is uint32_t, how is this ever undefined behavior? I don't think C ever "promotes" uint32_t to int.

   ---

   

   sipa commented at 7:07 PM on October 18, 2018:

   @laanwj With parenthesis it's pn[pos] & (1 << nbits); I believe the type of (1 << nbits) is indeed int, which in case of nbits == 31 is exceeding the size of the type.

   ---

   

   laanwj commented at 8:24 PM on October 18, 2018:

   so let's make the counter an 'unsigned' then? edit: oh, never mind, it's counting down, that wouldn't really work either...

   ---

   

   promag commented at 8:37 PM on October 18, 2018:

   Can you add a test that fails without this fix?

   ---

   

   laanwj commented at 8:52 PM on October 18, 2018:

   no, he can't—the whole problem, the whole shitshow with C and C++ is that you can't predict what will happen with undefined behavior—it can just work with one compiler or version and commit war crimes the next one

   ---

   

   practicalswift commented at 9:03 PM on October 18, 2018:

   @promag How would one test for the presence of undefined behaviour? :-)

   If you are referring to the sanitisers there are no sanitisers flagging this UB AFAIK.

5. 
   laanwj commented at 8:27 PM on October 18, 2018: member

   utACK 96f6dc9fc50b1cc59e26d50940ebf46e1bdcc0ba

6. 
   gmaxwell commented at 8:37 PM on October 18, 2018: contributor

   This should also get fixed src/primitives/transaction.h: static const uint32_t SEQUENCE_LOCKTIME_DISABLE_FLAG = (1 << 31);

   utACK

7. laanwj merged this on Oct 18, 2018
8. laanwj closed this on Oct 18, 2018

   ---

9. laanwj referenced this in commit 3715b2489e on Oct 18, 2018
10. sipa referenced this in commit b14db5abab on Oct 20, 2018
11. jasonbcox referenced this in commit 92889d584b on May 31, 2019
12. jtoomim referenced this in commit df92c5968f on Jun 29, 2019
13. practicalswift deleted the branch on Apr 10, 2021
14. PastaPastaPasta referenced this in commit 178dc378d0 on Jun 27, 2021
15. PastaPastaPasta referenced this in commit 8f8ce78663 on Jun 27, 2021
16. PastaPastaPasta referenced this in commit 744590af09 on Jun 28, 2021
17. PastaPastaPasta referenced this in commit 7d379e2257 on Jun 28, 2021
18. PastaPastaPasta referenced this in commit 18a96c5b26 on Jun 29, 2021
19. PastaPastaPasta referenced this in commit ed545447b8 on Jun 29, 2021
20. PastaPastaPasta referenced this in commit feae265077 on Jul 1, 2021
21. PastaPastaPasta referenced this in commit 14241cd52a on Jul 1, 2021
22. PastaPastaPasta referenced this in commit 3dcf5046b9 on Jul 1, 2021
23. PastaPastaPasta referenced this in commit c9bad29c45 on Jul 1, 2021
24. PastaPastaPasta referenced this in commit c89354dde2 on Jul 1, 2021
25. PastaPastaPasta referenced this in commit c72fff6bc4 on Jul 1, 2021
26. gades referenced this in commit 80bc3f9f42 on Apr 23, 2022
27. DrahtBot locked this on Aug 16, 2022

Contributors

practicalswiftlaanwjsipapromaggmaxwell

Review Requested
sipa