p2p: fix ubsan implicit conversion error in CSubNet::ToString() #22586

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:fix-netaddress-implicit-signed-integer-truncation changing 2 files +6 −2

jonatack commented at 10:22 PM on July 29, 2021: contributor
Resolves https://cirrus-ci.com/task/4787303177519104?logs=ci#L3020 related to #22584.
```
UndefinedBehaviorSanitizer: implicit-signed-integer-truncation

netaddress.cpp:1190:18: runtime error: implicit conversion
  from type 'int' of value -1 (32-bit, signed)
  to type 'uint8_t' (aka 'unsigned char')
  changed the value to 255 (8-bit, unsigned)
```
NetmaskBits() returns -1 if the subnet mask is invalid, which can be incompatible with uint8_t cidr and should probably be exited from.

This allows removing the netaddress UBSan suppression (implicit-signed-integer-truncation).
jonatack cross-referenced this on Jul 29, 2021 from issue test: Add temporary sanitizer suppression implicit-signed-integer-truncation:netaddress.cpp by MarcoFalke
tryphe commented at 10:33 PM on July 29, 2021: contributor

untested ACK 835cc11f22f7e6f1c29394f9d71508a7722f49e6
tryphe commented at 10:35 PM on July 29, 2021: contributor

I think it would be helpful to add a comment on why the conversion/check is taking place (as described in op).

in src/netaddress.cpp:1191 in 835cc11f22 outdated

1186 | @@ -1187,7 +1187,11 @@ std::string CSubNet::ToString() const
1187 |              if (netmask[i] == 0x00) {
1188 |                  break;
1189 |              }
1190 | -            cidr += NetmaskBits(netmask[i]);
1191 | +            const int num_bits{NetmaskBits(netmask[i])};
1192 | +            if (num_bits == -1) {

jonatack commented at 10:48 PM on July 29, 2021:

Will update per #22586 (comment) (thanks @tryphe!) if CI is green.

            if (num_bits == -1) {
                // Invalid subnet mask

DrahtBot added the label P2P on Jul 30, 2021
MarcoFalke commented at 6:31 AM on July 30, 2021: member

Needs a rebase to revert #22584

p2p: fix ubsan implicit conversion in CSubNet::ToString()

netaddress.cpp:1190:18: runtime error: implicit conversion
  from type 'int' of value -1 (32-bit, signed)
  to type 'uint8_t' (aka 'unsigned char')
  changed the value to 255 (8-bit, unsigned)

52492653b4

fuzz: remove netaddress ubsan sanitizer suppression 1416a80308
jonatack force-pushed on Jul 30, 2021
jonatack commented at 6:51 AM on July 30, 2021: contributor

Rebased, added comment and dropped the suppression.
jonatack renamed this:
~~p2p, refactor: fix ubsan implicit conversion error in CSubNet::ToString()~~
p2p: fix ubsan implicit conversion error in CSubNet::ToString()
on Jul 30, 2021
GeneFerneau commented at 7:21 PM on July 30, 2021: none

Code review ACK 1416a80
jonatack cross-referenced this on Aug 6, 2021 from issue [22.x] rc3 backports by hebasto
jonatack commented at 7:23 PM on August 16, 2021: contributor

This fix was opened the same day as the suppression, which was merged while the fix was not. It has two ACKs from the first day or two and I suppose if it was going to be merged, it would have been done so by now.
jonatack closed this on Aug 16, 2021
jonatack deleted the branch on Aug 16, 2021
MarcoFalke commented at 5:52 AM on August 17, 2021: member

I think that a fix for this is still needed. Though, I am not too familiar with this code, so I don't feel comfortable to review or merge this myself.
jonatack cross-referenced this on Oct 5, 2021 from issue p2p: fix CSubNet::ToString() UBSan and banman fuzz crash by jonatack
MarcoFalke commented at 3:42 PM on November 19, 2021: member

Sorry for the wrong comment, I already fixed this in commit efd6f904c78769ad2e93c1f1de43014d284e7561.
bitcoin locked this on Nov 19, 2022

Contributors

Labels

Linked (view graph)

#22584 test: Add temporary sanitizer suppression implicit-signed-integer-truncation:netaddress.cpp #22629 [22.x] rc3 backports #23197 p2p: fix CSubNet::ToString() UBSan and banman fuzz crash