From 1099215e30c3b09dfa390c638a45487c43b2b1e1 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Mon, 19 Sep 2016 17:31:55 -0700 Subject: [PATCH] adb: parse tcp socket specs with base::ParseNetAddress. libbase already has IPv6-aware address parsing, so use it instead of adb's handrolled IPv4-only parsing. Bug: http://b/31537253 Change-Id: I4e9ce56b55d7d02787c0fa67b724490bf49ce479 Test: mma && adb start-server && \ adb -L 'tcp:[::ffff:127.0.0.1]:5037' devices && \ adb -L 'tcp:localhost:5037' devices && \ adb -L 'tcp:127.0.0.1:5037' devices && \ adb -L 'tcp:5037' devices && \ $ANDROID_HOST_OUT/nativetest64/adb_test/adb_test --- adb/Android.mk | 1 + adb/socket_spec.cpp | 74 ++++++++++++++++++---------------------- adb/socket_spec.h | 4 +++ adb/socket_spec_test.cpp | 54 +++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 40 deletions(-) create mode 100644 adb/socket_spec_test.cpp diff --git a/adb/Android.mk b/adb/Android.mk index b2a0dc4d1..0114ca37b 100644 --- a/adb/Android.mk +++ b/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 \ diff --git a/adb/socket_spec.cpp b/adb/socket_spec.cpp index 18e6e6d88..14eb16b98 100644 --- a/adb/socket_spec.cpp +++ b/adb/socket_spec.cpp @@ -20,6 +20,8 @@ #include #include +#include +#include #include #include #include @@ -62,55 +64,47 @@ static auto& kLocalSocketTypes = *new std::unordered_map fragments = android::base::Split(spec, ":"); - if (fragments.size() == 1 || fragments.size() > 3) { - *error = StringPrintf("invalid tcp specification: '%s'", spec.c_str()); - return false; - } - - if (fragments[0] != "tcp") { + if (!StartsWith(spec, "tcp:")) { *error = StringPrintf("specification is not tcp: '%s'", spec.c_str()); return false; } - // strtol accepts leading whitespace. - const std::string& port_str = fragments.back(); - if (port_str.empty() || port_str[0] < '0' || port_str[0] > '9') { - *error = StringPrintf("invalid port '%s'", port_str.c_str()); - return false; - } + std::string hostname_value; + int port_value; - char* parsed_end; - long parsed_port = strtol(port_str.c_str(), &parsed_end, 10); - if (*parsed_end != '\0') { - *error = StringPrintf("trailing chars in port: '%s'", port_str.c_str()); - return false; - } - if (parsed_port > 65535) { - *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()); + // If the spec is tcp:, parse it ourselves. + // Otherwise, delegate to android::base::ParseNetAddress. + if (android::base::ParseInt(&spec[4], &port_value)) { + // Do the range checking ourselves, because ParseInt rejects 'tcp:65536' and 'tcp:foo:1234' + // identically. + if (port_value < 0 || port_value > 65535) { + *error = StringPrintf("bad port number '%d'", port_value); return false; } - if (hostname) { - *hostname = fragments[1]; + } else { + 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; } diff --git a/adb/socket_spec.h b/adb/socket_spec.h index 6302da5a2..6920e91ff 100644 --- a/adb/socket_spec.h +++ b/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); diff --git a/adb/socket_spec_test.cpp b/adb/socket_spec_test.cpp new file mode 100644 index 000000000..40ce21cea --- /dev/null +++ b/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 + +#include + +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)); +}