Merge "adb: parse tcp socket specs with base::ParseNetAddress."

adb/Android.mk

@@ -62,6 +62,7 @@ LIBADB_TEST_SRCS := \
     adb_listeners_test.cpp \
     adb_utils_test.cpp \
     fdevent_test.cpp \
+    socket_spec_test.cpp \
     socket_test.cpp \
     sysdeps_test.cpp \
     sysdeps/stat_test.cpp \

adb/socket_spec.cpp

@@ -20,6 +20,8 @@
 #include <unordered_map>
 #include <vector>
 
+#include <android-base/parseint.h>
+#include <android-base/parsenetaddress.h>
 #include <android-base/stringprintf.h>
 #include <android-base/strings.h>
 #include <cutils/sockets.h>
@@ -62,55 +64,47 @@ static auto& kLocalSocketTypes = *new std::unordered_map<std::string, LocalSocke
     { "localfilesystem", { ANDROID_SOCKET_NAMESPACE_FILESYSTEM, !ADB_WINDOWS } },
 });
 
-static bool parse_tcp_spec(const std::string& spec, std::string* hostname, int* port,
+bool parse_tcp_socket_spec(const std::string& spec, std::string* hostname, int* port,
                            std::string* error) {
-    std::vector<std::string> fragments = android::base::Split(spec, ":");
+    if (!StartsWith(spec, "tcp:")) {
-    if (fragments.size() == 1 || fragments.size() > 3) {
-        *error = StringPrintf("invalid tcp specification: '%s'", spec.c_str());
-        return false;
-    }
-
-    if (fragments[0] != "tcp") {
         *error = StringPrintf("specification is not tcp: '%s'", spec.c_str());
         return false;
     }
 
-    // strtol accepts leading whitespace.
+    std::string hostname_value;
-    const std::string& port_str = fragments.back();
+    int port_value;
-    if (port_str.empty() || port_str[0] < '0' || port_str[0] > '9') {
-        *error = StringPrintf("invalid port '%s'", port_str.c_str());
-        return false;
-    }
 
-    char* parsed_end;
+    // If the spec is tcp:<port>, parse it ourselves.
-    long parsed_port = strtol(port_str.c_str(), &parsed_end, 10);
+    // Otherwise, delegate to android::base::ParseNetAddress.
-    if (*parsed_end != '\0') {
+    if (android::base::ParseInt(&spec[4], &port_value)) {
-        *error = StringPrintf("trailing chars in port: '%s'", port_str.c_str());
+        // Do the range checking ourselves, because ParseInt rejects 'tcp:65536' and 'tcp:foo:1234'
-        return false;
+        // identically.
-    }
+        if (port_value < 0 || port_value > 65535) {
-    if (parsed_port > 65535) {
+            *error = StringPrintf("bad port number '%d'", port_value);
-        *error = StringPrintf("invalid port %ld", parsed_port);
-        return false;
-    }
-
-    // tcp:123 is valid, tcp::123 isn't.
-    if (fragments.size() == 2) {
-        // Empty hostname.
-        if (hostname) {
-            *hostname = "";
-        }
-    } else {
-        if (fragments[1].empty()) {
-            *error = StringPrintf("empty host in '%s'", spec.c_str());
             return false;
         }
-        if (hostname) {
+    } else {
-            *hostname = fragments[1];
+        std::string addr = spec.substr(4);
+        port_value = -1;
+
+        // FIXME: ParseNetAddress rejects port 0. This currently doesn't hurt, because listening
+        //        on an address that isn't 'localhost' is unsupported.
+        if (!android::base::ParseNetAddress(addr, &hostname_value, &port_value, nullptr, error)) {
+            return false;
         }
+
+        if (port_value == -1) {
+            *error = StringPrintf("missing port in specification: '%s'", spec.c_str());
+            return false;
+        }
+    }
+
+    if (hostname) {
+        *hostname = std::move(hostname_value);
     }
 
     if (port) {
-        *port = parsed_port;
+        *port = port_value;
     }
 
     return true;
@@ -141,7 +135,7 @@ bool is_local_socket_spec(const std::string& spec) {
 
     std::string error;
     std::string hostname;
-    if (!parse_tcp_spec(spec, &hostname, nullptr, &error)) {
+    if (!parse_tcp_socket_spec(spec, &hostname, nullptr, &error)) {
         return false;
     }
     return tcp_host_is_local(hostname);
@@ -151,7 +145,7 @@ int socket_spec_connect(const std::string& spec, std::string* error) {
     if (StartsWith(spec, "tcp:")) {
         std::string hostname;
         int port;
-        if (!parse_tcp_spec(spec, &hostname, &port, error)) {
+        if (!parse_tcp_socket_spec(spec, &hostname, &port, error)) {
             return -1;
         }
 
@@ -196,7 +190,7 @@ int socket_spec_listen(const std::string& spec, std::string* error, int* resolve
     if (StartsWith(spec, "tcp:")) {
         std::string hostname;
         int port;
-        if (!parse_tcp_spec(spec, &hostname, &port, error)) {
+        if (!parse_tcp_socket_spec(spec, &hostname, &port, error)) {
             return -1;
         }
 

adb/socket_spec.h

@@ -25,3 +25,7 @@ bool is_local_socket_spec(const std::string& spec);
 int socket_spec_connect(const std::string& spec, std::string* error);
 int socket_spec_listen(const std::string& spec, std::string* error,
                        int* resolved_tcp_port = nullptr);
+
+// Exposed for testing.
+bool parse_tcp_socket_spec(const std::string& spec, std::string* hostname, int* port,
+                           std::string* error);

adb/socket_spec_test.cpp

@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "socket_spec.h"
+
+#include <string>
+
+#include <gtest/gtest.h>
+
+TEST(socket_spec, parse_tcp_socket_spec) {
+    std::string hostname, error;
+    int port;
+    EXPECT_TRUE(parse_tcp_socket_spec("tcp:5037", &hostname, &port, &error));
+    EXPECT_EQ("", hostname);
+    EXPECT_EQ(5037, port);
+
+    // Bad ports:
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:-1", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:65536", &hostname, &port, &error));
+
+    EXPECT_TRUE(parse_tcp_socket_spec("tcp:localhost:1234", &hostname, &port, &error));
+    EXPECT_EQ("localhost", hostname);
+    EXPECT_EQ(1234, port);
+
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:localhost", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:localhost:", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:localhost:-1", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:localhost:65536", &hostname, &port, &error));
+
+    // IPv6:
+    EXPECT_TRUE(parse_tcp_socket_spec("tcp:[::1]:1234", &hostname, &port, &error));
+    EXPECT_EQ("::1", hostname);
+    EXPECT_EQ(1234, port);
+
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:[::1]", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:[::1]:", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:[::1]:-1", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:::1", &hostname, &port, &error));
+    EXPECT_FALSE(parse_tcp_socket_spec("tcp:::1:1234", &hostname, &port, &error));
+}