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

practicalswift · 2018-10-18T09:29:10Z

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.

laanwj · 2018-10-18T14:43:47Z

src/arith_uint256.cpp

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


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

@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.

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

Can you add a test that fails without this fix?

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

@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.

laanwj · 2018-10-18T20:27:08Z

utACK 96f6dc9

gmaxwell · 2018-10-18T20:37:47Z

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

utACK

…::bits() 96f6dc9 Avoid triggering undefined behaviour in base_uint<BITS>::bits() (practicalswift) Pull request description: 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`. Tree-SHA512: 995fa38e71c8921873139ecf1b7dd54178555219af3be60d07290f379439ddd8479e3963c6f3cae8178efb61063a0f9add6cba82a5578d13888597b5bcd54f22

…DISABLE_FLAG bc60c61 Avoid 1 << 31 (UB) in calculation of SEQUENCE_LOCKTIME_DISABLE_FLAG (practicalswift) Pull request description: Avoid `1 << 31` (UB) in calculation of `SEQUENCE_LOCKTIME_DISABLE_FLAG`. Context: #14510 (comment) Tree-SHA512: bdb4a913c6a82ff1a455ba67d3351f6408ff4116574329361644b483fea96b801fdc5c5659233856b591cd3a46ec669d3b5b438553e4240d7099c560eae2e2ae

…DISABLE_FLAG Summary: PR14513 backport https://github.com/bitcoin/bitcoin/pull/14513/files bc60c615a5 Avoid 1 << 31 (UB) in calculation of SEQUENCE_LOCKTIME_DISABLE_FLAG (practicalswift) Pull request description: Avoid `1 << 31` (UB) in calculation of `SEQUENCE_LOCKTIME_DISABLE_FLAG`. Context: bitcoin/bitcoin#14510 (comment) Test Plan: `make check` Reviewers: Fabien, #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D3127

…t<BITS>::bits() 96f6dc9 Avoid triggering undefined behaviour in base_uint<BITS>::bits() (practicalswift) Pull request description: 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`. Tree-SHA512: 995fa38e71c8921873139ecf1b7dd54178555219af3be60d07290f379439ddd8479e3963c6f3cae8178efb61063a0f9add6cba82a5578d13888597b5bcd54f22

…CKTIME_DISABLE_FLAG bc60c61 Avoid 1 << 31 (UB) in calculation of SEQUENCE_LOCKTIME_DISABLE_FLAG (practicalswift) Pull request description: Avoid `1 << 31` (UB) in calculation of `SEQUENCE_LOCKTIME_DISABLE_FLAG`. Context: bitcoin#14510 (comment) Tree-SHA512: bdb4a913c6a82ff1a455ba67d3351f6408ff4116574329361644b483fea96b801fdc5c5659233856b591cd3a46ec669d3b5b438553e4240d7099c560eae2e2ae