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 9c7aa42f. 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 included 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 9c7aa42f. The malformed-instance log now includes both service_name
and the instance index.
##########
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 9c7aa42f. Non-object instance entries are now skipped, matching the
tolerant handling of malformed node and address entries. The test response
mixes one malformed entry with one valid entry and verifies that the valid
server is retained.
##########
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:
Done in 9c7aa42f. The malformed-service loop now uses
SCOPED_TRACE(service_name).
--
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]
