fix: weak constraint of ipv4 and incorrect code logic in hostname_from_ip

include/dsn/tool-api/rpc_address.h

@@ -25,13 +25,17 @@
  */
 #pragma once
 
+#include <cstdlib>
+#include <cstdint>
+#include <cstring>
+#include <iostream>
+#include <sstream>
+#include <string>
 #include <unordered_map>
 #include <unordered_set>
 #include <vector>
-#include <cstring>
-#include <string>
-#include <cstdlib>
-#include <cstdint>
+
+#include <arpa/inet.h>
 #include <thrift/protocol/TProtocol.h>
 
 typedef enum dsn_host_type_t {
@@ -108,22 +112,33 @@ class rpc_address
     bool from_string_ipv4(const char *s)
     {
         set_invalid();
-        std::string str = std::string(s);
-        auto pos = str.find_last_of(':');
-        if (pos == std::string::npos)
+        std::string ip_port = std::string(s);
+        auto pos = ip_port.find_last_of(':');
+        if (pos == std::string::npos) {
             return false;
-        else {
-            auto host = str.substr(0, pos);
-            auto port_str = str.substr(pos + 1);
-            char *p = nullptr;
-            long port = ::strtol(port_str.data(), &p, 10);
-            if (*p != 0) // bad string
-                return false;
-            if (port <= 0 || port > UINT16_MAX) // out of range
+        }
+        std::string ip = ip_port.substr(0, pos);
+        std::string port = ip_port.substr(pos + 1);
+        if (pos == std::string::npos) {
+            return false;
+        }
+        // check port
+        if (port.size() > 5)
+            return false;
+        for (auto c : port) {
+            if (!isdigit(c))
                 return false;
-            assign_ipv4(host.c_str(), (uint16_t)port);
-            return true;
         }
+        int port_num = std::stoi(port);
+        if (port_num > UINT16_MAX)
+            return false;
+        // check ip
+        uint32_t ip_addr;
+        if (inet_pton(AF_INET, ip.c_str(), &ip_addr) != 1) {
+            return false;
+        }
+        assign_ipv4(ip.c_str(), (uint16_t)port_num);
+        return true;
     }
 
     uint64_t &value() { return _addr.value; }

src/core/tests/hostname_test.cpp

@@ -10,58 +10,68 @@
 namespace dsn {
 namespace replication {
 
+TEST(ip_to_hostname, ipv4_validate)
+{
+    rpc_address rpc_test_ipv4;
+    struct ip_test
+    {
+        std::string ip;
+        bool result;
+    } tests[] = {{"127.0.0.1:8080", 1},
+                 {"172.16.254.1:1234", 1},
+                 {"172.16.254.1:222222", 0},
+                 {"172.16.254.1", 0},
+                 {"2222,123,33,1:8080", 0},
+                 {"123.456.789.1:8080", 0},
+                 {"001.223.110.002:8080", 0},
+                 {"172.16.254.1.8080", 0},
+                 {"172.16.254.1:8080.", 0},
+                 {"127.0.0.11:123!", 0},
+                 {"127.0.0.11:123!", 1}};
+
+    for (auto test : tests) {
+        ASSERT_EQ(rpc_test_ipv4.from_string_ipv4(test.ip.c_str()), test.result);
+    }
+}
+
 TEST(ip_to_hostname, localhost)
 {
     std::string hostname_result;
 
-    const std::string success_ip = "127.0.0.1";
+    const std::string valid_ip = "127.0.0.1";
     const std::string expected_hostname = "localhost";
 
-    const std::string success_ip_port = "127.0.0.1:23010";
+    const std::string valid_ip_port = "127.0.0.1:23010";
     const std::string expected_hostname_port = "localhost:23010";
 
-    const std::string failed_ip = "123.456.789.111";
-    const std::string failed_ip_port = "123.456.789.111:23010";
-
-    const std::string success_ip_list = "127.0.0.1,127.0.0.1,127.0.0.1";
+    const std::string valid_ip_list = "127.0.0.1,127.0.0.1,127.0.0.1";
     const std::string expected_hostname_list = "localhost,localhost,localhost";
 
-    const std::string success_ip_port_list = "127.0.0.1:8080,127.0.0.1:8080,127.0.0.1:8080";
+    const std::string valid_ip_port_list = "127.0.0.1:8080,127.0.0.1:8080,127.0.0.1:8080";
     const std::string expected_hostname_port_list = "localhost:8080,localhost:8080,localhost:8080";
 
-    rpc_address rpc_example_success, rpc_example_failed;
-    rpc_example_success.assign_ipv4(success_ip.c_str(), 23010);
-    rpc_example_failed.assign_ipv4(failed_ip.c_str(), 23010);
+    rpc_address rpc_example_valid;
+    rpc_example_valid.assign_ipv4(valid_ip.c_str(), 23010);
 
     // static bool hostname(const rpc_address &address,std::string *hostname_result);
-    ASSERT_TRUE(dsn::utils::hostname(rpc_example_success, &hostname_result));
+    ASSERT_TRUE(dsn::utils::hostname(rpc_example_valid, &hostname_result));
     ASSERT_STREQ(expected_hostname_port.c_str(), hostname_result.c_str());
 
-    ASSERT_FALSE(dsn::utils::hostname(rpc_example_failed, &hostname_result));
-
     // static bool hostname_from_ip(uint32_t ip, std::string* hostname_result);
-    ASSERT_TRUE(dsn::utils::hostname_from_ip(htonl(rpc_example_success.ip()), &hostname_result));
+    ASSERT_TRUE(dsn::utils::hostname_from_ip(htonl(rpc_example_valid.ip()), &hostname_result));
     ASSERT_STREQ(expected_hostname.c_str(), hostname_result.c_str());
 
-    ASSERT_FALSE(dsn::utils::hostname_from_ip(htonl(rpc_example_failed.ip()), &hostname_result));
-
     // static bool hostname_from_ip(const char *ip,std::string *hostname_result);
-    ASSERT_TRUE(dsn::utils::hostname_from_ip(success_ip.c_str(), &hostname_result));
+    ASSERT_TRUE(dsn::utils::hostname_from_ip(valid_ip.c_str(), &hostname_result));
     ASSERT_STREQ(expected_hostname.c_str(), hostname_result.c_str());
 
-    ASSERT_FALSE(dsn::utils::hostname_from_ip(failed_ip.c_str(), &hostname_result));
-    ASSERT_STREQ(failed_ip.c_str(), hostname_result.c_str());
-
     // static bool hostname_from_ip_port(const char *ip_port,std::string *hostname_result);
-    ASSERT_TRUE(dsn::utils::hostname_from_ip_port(success_ip_port.c_str(), &hostname_result));
+    ASSERT_TRUE(dsn::utils::hostname_from_ip_port(valid_ip_port.c_str(), &hostname_result));
     ASSERT_STREQ(expected_hostname_port.c_str(), hostname_result.c_str());
 
-    ASSERT_FALSE(dsn::utils::hostname_from_ip_port(failed_ip_port.c_str(), &hostname_result));
-    ASSERT_STREQ(failed_ip_port.c_str(), hostname_result.c_str());
-
     // static bool list_hostname_from_ip(const char *ip_port_list,std::string
     // *hostname_result_list);
-    ASSERT_TRUE(dsn::utils::list_hostname_from_ip(success_ip_list.c_str(), &hostname_result));
+    ASSERT_TRUE(dsn::utils::list_hostname_from_ip(valid_ip_list.c_str(), &hostname_result));
     ASSERT_STREQ(expected_hostname_list.c_str(), hostname_result.c_str());
 
     ASSERT_FALSE(dsn::utils::list_hostname_from_ip("127.0.0.1,127.0.0.23323,111127.0.0.3",
@@ -74,13 +84,27 @@ TEST(ip_to_hostname, localhost)
     // static bool list_hostname_from_ip_port(const char *ip_port_list,std::string
     // *hostname_result_list);
     ASSERT_TRUE(
-        dsn::utils::list_hostname_from_ip_port(success_ip_port_list.c_str(), &hostname_result));
+        dsn::utils::list_hostname_from_ip_port(valid_ip_port_list.c_str(), &hostname_result));
     ASSERT_STREQ(expected_hostname_port_list.c_str(), hostname_result.c_str());
 
     ASSERT_FALSE(dsn::utils::list_hostname_from_ip_port(
         "127.0.3333.1:23456,1127.0.0.2:22233,127.0.0.1:8080", &hostname_result));
     ASSERT_STREQ("127.0.3333.1:23456,1127.0.0.2:22233,localhost:8080", hostname_result.c_str());
 }
 
+TEST(ip_to_hostname, invalid_ip)
+{
+
+    std::string hostname_result;
+    const std::string invalid_ip = "123.456.789.111";
+    const std::string invalid_ip_port = "123.456.789.111:23010";
+
+    ASSERT_FALSE(dsn::utils::hostname_from_ip(invalid_ip.c_str(), &hostname_result));
+    ASSERT_STREQ(invalid_ip.c_str(), hostname_result.c_str());
+
+    ASSERT_FALSE(dsn::utils::hostname_from_ip_port(invalid_ip_port.c_str(), &hostname_result));
+    ASSERT_STREQ(invalid_ip_port.c_str(), hostname_result.c_str());
+}
+
 } // namespace replication
 } // namespace dsn