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

1. 
   jonatack commented at 10:22 pm on July 29, 2021: member

   Resolves https://cirrus-ci.com/task/4787303177519104?logs=ci#L3020 related to #22584.

   0UndefinedBehaviorSanitizer: implicit-signed-integer-truncation
   1
   2netaddress.cpp:1190:18: runtime error: implicit conversion
   3  from type 'int' of value -1 (32-bit, signed)
   4  to type 'uint8_t' (aka 'unsigned char')
   5  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).

2. 
   tryphe commented at 10:33 pm on July 29, 2021: contributor

   untested ACK 835cc11f22f7e6f1c29394f9d71508a7722f49e6
3. 
   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).
4. 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.

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

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

   Needs a rebase to revert #22584
7. 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
8. fuzz: remove netaddress ubsan sanitizer suppression 1416a80308
9. jonatack force-pushed on Jul 30, 2021
10. 
    jonatack commented at 6:51 am on July 30, 2021: member

    Rebased, added comment and dropped the suppression.
11. 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
12. 
    GeneFerneau commented at 7:21 pm on July 30, 2021: none

    Code review ACK 1416a80
13. 
    jonatack commented at 7:23 pm on August 16, 2021: member

    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.
14. jonatack closed this on Aug 16, 2021

    ---

15. jonatack deleted the branch on Aug 16, 2021
16. 
    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.
17. 
    MarcoFalke commented at 3:42 pm on November 19, 2021: member

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

Contributors
jonatack tryphe MarcoFalke GeneFerneau

Labels
P2P