p2p: fix ubsan implicit conversion error in CSubNet::ToString()#22586
Closed
jonatack wants to merge 2 commits intobitcoin:masterfrom
Closed
p2p: fix ubsan implicit conversion error in CSubNet::ToString()#22586jonatack wants to merge 2 commits intobitcoin:masterfrom
jonatack wants to merge 2 commits intobitcoin:masterfrom
Conversation
Contributor
|
untested ACK 835cc11f22f7e6f1c29394f9d71508a7722f49e6 |
Contributor
|
I think it would be helpful to add a comment on why the conversion/check is taking place (as described in op). |
jonatack
commented
Jul 29, 2021
Member
|
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
force-pushed
the
fix-netaddress-implicit-signed-integer-truncation
branch
from
835cc11 to
1416a80
Compare
Member
Author
|
Rebased, added comment and dropped the suppression. |
jonatack
changed the title
|
Code review ACK 1416a80 |
Merged
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. |
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. |
Member
|
Sorry for the wrong comment, I already fixed this in commit efd6f90. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves https://cirrus-ci.com/task/4787303177519104?logs=ci#L3020 related to #22584.
NetmaskBits()returns -1 if the subnet mask is invalid, which can be incompatible withuint8_t cidrand should probably be exited from.This allows removing the netaddress UBSan suppression (implicit-signed-integer-truncation).