Page MenuHomePhabricator

p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind()
ClosedPublic

Authored by PiRK on Mar 14 2023, 10:18.

Details

Summary

p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind()

PF_NOBAN is a multi-flag that includes PF_DOWNLOAD, so the conditional
in CConnman::Bind() using a bitwise AND will return the same result
for both the "noban" status and the "download" status.

Example:

PF_DOWNLOAD is 0b1000000
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 a
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.

test: add net permissions noban/download unit test coverage

to clarify/test the relationship and NetPermissions operations
involving the NetPermissionFlags PF_NOBAN and PF_DOWNLOAD.

p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT

NetPermissions::ClearFlag() is currently only called in the codebase with
an f value of NetPermissionFlags::PF_ISIMPLICIT.

If that should change in the future, ClearFlag() should not be called
with f being a subflag of a multiflag, e.g. NetPermissionFlags::PF_RELAY
or NetPermissionFlags::PF_DOWNLOAD, as that would leave flags in an
invalid state corresponding to none of the existing NetPermissionFlags.

Therefore, allow only calling ClearFlag with the implicit flag for now.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This is a backport of core#21644

Test Plan

ninja all check-all bitcoin-fuzzers

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.Mar 14 2023, 10:18
This revision is now accepted and ready to land.Mar 14 2023, 11:09