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

@@ -271,11 +271,13 @@ absl::StatusOr<std::string> 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<grpc_core::URI> 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(

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"],

test/core/address_utils/sockaddr_uri_corpus/sample.dat

@@ -0,0 +1 @@
+

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 <stdbool.h>
+#include <stdint.h>
+#include <string.h>
+
+#include <grpc/grpc.h>
+
+#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<std::string> uri = grpc_sockaddr_to_uri(&address);
+  if (!uri.ok()) return 0;
+  absl::StatusOr<grpc_core::URI> parsed_uri =
+      grpc_core::URI::Parse(uri.value());
+
+  GPR_ASSERT(parsed_uri.ok());
+  return 0;
+}

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));

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 "

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);

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 kIpv6("ipv6:%5B::1%5D:");
-  const std::string kIpv4MappedIpv6("ipv6:[::ffff:127.0.0.1]:");
+  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 ||