From e8cd93d4bb5016cace70ea73fe368dcf2c01c78c Mon Sep 17 00:00:00 2001
From: yanjunxiang-google
 <78807980+yanjunxiang-google@users.noreply.github.com>
Date: Wed, 10 Mar 2021 14:46:58 -0500
Subject: [PATCH] upstream: fix subset_lb crash when host configured with no
 metadata (#15171)

This commit is to fix oss-fuzz test issue:
https://oss-fuzz.com/testcase-detail/5135200453525504

The problem is that when executing a fuzz test case with empty host
metadata, envoy crashed at subset_lb.cc:rebuildSingle(): line 131.

The root cause is that current envoy code is assuming host->medata is
valid, and directly using it without NULL check. In the case if metadata
is actually NULL, which is a valid configuration, it crashes.

The fix is to add a if (metadat != nullptr) before accessing metadata.

GTEST code is added to reproduce the issue, and verified the fix.

The fix is also verified by running the oss-fuzz test with that
special testcase input file.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
---
 source/common/upstream/subset_lb.cc           |  3 ++
 .../http_subset_lb_integration_test.cc        | 35 +++++++++++++++++++
 ...inimized-server_fuzz_test-5135200453525504 | 32 +++++++++++++++++
 3 files changed, 70 insertions(+)
 create mode 100644 test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5135200453525504

diff --git a/source/common/upstream/subset_lb.cc b/source/common/upstream/subset_lb.cc
index 33a922c3dbdb..65fc2d241c6a 100644
--- a/source/common/upstream/subset_lb.cc
+++ b/source/common/upstream/subset_lb.cc
@@ -126,6 +126,9 @@ void SubsetLoadBalancer::rebuildSingle() {
   for (const auto& host_set : original_priority_set_.hostSetsPerPriority()) {
     for (const auto& host : host_set->hosts()) {
       MetadataConstSharedPtr metadata = host->metadata();
+      if (metadata == nullptr) {
+        continue;
+      }
       const auto& filter_metadata = metadata->filter_metadata();
       auto filter_it = filter_metadata.find(Config::MetadataFilters::get().ENVOY_LB);
       if (filter_it != filter_metadata.end()) {
diff --git a/test/integration/http_subset_lb_integration_test.cc b/test/integration/http_subset_lb_integration_test.cc
index 3094db9cf42e..cad7d7586503 100644
--- a/test/integration/http_subset_lb_integration_test.cc
+++ b/test/integration/http_subset_lb_integration_test.cc
@@ -216,4 +216,39 @@ TEST_P(HttpSubsetLbIntegrationTest, SubsetLoadBalancer) {
   runTest(type_b_request_headers_, "b");
 }
 
+// Tests subset-compatible load balancer policy without metadata does not crash on initialization
+// with single_host_per_subset set to be true.
+TEST_P(HttpSubsetLbIntegrationTest, SubsetLoadBalancerSingleHostPerSubsetNoMetadata) {
+  config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+    auto* static_resources = bootstrap.mutable_static_resources();
+    auto* cluster = static_resources->mutable_clusters(0);
+
+    // Set single_host_per_subset to be true. This will avoid function
+    // SubsetLoadBalancer::rebuildSingle() bailout early. Thus exercise the no metadata logic.
+    auto* subset_selector = cluster->mutable_lb_subset_config()->mutable_subset_selectors(0);
+    subset_selector->set_single_host_per_subset(true);
+
+    // Clear the metadata for each host
+    auto* load_assignment = cluster->mutable_load_assignment();
+    auto* endpoints = load_assignment->mutable_endpoints(0);
+    for (uint32_t i = 0; i < num_hosts_; i++) {
+      auto* lb_endpoint = endpoints->mutable_lb_endpoints(i);
+      lb_endpoint->clear_metadata();
+    }
+  });
+
+  initialize();
+  codec_client_ = makeHttpConnection(lookupPort("http"));
+  auto response =
+      codec_client_->makeHeaderOnlyRequest(Http::TestRequestHeaderMapImpl{{":method", "GET"},
+                                                                          {":path", "/test"},
+                                                                          {":scheme", "http"},
+                                                                          {":authority", "host"},
+                                                                          {"x-type", "a"},
+                                                                          {"x-hash", "hash-a"}});
+  response->waitForEndStream();
+  EXPECT_TRUE(response->complete());
+  EXPECT_THAT(response->headers(), Http::HttpStatusIs("503"));
+}
+
 } // namespace Envoy
diff --git a/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5135200453525504 b/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5135200453525504
new file mode 100644
index 000000000000..7bc577ee5c80
--- /dev/null
+++ b/test/server/server_corpus/clusterfuzz-testcase-minimized-server_fuzz_test-5135200453525504
@@ -0,0 +1,32 @@
+static_resources {
+  clusters {
+    name: "0"
+    connect_timeout {
+      nanos: 813
+    }
+    lb_policy: RING_HASH
+    lb_subset_config {
+      fallback_policy: ANY_ENDPOINT
+      subset_selectors {
+        keys: "\010,"
+        single_host_per_subset: true
+      }
+    }
+    load_assignment {
+      cluster_name: "."
+      endpoints {
+        lb_endpoints {
+          endpoint {
+            address {
+              pipe {
+                path: "y"
+              }
+            }
+            hostname: "y"
+          }
+          health_status: UNHEALTHY
+        }
+      }
+    }
+  }
+}