From e89883cc876ec7837cd7f45b5ca7c3ddf6a84deb Mon Sep 17 00:00:00 2001 From: Anirudh Ramachandra Date: Thu, 2 Jun 2022 13:51:04 -0700 Subject: [PATCH] Revert "Revert "Fix grpc_sockaddr_to_uri to return URI encoded string. Also see #29323 which has a similar fix for unix abstract addresses."" (#29798) Reverts #29639 --- src/core/lib/address_utils/sockaddr_utils.cc | 8 ++-- test/core/address_utils/BUILD | 14 ++++++ .../sockaddr_uri_corpus/sample.dat | 1 + .../sockaddr_utils_fuzzer_test.cc | 43 +++++++++++++++++++ .../core/address_utils/sockaddr_utils_test.cc | 9 ++-- test/core/end2end/dualstack_socket_test.cc | 3 +- test/core/security/evaluate_args_test.cc | 2 +- test/cpp/end2end/end2end_test.cc | 4 +- 8 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 test/core/address_utils/sockaddr_uri_corpus/sample.dat create mode 100644 test/core/address_utils/sockaddr_utils_fuzzer_test.cc diff --git a/src/core/lib/address_utils/sockaddr_utils.cc b/src/core/lib/address_utils/sockaddr_utils.cc index a8aaa222ba6..a9b7b3fa25e 100644 --- a/src/core/lib/address_utils/sockaddr_utils.cc +++ b/src/core/lib/address_utils/sockaddr_utils.cc @@ -271,11 +271,13 @@ absl::StatusOr grpc_sockaddr_to_uri( if (scheme == nullptr || strcmp("unix", scheme) == 0) { return grpc_sockaddr_to_uri_unix_if_possible(resolved_addr); } - // TODO(anramach): Encode the string using URI::Create() and URI::ToString() - // before returning. auto path = grpc_sockaddr_to_string(resolved_addr, false /* normalize */); if (!path.ok()) return path; - return absl::StrCat(scheme, ":", path.value()); + absl::StatusOr uri = + grpc_core::URI::Create(scheme, /*authority=*/"", std::move(path.value()), + /*query_parameter_pairs=*/{}, /*fragment=*/""); + if (!uri.ok()) return uri.status(); + return uri->ToString(); } const char* grpc_sockaddr_get_uri_scheme( diff --git a/test/core/address_utils/BUILD b/test/core/address_utils/BUILD index e457eb19c40..83260909f4e 100644 --- a/test/core/address_utils/BUILD +++ b/test/core/address_utils/BUILD @@ -13,6 +13,7 @@ # limitations under the License. load("//bazel:grpc_build_system.bzl", "grpc_cc_test", "grpc_package") +load("//test/core/util:grpc_fuzzer.bzl", "grpc_fuzzer") licenses(["notice"]) @@ -46,6 +47,19 @@ grpc_cc_test( ], ) +grpc_fuzzer( + name = "sockaddr_utils_fuzzer_test", + srcs = ["sockaddr_utils_fuzzer_test.cc"], + corpus = "sockaddr_uri_corpus", + tags = ["no_windows"], + uses_event_engine = False, + uses_polling = False, + deps = [ + "//:grpc", + "//test/core/util:grpc_test_util", + ], +) + grpc_cc_test( name = "parse_address_with_named_scope_id_test", srcs = ["parse_address_with_named_scope_id_test.cc"], diff --git a/test/core/address_utils/sockaddr_uri_corpus/sample.dat b/test/core/address_utils/sockaddr_uri_corpus/sample.dat new file mode 100644 index 00000000000..6b2aaa76407 --- /dev/null +++ b/test/core/address_utils/sockaddr_uri_corpus/sample.dat @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/test/core/address_utils/sockaddr_utils_fuzzer_test.cc b/test/core/address_utils/sockaddr_utils_fuzzer_test.cc new file mode 100644 index 00000000000..2cecb09b2e7 --- /dev/null +++ b/test/core/address_utils/sockaddr_utils_fuzzer_test.cc @@ -0,0 +1,43 @@ +// +// Copyright 2022 gRPC authors. +// +// 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 +#include +#include + +#include + +#include "src/core/lib/address_utils/sockaddr_utils.h" +#include "src/core/lib/iomgr/resolve_address.h" +#include "src/core/lib/uri/uri_parser.h" + +bool squelch = true; + +extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { + if (size > GRPC_MAX_SOCKADDR_SIZE) return 0; + grpc_resolved_address address; + memset(&address, 0, sizeof(address)); + memcpy(address.addr, data, size); + address.len = size; + + absl::StatusOr uri = grpc_sockaddr_to_uri(&address); + if (!uri.ok()) return 0; + absl::StatusOr parsed_uri = + grpc_core::URI::Parse(uri.value()); + + GPR_ASSERT(parsed_uri.ok()); + return 0; +} diff --git a/test/core/address_utils/sockaddr_utils_test.cc b/test/core/address_utils/sockaddr_utils_test.cc index 9053ead6ac0..a4dac924cec 100644 --- a/test/core/address_utils/sockaddr_utils_test.cc +++ b/test/core/address_utils/sockaddr_utils_test.cc @@ -175,7 +175,8 @@ TEST(SockAddrUtilsTest, SockAddrToString) { "[2001:db8::1]:12345"); EXPECT_EQ(grpc_sockaddr_to_string(&input6, true).value(), "[2001:db8::1]:12345"); - EXPECT_EQ(grpc_sockaddr_to_uri(&input6).value(), "ipv6:[2001:db8::1]:12345"); + EXPECT_EQ(grpc_sockaddr_to_uri(&input6).value(), + "ipv6:%5B2001:db8::1%5D:12345"); SetIPv6ScopeId(&input6, 2); EXPECT_EQ(grpc_sockaddr_to_string(&input6, false).value(), @@ -183,7 +184,7 @@ TEST(SockAddrUtilsTest, SockAddrToString) { EXPECT_EQ(grpc_sockaddr_to_string(&input6, true).value(), "[2001:db8::1%252]:12345"); EXPECT_EQ(grpc_sockaddr_to_uri(&input6).value(), - "ipv6:[2001:db8::1%252]:12345"); + "ipv6:%5B2001:db8::1%25252%5D:12345"); SetIPv6ScopeId(&input6, 101); EXPECT_EQ(grpc_sockaddr_to_string(&input6, false).value(), @@ -191,7 +192,7 @@ TEST(SockAddrUtilsTest, SockAddrToString) { EXPECT_EQ(grpc_sockaddr_to_string(&input6, true).value(), "[2001:db8::1%25101]:12345"); EXPECT_EQ(grpc_sockaddr_to_uri(&input6).value(), - "ipv6:[2001:db8::1%25101]:12345"); + "ipv6:%5B2001:db8::1%2525101%5D:12345"); grpc_resolved_address input6x = MakeAddr6(kMapped, sizeof(kMapped)); EXPECT_EQ(grpc_sockaddr_to_string(&input6x, false).value(), @@ -206,7 +207,7 @@ TEST(SockAddrUtilsTest, SockAddrToString) { EXPECT_EQ(grpc_sockaddr_to_string(&input6y, true).value(), "[::fffe:c000:263]:12345"); EXPECT_EQ(grpc_sockaddr_to_uri(&input6y).value(), - "ipv6:[::fffe:c000:263]:12345"); + "ipv6:%5B::fffe:c000:263%5D:12345"); grpc_resolved_address phony; memset(&phony, 0, sizeof(phony)); diff --git a/test/core/end2end/dualstack_socket_test.cc b/test/core/end2end/dualstack_socket_test.cc index 5118b246c59..8e9ca0c5a04 100644 --- a/test/core/end2end/dualstack_socket_test.cc +++ b/test/core/end2end/dualstack_socket_test.cc @@ -292,7 +292,8 @@ int external_dns_works(const char* host) { // "dualstack_socket_test" due to loopback4.unittest.grpc.io resolving to // [64:ff9b::7f00:1]. (Working as expected for DNS64, but it prevents the // dualstack_socket_test from functioning correctly). See b/201064791. - if (grpc_sockaddr_to_uri(&addr).value() == "ipv6:[64:ff9b::7f00:1]:80") { + if (grpc_sockaddr_to_uri(&addr).value() == + "ipv6:%5B64:ff9b::7f00:1%5D:80") { gpr_log( GPR_INFO, "Detected DNS64 server response. Tests that depend on " diff --git a/test/core/security/evaluate_args_test.cc b/test/core/security/evaluate_args_test.cc index 2c511d0f5c2..2592555b27c 100644 --- a/test/core/security/evaluate_args_test.cc +++ b/test/core/security/evaluate_args_test.cc @@ -81,7 +81,7 @@ TEST_F(EvaluateArgsTest, TestLocalAddressAndPort) { EvaluateArgs args = util_.MakeEvaluateArgs(); grpc_resolved_address local_address = args.GetLocalAddress(); EXPECT_EQ(grpc_sockaddr_to_uri(&local_address).value(), - "ipv6:[2001:db8:85a3::8a2e:370:7334]:456"); + "ipv6:%5B2001:db8:85a3::8a2e:370:7334%5D:456"); EXPECT_EQ(args.GetLocalAddressString(), "2001:0db8:85a3:0000:0000:8a2e:0370:7334"); EXPECT_EQ(args.GetLocalPort(), 456); diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index add49c162bd..5f3f6d4c529 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -70,8 +70,8 @@ namespace testing { namespace { bool CheckIsLocalhost(const std::string& addr) { - const std::string kIpv6("ipv6:[::1]:"); - const std::string kIpv4MappedIpv6("ipv6:[::ffff:127.0.0.1]:"); + const std::string kIpv6("ipv6:%5B::1%5D:"); + const std::string kIpv4MappedIpv6("ipv6:%5B::ffff:127.0.0.1%5D:"); const std::string kIpv4("ipv4:127.0.0.1:"); return addr.substr(0, kIpv4.size()) == kIpv4 || addr.substr(0, kIpv4MappedIpv6.size()) == kIpv4MappedIpv6 ||