net: properly handle PF_NOBAN in CConnman::Bind() #21668
Conversation
`PF_DOWNLOAD` is `0b1000000` and `PF_NOBAN` is `0b1010000`. This makes a check like `flags & PF_NOBAN` return `true` even if `flags` is equal to `PF_DOWNLOAD`. If `-whitebind=download@1.1.1.1:8765` is specified, then `1.1.1.1:8765` should be added to the list of local addresses. We only want to avoid adding to local addresses (that are advertised) a whitebind that has `noban@` flag. As a result of a mis-check in `CConnman::Bind()` we would not have added `1.1.1.1:8765` to the local addresses in the example above.
|
Similar issue spotted in #20196 (comment) by @jonatack. I checked that what's fixed in this PR is the only other case. |
|
Proposed in #21644. |
|
(#21506 would eliminate this class of NetPermissionFlags bug as well, as only defined operations would compile.) |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Closing in favor of #21644. |
| } | ||
|
|
||
| if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !(permissions & PF_NOBAN)) { | ||
| const bool noban{NetPermissions::HasFlag(static_cast<NetPermissionFlags>(flags), PF_NOBAN)}; |
There was a problem hiding this comment.
I stared at this for a minute to figure out why the cast added. Then I realized it's because this line uses flags instead of permissions. The names are certainly similar enough to be confusing.
There was a problem hiding this comment.
Right, it is using the wrong variable 🤦
There was a problem hiding this comment.
Would be nice if there was a test that failed with this diff
This makes a check like
flags & PF_NOBANreturntrueeven ifflagsis equal to
PF_DOWNLOAD.If
-whitebind=download@1.1.1.1:8765is specified, then1.1.1.1:8765should be added to the list of local addresses. We only want to avoid
adding to local addresses (that are advertised) a whitebind that has
noban@flag.As a result of a mis-check in
CConnman::Bind()we would not have added1.1.1.1:8765to the local addresses in the example above.