Reject out-of-range RPC values instead of narrowing them silently.

Guards choke group max, set-port and info-hash length; parsing now uses std::from_chars.

src/command_groups.cc

@@ -1,5 +1,6 @@
 #include "config.h"
 
+#include <limits>
 #include <memory>
 
 #include <torrent/download/resource_manager.h>
@@ -232,6 +233,9 @@ apply_cg_max_set(const torrent::Object::list_type& args, bool is_up) {
   int64_t second_arg = 0;
   rpc::parse_whole_value(args.back().as_string().c_str(), &second_arg);
 
+  if (second_arg < -1 || second_arg > std::numeric_limits<uint32_t>::max())
+    throw torrent::input_error("Max unchoked must be between -1 and 4294967295.");
+
   if (is_up)
     cg_get_group(args.front())->up_queue()->set_max_unchoked(second_arg);
   else

src/command_helpers.h

@@ -1,12 +1,25 @@
 #ifndef RTORRENT_UTILS_COMMAND_HELPERS_H
 #define RTORRENT_UTILS_COMMAND_HELPERS_H
 
+#include <cstdint>
+#include <limits>
+#include <string>
+#include <torrent/exceptions.h>
+
 #include "rpc/command.h"
 #include "rpc/parse_commands.h"
 #include "rpc/object_storage.h"
 
 void initialize_commands();
 
+inline uint16_t
+checked_port_value(int64_t value, const char* label) {
+  if (value < 0 || value > std::numeric_limits<uint16_t>::max())
+    throw torrent::input_error(std::string("Invalid ") + label + " port number.");
+
+  return static_cast<uint16_t>(value);
+}
+
 //
 // Aliases with CMD_* for the below
 //

src/command_network.cc

@@ -54,14 +54,6 @@ set_listen_port_range(const std::string& arg) {
   torrent::runtime::client_config()->set_listen_port_range(port_first, port_last);
 }
 
-uint16_t
-checked_local_port_value(int64_t value, const char* label) {
-  if (value < 0 || value > 65535)
-    throw torrent::input_error(std::string("Invalid ") + label + " port number.");
-
-  return static_cast<uint16_t>(value);
-}
-
 torrent::Object
 get_encryption() {
   auto encryption_modes = torrent::runtime::network_config()->encryption_modes();
@@ -325,7 +317,7 @@ initialize_command_network() {
   auto nw_config      = torrent::runtime::network_config();
 
   CMD_ANY         ("network.listen.port",            [](auto, auto)        { return torrent::runtime::network_manager()->listen_port(); });
-  CMD_ANY_VALUE_V ("network.listen.port.set",        [](auto, auto& value) { return torrent::runtime::network_manager()->set_listen_port(value); });
+  CMD_ANY_VALUE_V ("network.listen.port.set",        [](auto, auto& value) { return torrent::runtime::network_manager()->set_listen_port(checked_port_value(value, "listen")); });
   CMD_ANY         ("network.listen.port.random",     [](auto, auto)        { return torrent::runtime::client_config()->listen_port_random(); });
   CMD_ANY_VALUE_V ("network.listen.port.random.set", [](auto, auto& value) { return torrent::runtime::client_config()->set_listen_port_random(value); });
   CMD_ANY         ("network.listen.port.range",      [](auto, auto)        { return listen_port_range(); });
@@ -394,11 +386,11 @@ initialize_command_network() {
   CMD_ANY_STRING_V("network.local_address.ipv6.set",         [nw_config](auto, auto& str)    { return nw_config->set_local_inet6_address(str); });
 
   CMD_ANY         ("network.local_port",                     [nw_config](auto, auto)         { return nw_config->local_port_best_match(); });
-  CMD_ANY_VALUE_V ("network.local_port.set",                 [nw_config](auto, auto& value)  { return nw_config->set_local_port(checked_local_port_value(value, "local")); });
+  CMD_ANY_VALUE_V ("network.local_port.set",                 [nw_config](auto, auto& value)  { return nw_config->set_local_port(checked_port_value(value, "local")); });
   CMD_ANY         ("network.local_port.ipv4",                [nw_config](auto, auto)         { return nw_config->local_inet_port(); });
-  CMD_ANY_VALUE_V ("network.local_port.ipv4.set",            [nw_config](auto, auto& value)  { return nw_config->set_local_inet_port(checked_local_port_value(value, "local ipv4")); });
+  CMD_ANY_VALUE_V ("network.local_port.ipv4.set",            [nw_config](auto, auto& value)  { return nw_config->set_local_inet_port(checked_port_value(value, "local ipv4")); });
   CMD_ANY         ("network.local_port.ipv6",                [nw_config](auto, auto)         { return nw_config->local_inet6_port(); });
-  CMD_ANY_VALUE_V ("network.local_port.ipv6.set",            [nw_config](auto, auto& value)  { return nw_config->set_local_inet6_port(checked_local_port_value(value, "local ipv6")); });
+  CMD_ANY_VALUE_V ("network.local_port.ipv6.set",            [nw_config](auto, auto& value)  { return nw_config->set_local_inet6_port(checked_port_value(value, "local ipv6")); });
 
   CMD_ANY         ("network.proxy.global",                   [](auto, auto)                  { return torrent::runtime::proxy_manager()->proxy_url(); });
   CMD_ANY_STRING_V("network.proxy.global.set",               [](auto, auto& str)             { return torrent::runtime::proxy_manager()->set_proxy_url(str); });

src/command_tracker.cc

@@ -142,7 +142,7 @@ initialize_command_tracker() {
       lt_log_print(torrent::LOG_DHT_ERROR, "dht.port.set is no longer supported, use dht.override_port.set", 0);
     });
   CMD2_ANY            ("dht.override_port",     [](auto, auto)        { return torrent::runtime::network_config()->override_dht_port(); });
-  CMD2_ANY_VALUE_V    ("dht.override_port.set", [](auto, auto& value) { return torrent::runtime::network_manager()->set_dht_port(value); });
+  CMD2_ANY_VALUE_V    ("dht.override_port.set", [](auto, auto& value) { return torrent::runtime::network_manager()->set_dht_port(checked_port_value(value, "DHT override")); });
 
   CMD2_ANY_STRING     ("dht.add_node",          [](auto, auto& str)   { return apply_dht_add_node(str); });
   CMD2_ANY            ("dht.statistics",        [](auto, auto)        { return control->dht_manager()->dht_statistics(); });

src/core/download_list.cc

@@ -82,7 +82,7 @@ DownloadList::find(const torrent::HashString& hash) {
 
 DownloadList::iterator
 DownloadList::find_hex(const char* hash) {
-  if (strlen(hash) < 40)
+  if (strlen(hash) != 40)
     return end();
 
   torrent::HashString key;

src/rpc/parse.cc

@@ -1,5 +1,7 @@
 #include "config.h"
 
+#include <cctype>
+#include <charconv>
 #include <cstring>
 #include <cstdio>
 #include <limits>
@@ -113,10 +115,27 @@ parse_value_nothrow(const char* src, int64_t* value, int base, int unit) {
   if (unit <= 0)
     throw torrent::input_error("Command::string_to_value_unit(...) received unit <= 0.");
 
-  char* last;
-  *value = strtoll(src, &last, base);
+  const char* first = src;
+
+  while (std::isspace(static_cast<unsigned char>(*first)))
+    first++;
+
+  if ((base == 0 || base == 16) && first[0] == '0' && (first[1] == 'x' || first[1] == 'X')) {
+    first += 2;
+    base = 16;
+
+  } else if (base == 0) {
+    base = 10;
+  }
+
+  const auto result = std::from_chars(first, first + std::strlen(first), *value, base);
+
+  if (result.ec == std::errc::result_out_of_range)
+    return src;
+
+  if (result.ec != std::errc()) {
+    *value = 0;
 
-  if (last == src) {
     if (strcasecmp(src, "no") == 0) { *value = 0; return src + strlen("no"); }
     if (strcasecmp(src, "yes") == 0) { *value = 1; return src + strlen("yes"); }
     if (strcasecmp(src, "true") == 0) { *value = 1; return src + strlen("true"); }
@@ -125,6 +144,8 @@ parse_value_nothrow(const char* src, int64_t* value, int base, int unit) {
     return src;
   }
 
+  const char* last = result.ptr;
+
   switch (*last) {
   case 'b':
   case 'B': ++last; break;

test/Makefile.am

@@ -51,6 +51,8 @@ rtorrent_Test_Rpc_SOURCES = $(rtorrent_Test_Common) \
 	rpc/test_command_slot.h \
 	rpc/test_object_storage.cc \
 	rpc/test_object_storage.h \
+	rpc/test_parse.cc \
+	rpc/test_parse.h \
 	rpc/test_parse_options.cc \
 	rpc/test_parse_options.h \
 	rpc/test_rpc_manager.cc \
@@ -59,6 +61,8 @@ rtorrent_Test_Rpc_SOURCES = $(rtorrent_Test_Common) \
 rtorrent_Test_Src_SOURCES = $(rtorrent_Test_Common) \
 	src/test_command_dynamic.cc \
 	src/test_command_dynamic.h \
+	src/test_command_groups.cc \
+	src/test_command_groups.h \
 	src/test_command_ip.cc \
 	src/test_command_ip.h \
 	src/test_command_system.cc \
@@ -67,6 +71,10 @@ rtorrent_Test_Src_SOURCES = $(rtorrent_Test_Common) \
 	src/test_command_path.h \
 	src/test_command_string.cc \
 	src/test_command_string.h \
+	src/test_command_tracker.cc \
+	src/test_command_tracker.h \
+	src/test_download_list.cc \
+	src/test_download_list.h \
 	src/test_setup.cc \
 	src/test_setup.h \
 	src/test_ui_download_list.cc \

test/rpc/test_parse.cc

@@ -0,0 +1,85 @@
+#include "config.h"
+
+#include "test/rpc/test_parse.h"
+
+#include <cstdint>
+#include <limits>
+
+#include "rpc/parse.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestParse);
+
+void
+TestParse::test_whole_value_in_range() {
+  int64_t value = 0;
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("9223372036854775807", &value));
+  CPPUNIT_ASSERT_EQUAL(std::numeric_limits<int64_t>::max(), value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("-9223372036854775808", &value));
+  CPPUNIT_ASSERT_EQUAL(std::numeric_limits<int64_t>::min(), value);
+}
+
+void
+TestParse::test_whole_value_out_of_range() {
+  int64_t value = 0;
+
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("9223372036854775808", &value));
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("-9223372036854775809", &value));
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("99999999999999999999999999", &value));
+}
+
+void
+TestParse::test_whole_value_bases() {
+  int64_t value = 0;
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("0x1f", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{31}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("0X1F", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{31}, value);
+
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("-0x1f", &value));
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("0022", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{22}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("22", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{22}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("1f", &value, 16));
+  CPPUNIT_ASSERT_EQUAL(int64_t{31}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("0x1f", &value, 16));
+  CPPUNIT_ASSERT_EQUAL(int64_t{31}, value);
+
+  const char* no_digits = "0x";
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow(no_digits, &value));
+  CPPUNIT_ASSERT_EQUAL(no_digits, rpc::parse_value_nothrow(no_digits, &value));
+
+  const char* bad_digits = "0xzz";
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow(bad_digits, &value));
+  CPPUNIT_ASSERT_EQUAL(bad_digits, rpc::parse_value_nothrow(bad_digits, &value));
+}
+
+void
+TestParse::test_whole_value_prefixes() {
+  int64_t value = 0;
+
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("+5", &value));
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("  5  ", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{5}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("5k", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{5120}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("yes", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{1}, value);
+
+  CPPUNIT_ASSERT(rpc::parse_whole_value_nothrow("false", &value));
+  CPPUNIT_ASSERT_EQUAL(int64_t{0}, value);
+
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("junk", &value));
+  CPPUNIT_ASSERT(!rpc::parse_whole_value_nothrow("", &value));
+}

test/rpc/test_parse.h

@@ -0,0 +1,18 @@
+#include "test/helpers/test_fixture.h"
+
+class TestParse : public test_fixture {
+  CPPUNIT_TEST_SUITE(TestParse);
+
+  CPPUNIT_TEST(test_whole_value_in_range);
+  CPPUNIT_TEST(test_whole_value_out_of_range);
+  CPPUNIT_TEST(test_whole_value_bases);
+  CPPUNIT_TEST(test_whole_value_prefixes);
+
+  CPPUNIT_TEST_SUITE_END();
+
+public:
+  void test_whole_value_in_range();
+  void test_whole_value_out_of_range();
+  void test_whole_value_bases();
+  void test_whole_value_prefixes();
+};

test/src/test_command_groups.cc

@@ -0,0 +1,65 @@
+#include "config.h"
+
+#include "test/src/test_command_groups.h"
+
+#include <torrent/torrent.h>
+
+#include "control.h"
+#include "globals.h"
+#include "rpc/parse_commands.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestCommandGroups);
+
+void initialize_command_groups();
+
+static void
+call_set(const char* value) {
+  torrent::Object::list_type args;
+  args.push_back(torrent::Object(int64_t{0}));
+  args.push_back(torrent::Object(std::string(value)));
+
+  rpc::commands.call_command("choke_group.up.max.set", torrent::Object::create_list_range(args.begin(), args.end()));
+}
+
+static int64_t
+call_get(const char* key) {
+  return rpc::commands.call_command(key, torrent::Object(int64_t{0})).as_value();
+}
+
+void
+TestCommandGroups::setUp() {
+  torrent::initialize_main_thread();
+  torrent::initialize();
+
+  if (control == nullptr)
+    control = new Control;
+
+  if (!rpc::commands.has("choke_group.up.max.set"))
+    initialize_command_groups();
+}
+
+void
+TestCommandGroups::tearDown() {
+  torrent::cleanup();
+}
+
+void
+TestCommandGroups::test_max_unchoked_in_range() {
+  call_set("50");
+  CPPUNIT_ASSERT_EQUAL(int64_t{50}, call_get("choke_group.up.max"));
+  CPPUNIT_ASSERT_EQUAL(int64_t{0}, call_get("choke_group.up.max.unlimited"));
+
+  call_set("-1");
+  CPPUNIT_ASSERT_EQUAL(int64_t{1}, call_get("choke_group.up.max.unlimited"));
+}
+
+void
+TestCommandGroups::test_max_unchoked_out_of_range() {
+  call_set("50");
+
+  CPPUNIT_ASSERT_THROW(call_set("4294967296"), torrent::input_error);
+  CPPUNIT_ASSERT_EQUAL(int64_t{50}, call_get("choke_group.up.max"));
+
+  CPPUNIT_ASSERT_THROW(call_set("-2"), torrent::input_error);
+  CPPUNIT_ASSERT_EQUAL(int64_t{50}, call_get("choke_group.up.max"));
+}

test/src/test_command_groups.h

@@ -0,0 +1,17 @@
+#include "test/helpers/test_fixture.h"
+
+class TestCommandGroups : public test_fixture {
+  CPPUNIT_TEST_SUITE(TestCommandGroups);
+
+  CPPUNIT_TEST(test_max_unchoked_in_range);
+  CPPUNIT_TEST(test_max_unchoked_out_of_range);
+
+  CPPUNIT_TEST_SUITE_END();
+
+public:
+  void setUp();
+  void tearDown();
+
+  void test_max_unchoked_in_range();
+  void test_max_unchoked_out_of_range();
+};

test/src/test_command_tracker.cc

@@ -0,0 +1,73 @@
+#include "config.h"
+
+#include "test/src/test_command_tracker.h"
+
+#include <torrent/runtime/network_config.h>
+#include <torrent/torrent.h>
+
+#include "control.h"
+#include "globals.h"
+#include "command_helpers.h"
+#include "rpc/parse_commands.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestCommandTracker);
+
+void initialize_command_tracker();
+
+static void
+call_set(const char* value) {
+  rpc::commands.call_command("dht.override_port.set", torrent::Object(std::string(value)));
+}
+
+static uint16_t
+override_port() {
+  return torrent::runtime::network_config()->override_dht_port();
+}
+
+void
+TestCommandTracker::setUp() {
+  torrent::initialize_main_thread();
+  torrent::initialize();
+
+  if (control == nullptr)
+    control = new Control;
+
+  if (!rpc::commands.has("dht.override_port.set"))
+    initialize_command_tracker();
+}
+
+void
+TestCommandTracker::tearDown() {
+  torrent::cleanup();
+}
+
+void
+TestCommandTracker::test_dht_override_port_in_range() {
+  call_set("6881");
+  CPPUNIT_ASSERT_EQUAL(uint16_t{6881}, override_port());
+
+  call_set("65535");
+  CPPUNIT_ASSERT_EQUAL(uint16_t{65535}, override_port());
+}
+
+void
+TestCommandTracker::test_dht_override_port_out_of_range() {
+  call_set("6881");
+
+  CPPUNIT_ASSERT_THROW(call_set("70000"), torrent::input_error);
+  CPPUNIT_ASSERT_EQUAL(uint16_t{6881}, override_port());
+
+  CPPUNIT_ASSERT_THROW(call_set("-1"), torrent::input_error);
+  CPPUNIT_ASSERT_EQUAL(uint16_t{6881}, override_port());
+}
+
+void
+TestCommandTracker::test_checked_port_value() {
+  CPPUNIT_ASSERT_EQUAL(uint16_t{0}, checked_port_value(0, "test"));
+  CPPUNIT_ASSERT_EQUAL(uint16_t{6881}, checked_port_value(6881, "test"));
+  CPPUNIT_ASSERT_EQUAL(uint16_t{65535}, checked_port_value(65535, "test"));
+
+  CPPUNIT_ASSERT_THROW(checked_port_value(-1, "test"), torrent::input_error);
+  CPPUNIT_ASSERT_THROW(checked_port_value(65536, "test"), torrent::input_error);
+  CPPUNIT_ASSERT_THROW(checked_port_value(4294967296, "test"), torrent::input_error);
+}

test/src/test_command_tracker.h

@@ -0,0 +1,19 @@
+#include "test/helpers/test_fixture.h"
+
+class TestCommandTracker : public test_fixture {
+  CPPUNIT_TEST_SUITE(TestCommandTracker);
+
+  CPPUNIT_TEST(test_dht_override_port_in_range);
+  CPPUNIT_TEST(test_dht_override_port_out_of_range);
+  CPPUNIT_TEST(test_checked_port_value);
+
+  CPPUNIT_TEST_SUITE_END();
+
+public:
+  void setUp();
+  void tearDown();
+
+  void test_dht_override_port_in_range();
+  void test_dht_override_port_out_of_range();
+  void test_checked_port_value();
+};

test/src/test_download_list.cc

@@ -0,0 +1,68 @@
+#include "config.h"
+
+#include "test/src/test_download_list.h"
+
+#include <cstdio>
+#include <torrent/download.h>
+#include <torrent/download_info.h>
+#include <torrent/hash_string.h>
+#include <torrent/object.h>
+#include <torrent/torrent.h>
+
+#include "control.h"
+#include "core/download.h"
+#include "globals.h"
+
+CPPUNIT_TEST_SUITE_REGISTRATION(TestDownloadList);
+
+static std::string
+insert_download(core::DownloadList* list) {
+  torrent::Object info = torrent::Object::create_map();
+  info.insert_key("name", std::string("test_download_list"));
+  info.insert_key("length", int64_t{16});
+  info.insert_key("piece length", int64_t{262144});
+  info.insert_key("pieces", std::string(20, char(0)));
+
+  auto* object = new torrent::Object(torrent::Object::create_map());
+  object->insert_key("info", info);
+
+  auto download = torrent::download_add(object, 0);
+  list->insert(new core::Download(download));
+
+  char buffer[41];
+
+  for (unsigned int i = 0; i < torrent::HashString::size_data; i++)
+    snprintf(buffer + i * 2, 3, "%02x", static_cast<unsigned char>(download.info()->hash()[i]));
+
+  return std::string(buffer, 40);
+}
+
+void
+TestDownloadList::setUp() {
+  torrent::initialize_main_thread();
+  torrent::initialize();
+
+  if (control == nullptr)
+    control = new Control;
+
+  m_hex = insert_download(&m_list);
+}
+
+void
+TestDownloadList::tearDown() {
+  m_list.clear();
+
+  torrent::cleanup();
+}
+
+void
+TestDownloadList::test_find_hex() {
+  CPPUNIT_ASSERT(m_list.find_hex(m_hex.c_str()) != m_list.end());
+}
+
+void
+TestDownloadList::test_find_hex_wrong_length() {
+  CPPUNIT_ASSERT(m_list.find_hex((m_hex + "f").c_str()) == m_list.end());
+  CPPUNIT_ASSERT(m_list.find_hex((m_hex + m_hex).c_str()) == m_list.end());
+  CPPUNIT_ASSERT(m_list.find_hex(m_hex.substr(0, 39).c_str()) == m_list.end());
+}

test/src/test_download_list.h

@@ -0,0 +1,24 @@
+#include <string>
+
+#include "core/download_list.h"
+#include "test/helpers/test_fixture.h"
+
+class TestDownloadList : public test_fixture {
+  CPPUNIT_TEST_SUITE(TestDownloadList);
+
+  CPPUNIT_TEST(test_find_hex);
+  CPPUNIT_TEST(test_find_hex_wrong_length);
+
+  CPPUNIT_TEST_SUITE_END();
+
+public:
+  void setUp();
+  void tearDown();
+
+  void test_find_hex();
+  void test_find_hex_wrong_length();
+
+private:
+  core::DownloadList m_list;
+  std::string        m_hex;
+};