p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning #19818

jonatack · 2020-08-27T09:11:24Z

Fixes UBSan implicit-integer-sign-change issue per #19610 (comment).

Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in #19590 (review).

Closes #19678.

fixes issue bitcoin#19678 UBSan implicit-integer-sign-change Credit to Eugene (Crypt-iQ) for finding and reporting the issue and to Vasil Dimov (vasild) for the original suggestion

laanwj · 2020-08-27T09:17:22Z

src/protocol.h

@@ -425,7 +425,7 @@ class CInv
    // Combined-message helper methods
    bool IsGenTxMsg()     const { return type == MSG_TX || type == MSG_WTX || type == MSG_WITNESS_TX; }

-    int type;
+    uint32_t type;


ACK on using a sized type here.
However, changing from signed to unsigned is, in principle, a P2P protocol change.
This needs to be carefully reviewed for potential unexpected by-effects.

maflcko · 2020-08-27T09:31:56Z

According to https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors (section written in 2010 by "MagicalTux") this is unsigned. Though, our python tests also treat this as signed. https://btcinformation.org/en/developer-reference#inv doesn't say anything about signedness.

Would be good to at least update the python test framework and also add a p2p test.

jonatack · 2020-08-27T14:13:47Z

Would be good to at least update the python test framework and also add a p2p test.

Updated the python test framework and added test coverage.

test/functional/p2p_invalid_messages.py

practicalswift · 2020-08-27T18:18:06Z

Concept ACK

Thanks for fixing UBSan fuzzing finds!

practicalswift · 2020-08-28T05:48:13Z

@jonatack Just to confirm: if the test changes we're dropped in this change set (keeping only the changes to src/protocol.{cpp,h}) then all functional tests would still pass, right?

test/functional/p2p_invalid_messages.py

jonatack · 2020-08-28T07:53:07Z

@jonasnick Just to confirm: if the test changes we're dropped in this change set (keeping only the changes to src/protocol.{cpp,h}) then all functional tests would still pass, right?

Right. I pushed the changes to src/protocol.{h/cpp} several times in other PR changesets (#19610, #19611) before dropping them, and the CI passes.

jnewbery · 2020-08-28T11:38:37Z

According to https://en.bitcoin.it/wiki/Protocol_documentation#Inventory_Vectors (section written in 2010 by "MagicalTux") this is unsigned. Though, our python tests also treat this as signed. https://btcinformation.org/en/developer-reference#inv doesn't say anything about signedness.

btcinformation.org does show this as uint32_t: https://btcinformation.org/en/developer-reference#data-messages. That was added by @harding to the bitcoin.org developer docs here: bitcoin-dot-org/Bitcoin.org@1634212#diff-693f5b3e9d88b97c899694c8d1656cbbR110

Note that even though comparing an int to a uint32_t causes a UBSan warning, it's not actually undefined behaviour: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#:~:text=%2Dfsanitize%3Dimplicit%2Dinteger%2D,the%20new%20value%20is%20negative. However, I think it's still worth changing this, so that we're always comparing / xor'ing a uint32_t with a uint32_t, so concept ACK.

jonatack · 2020-08-28T11:54:28Z

Great links @jnewbery, thanks.

Note that even though comparing an int to a uint32_t causes a UBSan warning, it's not actually undefined behaviour: clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#:~:text=%2Dfsanitize%3Dimplicit%2Dinteger%2D,the%20new%20value%20is%20negative.

"-fsanitize=implicit-integer-sign-change: Implicit conversion between integer types, if that changes the sign of the value...issues caught by this sanitizer are not undefined behavior, but are often unintentional."

test/functional/test_framework/messages.py

jonatack · 2020-08-28T18:19:42Z

Thanks everyone for your feedback. I wrote a few tests but they all covered the change to the CInv class in the test framework, not the CInv change in protocol.{h,cpp}, so I've simplified and removed them in the latest push.

laanwj · 2020-08-29T12:51:55Z

Thanks for addressing the comments, the code changes look good to me now.

vasild

ACK 7984c39

All the values we ever assign to CInv::type are non-negative within the reach of a signed 32 bit integer. GetDataMsg is uint32_t and in some places we |= a variable of type uint32_t into CInv::type:

bitcoin/src/net_processing.cpp

Line 2717 in bab4cce

inv.type |= nFetchFlags;

So it makes sense to have CInv::type as uint32_t.

Actually serializing a signed integer in the way we do it is not portable, it just works because all platforms use two's complement, but that's not guaranteed.

laanwj · 2020-09-02T14:17:07Z

All the values we ever assign to CInv::type are non-negative within the reach of a signed 32 bit integer. GetDataMsg is uint32_t and in some places we |= a variable of type uint32_t into CInv::type

Bitwise arithmetic should definitely use unsigned integers, good point.

Actually serializing a signed integer in the way we do it is not portable, it just works because all platforms use two's complement, but that's not guaranteed.

I thought this was already guaranteed in C++11, but no, apparently only C++20 will guarantee two's complement signed integers.

laanwj · 2020-09-03T11:29:58Z

ACK 7984c39

maflcko · 2020-09-03T14:42:06Z

ACK 7984c39 🌻

Show signature and timestamp

Signature:

Timestamp of file with hash a72c948f27f3e6c7aa323c708d61eff780f8ac68076405b22c540f1cb6199d2f -

…`, fix UBSan warning 7984c39 test framework: serialize/deserialize inv type as unsigned int (Jon Atack) 407175e p2p: change CInv::type from int to uint32_t (Jon Atack) Pull request description: Fixes UBSan implicit-integer-sign-change issue per bitcoin#19610 (comment). Credit to Crypt-iQ for finding and reporting the issue and to vasild for the original review suggestion in bitcoin#19590 (review). Closes bitcoin#19678. ACKs for top commit: laanwj: ACK 7984c39 MarcoFalke: ACK 7984c39 🌻 vasild: ACK 7984c39 Tree-SHA512: 59f3a75f40ce066ca6f0bb1927197254238302b4073af1574bdbfe6ed580876437be804be4e47d51467d604f0d9e3a5875159f7f2edbb2351fdb2bb9465100b5

p2p: change CInv::type from int to uint32_t

407175e

fixes issue bitcoin#19678 UBSan implicit-integer-sign-change Credit to Eugene (Crypt-iQ) for finding and reporting the issue and to Vasil Dimov (vasild) for the original suggestion

This was referenced Aug 27, 2020

p2p: change CInv::type from int to uint32_t, fix UBSan warning #19611

Closed

p2p: refactor AlreadyHave(), CInv::type, INV/TX processing #19610

Merged

laanwj reviewed Aug 27, 2020

View reviewed changes

fanquake added the P2P label Aug 27, 2020

jonatack force-pushed the CInv-type-refactoring branch 2 times, most recently from ac8a804 to 9e05ed2 Compare August 27, 2020 14:02

maflcko reviewed Aug 27, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

jonatack force-pushed the CInv-type-refactoring branch 2 times, most recently from 1ab179b to 6c0f4ae Compare August 27, 2020 17:47

maflcko reviewed Aug 27, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

jonatack force-pushed the CInv-type-refactoring branch from 6c0f4ae to 1bb9a92 Compare August 27, 2020 19:05

maflcko reviewed Aug 28, 2020

View reviewed changes

test/functional/p2p_invalid_messages.py Outdated Show resolved Hide resolved

jonatack force-pushed the CInv-type-refactoring branch from 1bb9a92 to 7bfc631 Compare August 28, 2020 08:44

laanwj reviewed Aug 28, 2020

View reviewed changes

test/functional/test_framework/messages.py Outdated Show resolved Hide resolved

laanwj reviewed Aug 28, 2020

View reviewed changes

test/functional/test_framework/messages.py Outdated Show resolved Hide resolved

test framework: serialize/deserialize inv type as unsigned int

7984c39

jonatack force-pushed the CInv-type-refactoring branch from 7bfc631 to 7984c39 Compare August 28, 2020 18:13

vasild approved these changes Sep 1, 2020

View reviewed changes

laanwj merged commit bd60a9a into bitcoin:master Sep 3, 2020

jonatack deleted the CInv-type-refactoring branch September 3, 2020 15:30

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

p2p: change CInv::type from int to uint32_t, fix UBSan warning #19818

p2p: change CInv::type from int to uint32_t, fix UBSan warning #19818

Uh oh!

Conversation

jonatack commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Aug 27, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 27, 2020

Uh oh!

jonatack commented Aug 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Aug 27, 2020

Uh oh!

practicalswift commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jonatack commented Aug 28, 2020

Uh oh!

jnewbery commented Aug 28, 2020

Uh oh!

jonatack commented Aug 28, 2020

Uh oh!

Uh oh!

Uh oh!

jonatack commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 29, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 3, 2020

Uh oh!

Uh oh!

p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning #19818

p2p: change `CInv::type` from `int` to `uint32_t`, fix UBSan warning #19818

jonatack commented Aug 27, 2020 •

edited

Loading

jonatack commented Aug 27, 2020 •

edited

Loading

practicalswift commented Aug 28, 2020 •

edited

Loading

jonatack commented Aug 28, 2020 •

edited

Loading

laanwj commented Sep 2, 2020 •

edited

Loading

laanwj commented Sep 3, 2020 •

edited

Loading