wasphin commented on code in PR #3533:
URL: https://github.com/apache/brpc/pull/3533#discussion_r3959867731
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -385,12 +388,20 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
return -1;
}
const BUTIL_RAPIDJSON_NAMESPACE::Value& data = itr_data->value;
+ if (!data.IsObject()) {
+ LOG(ERROR) << "data field is not a json object";
+ return -1;
+ }
Review Comment:
Done in 88d4eff9. The data-type error now includes service_name. The
selected Discovery endpoint is not directly available at this parsing point
after Channel::CallMethod returns, so the service provides the stable request
context.
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -385,12 +388,20 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
return -1;
}
const BUTIL_RAPIDJSON_NAMESPACE::Value& data = itr_data->value;
+ if (!data.IsObject()) {
+ LOG(ERROR) << "data field is not a json object";
+ return -1;
+ }
auto itr_service = data.FindMember(service_name);
if (itr_service == data.MemberEnd()) {
LOG(ERROR) << "No " << service_name << " field in discovery response";
return -1;
}
const BUTIL_RAPIDJSON_NAMESPACE::Value& services = itr_service->value;
+ if (!services.IsObject()) {
+ LOG(ERROR) << "Service " << service_name << " is not a json object";
+ return -1;
+ }
Review Comment:
The service-value error already includes service_name. The selected
Discovery endpoint is not directly available at this parsing point, so no
additional endpoint logging was added.
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -403,6 +414,10 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
}
for (BUTIL_RAPIDJSON_NAMESPACE::SizeType i = 0; i < instances.Size(); ++i)
{
+ if (!instances[i].IsObject()) {
+ LOG(ERROR) << "instance is not a json object";
+ return -1;
Review Comment:
Done in 88d4eff9. Non-object instance entries are now skipped, matching the
tolerant handling of malformed node and address entries. The shared Discovery
test fixture remains unchanged; its existing single client/server request
covers skipping a non-object node entry.
##########
src/brpc/policy/discovery_naming_service.cpp:
##########
@@ -403,6 +414,10 @@ int DiscoveryNamingService::GetServers(const char*
service_name,
}
for (BUTIL_RAPIDJSON_NAMESPACE::SizeType i = 0; i < instances.Size(); ++i)
{
+ if (!instances[i].IsObject()) {
+ LOG(ERROR) << "instance is not a json object";
+ return -1;
+ }
Review Comment:
Done in 88d4eff9. The malformed-instance log now includes both service_name
and the instance index.
##########
test/brpc_naming_service_unittest.cpp:
##########
@@ -710,6 +728,17 @@ TEST(NamingServiceTest, discovery_sanity) {
ASSERT_FALSE(svc.HasAddr(std::string()));
ASSERT_EQ(2, svc.AddrCount());
}
+
+ const char* invalid_services[] = {
+ "invalid-data.test",
+ "invalid-service.test",
+ "invalid-instance.test",
+ };
+ for (const char* service_name : invalid_services) {
+ servers.clear();
+ ASSERT_EQ(-1, dcns.GetServers(service_name, &servers));
+ ASSERT_TRUE(servers.empty());
+ }
Review Comment:
The loop was removed in 88d4eff9, so SCOPED_TRACE no longer applies.
Consecutive GetServers calls use the process-wide round-robin Discovery channel
and could reach fixture nodes without local servers, making network failure
indistinguishable from parser rejection. The shared fixture is now unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
