Validate JSON object types in Consul responses

src/brpc/policy/consul_naming_service.cpp

@@ -137,6 +137,11 @@ int ConsulNamingService::GetServers(const char* service_name,
     }
 
     for (BUTIL_RAPIDJSON_NAMESPACE::SizeType i = 0; i < services.Size(); ++i) {
+        if (!services[i].IsObject()) {
+            LOG(ERROR) << "Service node is not a json object: "
+                       << RapidjsonValueToString(services[i]);
+            continue;
+        }
         auto itr_service = services[i].FindMember("Service");
         if (itr_service == services[i].MemberEnd()) {
             LOG(ERROR) << "No service info in node: "
@@ -145,6 +150,11 @@ int ConsulNamingService::GetServers(const char* service_name,
         }
 
         const BUTIL_RAPIDJSON_NAMESPACE::Value& service = itr_service->value;
+        if (!service.IsObject()) {
+            LOG(ERROR) << "Service info is not a json object: "
+                       << RapidjsonValueToString(service);
+            continue;
+        }
         auto itr_address = service.FindMember("Address");
         auto itr_port = service.FindMember("Port");
         if (itr_address == service.MemberEnd() ||

test/brpc_naming_service_unittest.cpp

@@ -427,6 +427,72 @@ class ConsulNamingServiceImpl : public test::UserNamingService {
     butil::atomic<int64_t> touch_count;
 };
 
+class ConsulResponseService : public test::UserNamingService {
+public:
+    explicit ConsulResponseService(const std::string& response)
+        : _response(response) {}
+
+    void ListNames(google::protobuf::RpcController* cntl_base,
+                   const test::HttpRequest*,
+                   test::HttpResponse*,
+                   google::protobuf::Closure* done) override {
+        brpc::ClosureGuard done_guard(done);
+        brpc::Controller* cntl = static_cast<brpc::Controller*>(cntl_base);
+        cntl->http_response().SetHeader("X-Consul-Index", "1");
+        cntl->response_attachment().append(_response);
+    }
+
+private:
+    const std::string _response;
+};
+
+TEST(NamingServiceTest, consul_response_object_types) {
+    GFLAGS_NAMESPACE::FlagSaver flags_saver;
+    brpc::policy::FLAGS_consul_enable_degrade_to_file_naming_service = false;
+    brpc::policy::FLAGS_consul_service_discovery_url = "/v1/health/service/";
+    const char* invalid_entries[] = {
+        R"(null, true, false, 1, 1.5, "node", [])",
+        R"({"Service":null}, {"Service":true}, {"Service":false},
+            {"Service":1}, {"Service":1.5}, {"Service":"service"},
+            {"Service":[]})"
+    };
+    butil::EndPoint endpoint;
+    ASSERT_EQ(0, butil::str2endpoint("127.0.0.1:8003", &endpoint));
+    const brpc::ServerNode expected_node(endpoint, "tag");
+
+    for (const char* entries : invalid_entries) {
+        for (bool with_valid_service : {false, true}) {
+            SCOPED_TRACE(entries);
+            SCOPED_TRACE(with_valid_service);
+            std::string response = std::string("[") + entries;
+            if (with_valid_service) {
+                response += R"(,{"Service":{"Address":"127.0.0.1",
+                              "Port":8003,"Tags":["tag"]}})";
+            }
+            response += "]";
+            ConsulResponseService svc(response);
+            brpc::Server server;
+            ASSERT_EQ(0, server.AddService(&svc, brpc::SERVER_DOESNT_OWN_SERVICE,
+                                          "/v1/health/service/test => ListNames"));
+            ASSERT_EQ(0, server.Start(0, nullptr));
+            brpc::policy::FLAGS_consul_agent_addr = butil::string_printf(
+                "http://%s", butil::endpoint2str(server.listen_address()).c_str());
+
+            brpc::policy::ConsulNamingService cns;
+            // GetServers must clear stale results, including on invalid input.
+            std::vector<brpc::ServerNode> servers(1, expected_node);
+            ASSERT_EQ(with_valid_service ? 0 : -1,
+                      cns.GetServers("test", &servers));
+            if (with_valid_service) {
+                ASSERT_EQ(1u, servers.size());
+                EXPECT_EQ(expected_node, servers[0]);
+            } else {
+                EXPECT_TRUE(servers.empty());
+            }
+        }
+    }
+}
+
 TEST(NamingServiceTest, consul_with_backup_file) {
     GFLAGS_NAMESPACE::FlagSaver flags_saver;
     brpc::policy::FLAGS_consul_enable_degrade_to_file_naming_service = true;