Merge #15401: rpc: Actually throw help when passed invalid number of params

fa4ce7038d444defe0b98a30097174c278054a33 rpc: Actually throw help when passed invalid number of params (MarcoFalke)
fa05626ca7a0fe896ac554c79eaea4c36acdf861 rpc: Add RPCHelpMan::IsValidNumArgs() (MarcoFalke)

Pull request description:

  Can be tested by

  * running the included test against an old binary (compiled without this patch)
  * calling `setban 1 "add" 3 4 5 6 7 8 9 0` in the gui

Tree-SHA512: aa6a25bbe6f40722913ea292252a62a4012c964eed9f4035335a2e2d13be98eb60f368e8a3251a104a26a62c08b2cb926b06e5ab1418ef1cf4abdd71d87c2919

5 files changed, 46 insertions, 22 deletions

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 7fb9ff2eaf..c73a8d48cf 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1,5 +1,5 @@
 // Copyright (c) 2010 Satoshi Nakamoto
-// Copyright (c) 2009-2018 The Bitcoin Core developers
+// Copyright (c) 2009-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -1778,9 +1778,7 @@ static constexpr size_t PER_UTXO_OVERHEAD = sizeof(COutPoint) + sizeof(uint32_t)
 
 static UniValue getblockstats(const JSONRPCRequest& request)
 {
-    if (request.fHelp || request.params.size() < 1 || request.params.size() > 4) {
-        throw std::runtime_error(
-            RPCHelpMan{"getblockstats",
+    const RPCHelpMan help{"getblockstats",
                 "\nCompute per block statistics for a given window. All amounts are in satoshis.\n"
                 "It won't work for some heights with pruning.\n"
                 "It won't work without -txindex for utxo_size_inc, *fee or *feerate stats.\n",
@@ -1836,7 +1834,9 @@ static UniValue getblockstats(const JSONRPCRequest& request)
                     HelpExampleCli("getblockstats", "1000 '[\"minfeerate\",\"avgfeerate\"]'")
             + HelpExampleRpc("getblockstats", "1000 '[\"minfeerate\",\"avgfeerate\"]'")
                 },
-            }.ToString());
+    };
+    if (request.fHelp || !help.IsValidNumArgs(request.params.size())) {
+        throw std::runtime_error(help.ToString());
     }
 
     LOCK(cs_main);
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6bbbbc9876..c7b3478f44 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -523,13 +523,7 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
 
 static UniValue setban(const JSONRPCRequest& request)
 {
-    std::string strCommand;
-    if (!request.params[1].isNull())
-        strCommand = request.params[1].get_str();
-    if (request.fHelp || request.params.size() < 2 ||
-        (strCommand != "add" && strCommand != "remove"))
-        throw std::runtime_error(
-            RPCHelpMan{"setban",
+    const RPCHelpMan help{"setban",
                 "\nAttempts to add or remove an IP/Subnet from the banned list.\n",
                 {
                     {"subnet", RPCArg::Type::STR, RPCArg::Optional::NO, "The IP/Subnet (see getpeerinfo for nodes IP) with an optional netmask (default is /32 = single IP)"},
@@ -543,7 +537,13 @@ static UniValue setban(const JSONRPCRequest& request)
                             + HelpExampleCli("setban", "\"192.168.0.0/24\" \"add\"")
                             + HelpExampleRpc("setban", "\"192.168.0.6\", \"add\", 86400")
                 },
-            }.ToString());
+    };
+    std::string strCommand;
+    if (!request.params[1].isNull())
+        strCommand = request.params[1].get_str();
+    if (request.fHelp || !help.IsValidNumArgs(request.params.size()) || (strCommand != "add" && strCommand != "remove")) {
+        throw std::runtime_error(help.ToString());
+    }
     if (!g_banman) {
         throw JSONRPCError(RPC_DATABASE_ERROR, "Error: Ban database not loaded");
     }
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 1eec916abf..86695bc1a5 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2018 The Bitcoin Core developers
+// Copyright (c) 2017-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -314,6 +314,17 @@ std::string RPCExamples::ToDescriptionString() const
     return m_examples.empty() ? m_examples : "\nExamples:\n" + m_examples;
 }
 
+bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
+{
+    size_t num_required_args = 0;
+    for (size_t n = m_args.size(); n > 0; --n) {
+        if (!m_args.at(n - 1).IsOptional()) {
+            num_required_args = n;
+            break;
+        }
+    }
+    return num_required_args <= num_args && num_args <= m_args.size();
+}
 std::string RPCHelpMan::ToString() const
 {
     std::string ret;
@@ -322,12 +333,7 @@ std::string RPCHelpMan::ToString() const
     ret += m_name;
     bool was_optional{false};
     for (const auto& arg : m_args) {
-        bool optional;
-        if (arg.m_fallback.which() == 1) {
-            optional = true;
-        } else {
-            optional = RPCArg::Optional::NO != boost::get<RPCArg::Optional>(arg.m_fallback);
-        }
+        const bool optional = arg.IsOptional();
         ret += " ";
         if (optional) {
             if (!was_optional) ret += "( ";
@@ -369,6 +375,15 @@ std::string RPCHelpMan::ToString() const
     return ret;
 }
 
+bool RPCArg::IsOptional() const
+{
+    if (m_fallback.which() == 1) {
+        return true;
+    } else {
+        return RPCArg::Optional::NO != boost::get<RPCArg::Optional>(m_fallback);
+    }
+}
+
 std::string RPCArg::ToDescriptionString() const
 {
     std::string ret;
diff --git a/src/rpc/util.h b/src/rpc/util.h
index a83ae98b7e..06800ad63c 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2017-2018 The Bitcoin Core developers
+// Copyright (c) 2017-2019 The Bitcoin Core developers
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
@@ -54,7 +54,7 @@ struct RPCArg {
         /** Required arg */
         NO,
         /**
-         * Optinal arg that is a named argument and has a default value of
+         * Optional arg that is a named argument and has a default value of
          * `null`. When possible, the default value should be specified.
          */
         OMITTED_NAMED_ARG,
@@ -111,6 +111,8 @@ struct RPCArg {
         assert(type == Type::ARR || type == Type::OBJ);
     }
 
+    bool IsOptional() const;
+
     /**
      * Return the type string of the argument.
      * Set oneline to allow it to be overridden by a custom oneline type string (m_oneline_description).
@@ -186,6 +188,8 @@ public:
     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples);
 
     std::string ToString() const;
+    /** If the supplied number of args is neither too small nor too high */
+    bool IsValidNumArgs(size_t num_args) const;
 
 private:
     const std::string m_name;
diff --git a/test/functional/rpc_getblockstats.py b/test/functional/rpc_getblockstats.py
index ca9e24367a..feba16f1b3 100755
--- a/test/functional/rpc_getblockstats.py
+++ b/test/functional/rpc_getblockstats.py
@@ -178,5 +178,10 @@ class GetblockstatsTest(BitcoinTestFramework):
         assert_raises_rpc_error(-5, 'Block not found', self.nodes[0].getblockstats,
                                 hash_or_height='000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f')
 
+        # Invalid number of args
+        assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats, '00', 1, 2)
+        assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats)
+
+
 if __name__ == '__main__':
     GetblockstatsTest().main()