Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Jul 29, 2021

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

@tryphe
Copy link
Contributor

tryphe commented Jul 29, 2021

untested ACK 835cc11

@tryphe
Copy link
Contributor

tryphe commented Jul 29, 2021

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

@DrahtBot DrahtBot added the P2P label Jul 30, 2021
@maflcko
Copy link
Member

maflcko commented Jul 30, 2021

Needs a rebase to revert #22584

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)
@jonatack jonatack force-pushed the fix-netaddress-implicit-signed-integer-truncation branch from 835cc11 to 1416a80 Compare July 30, 2021 06:50
@jonatack
Copy link
Member Author

Rebased, added comment and dropped the suppression.

@jonatack jonatack changed the title p2p, refactor: fix ubsan implicit conversion error in CSubNet::ToString() p2p: fix ubsan implicit conversion error in CSubNet::ToString() Jul 30, 2021
@GeneFerneau
Copy link

Code review ACK 1416a80

@jonatack jonatack mentioned this pull request Aug 6, 2021
@jonatack
Copy link
Member Author

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 jonatack closed this Aug 16, 2021
@jonatack jonatack deleted the fix-netaddress-implicit-signed-integer-truncation branch August 16, 2021 19:25
@maflcko
Copy link
Member

maflcko commented Aug 17, 2021

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.

@maflcko
Copy link
Member

maflcko commented Nov 19, 2021

Sorry for the wrong comment, I already fixed this in commit efd6f90.

@bitcoin bitcoin locked and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants