p2p: change `CInv::type` from `int` to `uint32

jonatack commented at 9:11 am on August 27, 2020: member

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#pullrequestreview-455788826.

Closes #19678.

p2p: change CInv::type from int to uint32_t

fixes issue #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

407175e0c2

in src/protocol.h:428 in 407175e0c2 outdated

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

laanwj commented at 9:17 am on August 27, 2020:

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.

fanquake added the label P2P on Aug 27, 2020

MarcoFalke commented at 9:31 am on August 27, 2020: member

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 force-pushed on Aug 27, 2020

jonatack commented at 2:13 pm on August 27, 2020: member

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.

in test/functional/p2p_invalid_messages.py:136 in 9e05ed27fd outdated

127@@ -125,6 +128,16 @@ def test_msgtype(self):
128             conn.sync_with_ping(timeout=1)
129         self.nodes[0].disconnect_p2ps()
130 
131+    def test_inv_msg_with_negative_type(self):
132+        self.log.info("Test inv with negative type raises argument out of range error")
133+        invalid_inv = msg_inv([CInv(t=-1, h=1)])  # inv type must be an unsigned integer
134+        try:
135+            self.nodes[0].add_p2p_connection(P2PInterface()).send_message(invalid_inv)
136+        except struct.error as e:

MarcoFalke commented at 2:29 pm on August 27, 2020:

We generally don’t test the test framework. Sending an uint32_t{-1} and observing the node doesn’t crash with sanitizers enabled should be a sufficient smoke test. Maybe add an assert_debug_log to observe how the inv will be formatted.

0            LogPrint(BCLog::NET, "got inv: %s  %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom.GetId());

jonatack commented at 5:54 pm on August 27, 2020:

Thanks Marco. Trying some stuff to see how the CI reacts to it.

jonatack force-pushed on Aug 27, 2020

in test/functional/p2p_invalid_messages.py:144 in 6c0f4aefe3 outdated

139+        else:
140+            raise AssertionError("No exception raised")
141+
142+        # Repeat, passing signed integer CInv type format to override default.
143+        LE_signed_int = "<i"  # Python format chars for little-endian signed int
144+        inv_with_signed_type = msg_inv([CInv(t=-MSG_TX, h=1, f=LE_signed_int)])

MarcoFalke commented at 5:59 pm on August 27, 2020:

0        inv_with_signed_type = msg_inv([CInv(t=0xffffffff, h=1)])

Serialized -1 as signed integer is identical to 0xffffffff serialized as unsigned integer, so might as well use the latter

jonatack commented at 6:14 pm on August 27, 2020:

That was breaking this method in messages.py::CInv with KeyError: 4294967295 from calling CInv::typemap[4294967295]

0    def __repr__(self):
1        return "CInv(type=%s hash=%064x)" \
2            % (self.typemap[abs(self.type)], self.hash)

MarcoFalke commented at 6:22 pm on August 27, 2020:

Even when adding type 0xffffffff: "Error"?

jonatack commented at 7:07 pm on August 27, 2020:

Done, simplified, good idea. LMK if we should drop the first test.

practicalswift commented at 6:18 pm on August 27, 2020: contributor

Concept ACK

Thanks for fixing UBSan fuzzing finds!

jonatack force-pushed on Aug 27, 2020

practicalswift commented at 5:48 am on August 28, 2020: contributor

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

in test/functional/p2p_invalid_messages.py:136 in 1bb9a9221d outdated

127@@ -125,6 +128,22 @@ def test_msgtype(self):
128             conn.sync_with_ping(timeout=1)
129         self.nodes[0].disconnect_p2ps()
130 
131+    def test_inv_msg_with_signed_integer_type(self):
132+        self.log.info("Test inv message with MSG_TX type as signed int raises out of range error")
133+        inv_with_signed_type = msg_inv([CInv(t=-MSG_TX, h=1)])
134+        try:
135+            self.nodes[0].add_p2p_connection(P2PInterface()).build_message(inv_with_signed_type)
136+        except struct.error as e:

MarcoFalke commented at 5:50 am on August 28, 2020:

imo could drop the test framework self-test

jonatack commented at 8:15 am on August 28, 2020:

Done, but I think both of these tests only tested the framework change anyway.

jonatack commented at 7:53 am on August 28, 2020: member

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

jonatack force-pushed on Aug 28, 2020

jnewbery commented at 11:38 am on August 28, 2020: member

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: https://github.com/bitcoin-dot-org/Bitcoin.org/commit/1634212dd5550bb9b610341668b4b7b5fa3690a5#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 commented at 11:54 am on August 28, 2020: member

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

in test/functional/test_framework/messages.py:268 in 7bfc631a11 outdated

268         return r
269 
270     def __repr__(self):
271         return "CInv(type=%s hash=%064x)" \
272-            % (self.typemap[self.type], self.hash)
273+            % (self.typemap[abs(self.type)], self.hash)

laanwj commented at 12:36 pm on August 28, 2020:

I don’t think this abs(…) is correct. There should be no reason to take an absolute value here, convert, say, -1 to 1.

maybe

0self.typemap.get(self.type, "Error")

jonatack commented at 1:36 pm on August 28, 2020:

Thanks for reviewing. The thought was to not allow shallow errors of type KeyError to hide deeper issues, like argument out of datatype range.

jonatack commented at 6:14 pm on August 28, 2020:

Dropped this line.

test framework: serialize/deserialize inv type as unsigned int 7984c39be1

in test/functional/test_framework/messages.py:249 in 7bfc631a11 outdated

243@@ -244,27 +244,28 @@ class CInv:
244         MSG_TX | MSG_WITNESS_FLAG: "WitnessTx",
245         MSG_BLOCK | MSG_WITNESS_FLAG: "WitnessBlock",
246         MSG_FILTERED_BLOCK: "filtered Block",
247-        4: "CompactBlock",
248-        5: "WTX",
249+        MSG_CMPCT_BLOCK: "CompactBlock",
250+        MSG_WTX: "WTX",
251+        0xffffffff: "Error",

laanwj commented at 12:37 pm on August 28, 2020:

I’d prefer to report anything that is unkonwn as “Error” instead of specifically adding 0xffffffff here.

jonatack commented at 1:44 pm on August 28, 2020:

I’m not sure what to do here. This was only needed for the test. I tried a few different tests; the first one doesn’t require adding entries to typemap. I don’t think any of the tests cover the protocol.{h,cpp} CInv::type type change, only the test framework change.

jonatack commented at 6:14 pm on August 28, 2020:

Dropped this line.

jonatack force-pushed on Aug 28, 2020

jonatack commented at 6:19 pm on August 28, 2020: member

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 commented at 12:51 pm on August 29, 2020: member

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

vasild approved

vasild commented at 7:21 pm on September 1, 2020: member

ACK 7984c39be

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:

https://github.com/bitcoin/bitcoin/blob/bab4cce1b0eedc1a51692aaf83ba54dd0a9d17e6/src/net_processing.cpp#L2717

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 commented at 2:17 pm on September 2, 2020: member

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 commented at 11:29 am on September 3, 2020: member

ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e

MarcoFalke commented at 2:42 pm on September 3, 2020: member

ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e 🌻

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7984c39be11ca04460883365e1ae2a496aaa6c0e 🌻
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjtLAwAtfwkLzUyVHkPG+CsHcnD4C7PQgsfTFBMxOM9trXOHY299iYHJehs2jwr
 8j748j23g+pETizxSTTPal7FmbYGvmVrITJiLVfLtMeW+9vOoP59qzb1/mvxEWFWw
 98hQnBJtUxYH1t8DxdWcVLew4jTlGyds2E6hGb09htsPam9LAxW+eUDynlwFXQgE1
10HNxmk8K2mMLYoHeLidculEx2UcQFIH7GqFWDplFDpzSAruFneWqyg+V2LUqhCTpH
11Jq/YFXupkfmHrySzWkJz3Ix2mGslNaxtqw18KnFFIc8Kx1qzZKOvloWIZCjvnCQt
12GbWMZfSvvf1ALu44WX+dZLl6NtGU8x18bS0y6tkKrPyW51XyouCn7J3aOk03IcXn
133LizPJLA5DFf9jCILzLwoQLRhZbLAVsSoWIBVIyIIuoOrsbf2omAoxe13HoepQzK
14a02g086odTrNRvKdwARXzC6tLEdgQHW4mJbZJLDCJ1Gz+qXJOla2tCIrieZq0Y8T
15vC5hwFiZ
16=81Qa
17-----END PGP SIGNATURE-----

Timestamp of file with hash a72c948f27f3e6c7aa323c708d61eff780f8ac68076405b22c540f1cb6199d2f -

laanwj merged this on Sep 3, 2020

laanwj closed this on Sep 3, 2020

jonatack deleted the branch on Sep 3, 2020

sidhujag referenced this in commit addddc0053 on Sep 3, 2020

DrahtBot locked this on 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