net processing: clamp PeerManager::Options user input #28149

pull stickies-v wants to merge 3 commits into bitcoin:master from stickies-v:2023-07/peerman-opts-check-bounds changing 2 files +16 −7
  1. stickies-v commented at 2:59 pm on July 25, 2023: contributor

    Avoid out-of-bounds user input for PeerManager::Options by safely clamping -maxorphantx and -blockreconstructionextratxn, and avoid platform-specific behaviour by changing PeerManager::Options::max_extra_txs from size_t to a uint32_t. Addresses #27499#pullrequestreview-1544114932.

    Also documents all PeerManager::Options members, addressing #27499 (review).

  2. DrahtBot commented at 2:59 pm on July 25, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK dergoegge, glozow

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label P2P on Jul 25, 2023
  4. stickies-v renamed this:
    Net processing: clamp PeerManager::Options user input
    net processing: clamp PeerManager::Options user input
    on Jul 25, 2023
  5. in src/net_processing.h:55 in 128ad03792 outdated
    52+        //! Whether transaction reconciliation protocol is enabled
    53         bool reconcile_txs{DEFAULT_TXRECONCILIATION_ENABLE};
    54+        //! Maximum number of orphan transactions kept in memory
    55         uint32_t max_orphan_txs{DEFAULT_MAX_ORPHAN_TRANSACTIONS};
    56-        size_t max_extra_txs{DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN};
    57+        //! Number of orphan+recently-replaced transactions to keep around for block reconstruction
    


    glozow commented at 4:42 pm on July 25, 2023:

    nit, includes a bit more than replaced and orphans

    0        //! Number of non-mempool transactions to keep around for block reconstruction. Includes orphan, replaced, and rejected transactions.
    

    stickies-v commented at 8:54 pm on July 25, 2023:
    Ah thanks, fixed in aa89e04e07ca9ff51b1d7d310a11821c6ad963cf
  6. glozow commented at 4:45 pm on July 25, 2023: member

    utACK 128ad03792cd4aeeaf32807d07f01e3f85adaf28

    Thanks for the followup

  7. in src/node/peerman_args.cpp:20 in 128ad03792 outdated
    17+        options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
    18     }
    19 
    20     if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
    21-        options.max_extra_txs = size_t(std::max(int64_t{0}, *value));
    22+        options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
    


    dergoegge commented at 8:01 pm on July 25, 2023:
    Wondering if we can come up with a more sane upper limit for both of these?

    stickies-v commented at 9:17 pm on July 25, 2023:
    Given that we use this value to size a vector, that could be sensible, but of course, arbitrary upper limits bring their own set of problems. I’ll look into it more, thanks!

    stickies-v commented at 3:01 pm on July 26, 2023:

    I’m not really keen on setting limits. I don’t know what good values would be, and ultimately it’s not a huge problem if the value is too high. I think issuing a warning could be sensible as it does have meaningful impact, but I don’t have a strong view. Too many warnings/popups is also not great UX.

    For example, this would log a warning in the console, and on qt issue a warning pop-up if a value > 100x default value is entered:

     0diff --git a/src/node/peerman_args.cpp b/src/node/peerman_args.cpp
     1index efe4514271..53034d6277 100644
     2--- a/src/node/peerman_args.cpp
     3+++ b/src/node/peerman_args.cpp
     4@@ -2,6 +2,8 @@
     5 
     6 #include <common/args.h>
     7 #include <net_processing.h>
     8+#include <node/interface_ui.h>
     9+#include <util/translation.h>
    10 
    11 #include <algorithm>
    12 #include <limits>
    13@@ -13,10 +15,16 @@ void ApplyArgsManOptions(const ArgsManager& argsman, PeerManager::Options& optio
    14     if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
    15 
    16     if (auto value{argsman.GetIntArg("-maxorphantx")}) {
    17+        if (*value > (100 * options.max_orphan_txs)) {
    18+            InitWarning(_("-maxorphantx is > 100x larger than the default value, affecting memory consumption"));
    19+        }
    20         options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
    21     }
    22 
    23     if (auto value{argsman.GetIntArg("-blockreconstructionextratxn")}) {
    24+        if (*value > (100 * options.max_extra_txs)) {
    25+            InitWarning(_("-blockreconstructionextratxn is > 100x larger than the default value, affecting memory consumption"));
    26+        }
    27         options.max_extra_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
    28     }
    29 
    

    What do you think?


    dergoegge commented at 1:34 pm on August 7, 2023:
    Let’s just leave it as is, was more of a nit anyway
  8. dergoegge commented at 8:03 pm on July 25, 2023: member
    Concept ACK
  9. doc: document PeerManager::Options members aa89e04e07
  10. net processing: clamp -maxorphantx to uint32_t bounds e451d1e3c6
  11. net processing: clamp -blockreconstructionextratxn to uint32_t bounds
    Also changes max_extra_txs into a uint32_t to avoid platform-specific
    behaviour
    547fa52443
  12. stickies-v force-pushed on Jul 25, 2023
  13. dergoegge approved
  14. dergoegge commented at 1:35 pm on August 7, 2023: member
    Code review ACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b
  15. DrahtBot requested review from glozow on Aug 7, 2023
  16. in src/node/peerman_args.cpp:16 in 547fa52443
    12 {
    13     if (auto value{argsman.GetBoolArg("-txreconciliation")}) options.reconcile_txs = *value;
    14 
    15     if (auto value{argsman.GetIntArg("-maxorphantx")}) {
    16-        options.max_orphan_txs = uint32_t(std::max(int64_t{0}, *value));
    17+        options.max_orphan_txs = uint32_t((std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max())));
    


    maflcko commented at 2:52 pm on August 7, 2023:

    unrelated: May be good to write a clang-tidy plugin to enforce the limits are compile-time constants and in range to avoid silent UB at runtime?

    The in-range one can be submitted to upstream and the other check can be done in this repo.


    maflcko commented at 2:55 pm on August 7, 2023:
    0        options.max_orphan_txs = uint32_t(std::clamp<int64_t>(*value, 0, std::numeric_limits<uint32_t>::max()));
    

    nit, if you re-touch?

  17. glozow commented at 12:22 pm on August 9, 2023: member
    reACK 547fa52443cbb5e8ccfee993486f5ced8cdbb33b
  18. glozow merged this on Aug 9, 2023
  19. glozow closed this on Aug 9, 2023

  20. stickies-v deleted the branch on Aug 9, 2023
  21. sidhujag referenced this in commit ce8b3ee3ea on Aug 9, 2023
  22. Fabcien referenced this in commit 83352fab43 on Jul 12, 2024
  23. bitcoin locked this on Aug 8, 2024

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-12-11 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me