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

pull jonatack wants to merge 3 commits into bitcoin:master from jonatack:NetPermissionFlags-noban-bugfix changing 4 files +36 −5
  1. jonatack commented at 7:48 pm on April 9, 2021: member

    This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected.

    Since #19191, noban is a multi-flag that implies download, so the conditional in CConnman::Bind() using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.

    The second commit adds unit test coverage to illustrate and test the noban/download relationship and the NetPermissions operations involving them.

    The final commit adds documentation and disallows calling NetPermissions::ClearFlag() with any second param other than NetPermissionFlags “implicit” – per current usage in the codebase – because ClearFlag() should not be called with any second param that is a subflag of a multiflag, e.g. “relay” or “download,” as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.

  2. DrahtBot added the label P2P on Apr 9, 2021
  3. DrahtBot commented at 9:13 pm on April 9, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21506 (p2p, refactor: make NetPermissionFlags an enum class by jonatack)

    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.

  4. jonatack renamed this:
    net: use NetPermissions::HasFlag() in CConnman::Bind()
    p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind()
    on Apr 11, 2021
  5. in src/net.cpp:2421 in 693af714ea outdated 
    2417@@ -2418,7 +2418,7 @@ bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags
2418         return false;
2419     }
2420 
2421-    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !(permissions & PF_NOBAN)) {
2422+    if (addr.IsRoutable() && fDiscover && !(flags & BF_DONT_ADVERTISE) && !NetPermissions::HasFlag(permissions, PF_NOBAN)) {
    vasild commented at 6:46 am on April 14, 2021:
    s/PF_NOBAN/NetPermissionsFlags::PF_NOBAN/?
    jonatack commented at 2:17 pm on April 14, 2021:
    sure, it’s done in https://github.com/bitcoin/bitcoin/pull/21506/commits/873bbe6e46b8561acea365cb0fffa226fd891826#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R2413 but may as well update it here since I’m retouching both pulls
  6. in src/test/netbase_tests.cpp:399 in 693af714ea outdated 
    394@@ -395,6 +395,51 @@ BOOST_AUTO_TEST_CASE(netpermissions_test)
395     BOOST_CHECK(NetWhitebindPermissions::TryParse("@1.2.3.4:32", whitebindPermissions, error));
396     BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NONE);
397 
398+    // "noban" implies "download"
399+    BOOST_CHECK(NetWhitebindPermissions::TryParse("noban@1.2.3.4:32", whitebindPermissions, error));
    vasild commented at 6:49 am on April 14, 2021:
    nit: maybe BOOST_REQUIRE() is more appropriate here - if TryParse() fails, then the checks below are meaningless.
    jonatack commented at 2:17 pm on April 14, 2021:
    done
  7. in src/test/netbase_tests.cpp:406 in 693af714ea outdated 
    401+    BOOST_CHECK(whitebindPermissions.m_flags & PF_DOWNLOAD);
402+    BOOST_CHECK(whitebindPermissions.m_flags & PF_NOBAN);
403+    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_DOWNLOAD));
404+    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_NOBAN));
405+
406+    NetPermissions::ClearFlag(whitebindPermissions.m_flags, PF_DOWNLOAD);
    vasild commented at 7:16 am on April 14, 2021:

    out of scope of this PR: this ClearFlag() will leave the flags in an invalid state 0b10000 that does not equal any of the PF_* flags or a combination of them. This shouldn’t be even possible/allowed.

    It is like having some fruits in a bag - apple, orange, pear and when you remove the apple, then the bag does not contain fruits anymore, it now contains a frog :frog:.

    jonatack commented at 3:28 pm on April 14, 2021:
    Good point. Added an assert and doxygen doc explanation to ClearFlags to allow only clearing the implicit flag, as it’s the only current use.
  8. in src/test/netbase_tests.cpp:426 in 693af714ea outdated 
    421+    BOOST_CHECK(NetWhitebindPermissions::TryParse("download,noban@1.2.3.4:32", whitebindPermissions, error));
422+    BOOST_CHECK_EQUAL(whitebindPermissions.m_flags, PF_NOBAN);
423+    BOOST_CHECK(whitebindPermissions.m_flags & PF_DOWNLOAD);
424+    BOOST_CHECK(whitebindPermissions.m_flags & PF_NOBAN);
425+    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_DOWNLOAD));
426+    BOOST_CHECK(NetPermissions::HasFlag(whitebindPermissions.m_flags, PF_NOBAN));
    vasild commented at 7:25 am on April 14, 2021:

    This is repeated 3 times: for noban, noban,download and download,noban. Could avoid the repetitions with something like:

     0NetWhitebindPermissions noban;
 1NetWhitebindPermissions noban_download;
 2NetWhitebindPermissions download_noban;
 3
 4TryParse("noban@...", noban);
 5...all 6 checks...
 6
 7TryParse("noban,download@...", noban_download);
 8BOOST_CHECK_EQUAL(noban_download, noban); // just this instead of the 6 repeated checks
 9
10TryParse("download,noban@...", download_noban);
11BOOST_CHECK_EQUAL(download_noban, noban); // just this instead of the 6 repeated checks
    jonatack commented at 2:20 pm on April 14, 2021:
    done
  9. vasild approved
  10. vasild commented at 7:39 am on April 14, 2021: member

    ACK 693af714ea264cd5edeadecaa0e7762cfe377e5a

    Some non-blocker comments below.

  11. vasild commented at 7:45 am on April 14, 2021: member

    …If the intent is for this conditional to return false only for noban…

    I think it is - the reasoning must be that we do not want to advertise our listen address if it has noban.

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

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    dde69f20a0
  13. jonatack force-pushed on Apr 14, 2021
  14. jonatack commented at 4:26 pm on April 14, 2021: member
    Thanks for the excellent review, @vasild. I added your commit description example from #21668, incorporated your suggestions, followed up on your #21644 (review) with the last commit, and updated all the commit messages and the PR description.
  15. in src/net_permissions.h:61 in acef9629a3 outdated 
    56+    //! be called with subflags of multiflags, e.g. NetPermissionFlags::PF_RELAY
57+    //! or NetPermissionFlags::PF_DOWNLOAD, as that would leave the result in an
58+    //! invalid state corresponding to none of the existing flags.
59     static inline void ClearFlag(NetPermissionFlags& flags, NetPermissionFlags f)
60     {
61+        assert(f == NetPermissionFlags::PF_ISIMPLICIT);
    vasild commented at 4:46 pm on April 14, 2021:

    During fuzzing ClearFlag() is called with a fuzzed/random argument:

    https://github.com/bitcoin/bitcoin/blob/773f8c1a7d568012768e16d2ede65b5d4d62aced/src/test/fuzz/net_permissions.cpp#L28

    https://github.com/bitcoin/bitcoin/blob/773f8c1a7d568012768e16d2ede65b5d4d62aced/src/test/fuzz/net_permissions.cpp#L38

    I guess this means that eventually this newly added assert will be triggered? I think either remove the assert or remove the ClearFlag() calls from the fuzz tests.

    jonatack commented at 5:07 pm on April 14, 2021:
    Ah, didn’t check the fuzzer. Thanks!
    jonatack commented at 6:46 pm on April 14, 2021:
    Fixed.
  16. jonatack force-pushed on Apr 14, 2021
  17. jonatack force-pushed on Apr 14, 2021
  18. vasild approved
  19. vasild commented at 5:34 am on April 15, 2021: member
    ACK 2d34c4874775f9e1fc7e8f02a9817a6898461abd
  20. theStack approved
  21. theStack commented at 2:04 pm on April 18, 2021: member

    ACK 2d34c4874775f9e1fc7e8f02a9817a6898461abd

    Thanks for fixing!

    nit: It’s generally better to have more tests rather than less, but personally I don’t see much gain in also repeatedly testing the flags with bitmasks, e.g. this part

    0    BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::PF_DOWNLOAD);
1    BOOST_CHECK(download.m_flags != NetPermissionFlags::PF_NOBAN);
2    BOOST_CHECK(download.m_flags & NetPermissionFlags::PF_DOWNLOAD);
3    BOOST_CHECK(download.m_flags & NetPermissionFlags::PF_NOBAN);

    is semantically equivalent to

    0    BOOST_CHECK_EQUAL(download.m_flags, NetPermissionFlags::PF_DOWNLOAD);
1    BOOST_CHECK(NetPermissionFlags::PF_DOWNLOAD != NetPermissionsFlags::PF_NOBAN);
2    BOOST_CHECK(NetPermissionFlags::PF_DOWNLOAD & NetPermissionsFlags::PF_DOWNLOAD); // tautology?
3    BOOST_CHECK(NetPermissionFlags::PF_DOWNLOAD & NetPermissionsFlags::PF_NOBAN);

    i.e. everything after the first line doesn’t test the result related to the previous tryParse-call, but only what bits the PF_ enums consist of internally, which could potentially be confusing to readers of this test at this place.

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

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    4e0d5788ba
  23. 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>
    36fb036d25
  24. jonatack force-pushed on Apr 18, 2021
  25. jonatack commented at 2:41 pm on April 18, 2021: member
    @theStack Thanks for having a look! Dropped the bitwise AND test assertions; they would be removed in #21506 anyway so we may as well not add them.
  26. theStack approved
  27. theStack commented at 2:46 pm on April 18, 2021: member
    re-ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8 ☕
  28. vasild approved
  29. vasild commented at 2:41 pm on April 19, 2021: member
    ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8
  30. hebasto commented at 2:40 pm on April 29, 2021: member

    Approach ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8.

    This PR clearly shows how fragile “multibit flags” are. We definitely shouldn’t introduce new ones.

    From the second “test: add net permissions noban/download unit test coverage” commit I expected that it will fail without the first one. Is it possible to improve tests to prevent regressions in the future?

  31. jonatack commented at 5:20 pm on April 29, 2021: member
    @hebasto Thanks. Follow-up #21506 prevents this class of bugs.
  32. hebasto approved
  33. hebasto commented at 5:35 pm on April 29, 2021: member
    ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8, I have reviewed the code and it looks OK, I agree it can be merged.
  34. jonatack commented at 10:16 am on May 11, 2021: member
    Thanks @vasild, @theStack, and @hebasto for the reviews and ACKs but this bugfix isn’t being merged and I am cleaning up some loose ends before being less present here for some seminars. Up for grabs.
  35. jonatack closed this on May 11, 2021
  36. kallewoof commented at 10:45 am on May 11, 2021: member

    Code review ACK 36fb036d25e2a3016b36873456e5a9e6251ffef8

    Seems like ClearFlag could be renamed ClearImplicitFlag and remove 2nd argument instead of the assert stuff, but that would make the diff bigger, so no big deal.

  37. jonatack commented at 10:47 am on May 11, 2021: member
    @kallewoof agree, I thought about removing the argument too, could go one way or the other.
  38. jonatack reopened this on May 11, 2021
  39. laanwj merged this on May 11, 2021
  40. laanwj closed this on May 11, 2021
  41. jonatack deleted the branch on May 11, 2021
  42. sidhujag referenced this in commit beef02c60d on May 11, 2021
  43. in src/net.cpp:2411 in 36fb036d25 
    2407@@ -2408,8 +2408,9 @@ NodeId CConnman::GetNewNodeId()
2408 
2409 
2410 bool CConnman::Bind(const CService &addr, unsigned int flags, NetPermissionFlags permissions) {
2411-    if (!(flags & BF_EXPLICIT) && !IsReachable(addr))
2412+    if (!(flags & BF_EXPLICIT) && !IsReachable(addr)) {
    rebroad commented at 5:38 pm on May 15, 2021:
    why the brackets?
    jonatack commented at 5:45 pm on May 15, 2021:

    why the brackets?

    doc/developer-notes.md:

    0  - If an `if` only has a single-statement `then`-clause, it can appear
1    on the same line as the `if`, without braces. In every other case,
2    braces are required, and the `then` and `else` clauses must appear
3    correctly indented on a new line.

    (there was a famous bug caused by this, I don’t recall the link)

    practicalswift commented at 5:54 pm on May 16, 2021:

    (there was a famous bug caused by this, I don’t recall the link)

    CVE-2014-1266: the Apple “goto fail” vulnerability :)

    vasild commented at 4:19 pm on May 17, 2021:

    In addition to bugs, maintenance/code review is easier if braces are always present. Consider this:

    0 if ((some long expression && what is this == 123) || oh no) {
1     statement 1;
2+    statement 2;
3 }

    vs

    0- if ((some long expression && what is this == 123) || oh no)
1+ if ((some long expression && what is this == 123) || oh no) {
2     statement 1;
3+    statement 2;
4+ }
  44. laanwj referenced this in commit 4da26fb85d on May 19, 2021
  45. MarcoFalke added the label Needs backport (0.19) on May 22, 2021
  46. MarcoFalke added the label Needs backport (0.20) on May 22, 2021
  47. MarcoFalke added the label Needs backport (0.21) on May 22, 2021
  48. MarcoFalke removed the label Needs backport (0.19) on May 22, 2021
  49. MarcoFalke removed the label Needs backport (0.20) on May 22, 2021
  50. MarcoFalke commented at 7:54 am on May 22, 2021: member
    Only backport to 0.21 needed
  51. MarcoFalke referenced this in commit f2a88986a1 on May 22, 2021
  52. MarcoFalke commented at 8:01 am on May 22, 2021: member
    Backported in #22022
  53. MarcoFalke removed the label Needs backport (0.21) on May 22, 2021
  54. luke-jr referenced this in commit cc48bf0a77 on Jun 27, 2021
  55. gwillen referenced this in commit edd579466f on Jun 1, 2022
  56. DrahtBot locked this on Aug 18, 2022


jonatack DrahtBot vasild theStack hebasto kallewoof rebroad practicalswift MarcoFalke

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-05 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me