You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "wasphin (via GitHub)" <gi...@apache.org> on 2026/09/08 15:26:53 UTC
[PR] Validate Discovery response object fields (brpc)
wasphin opened a new pull request, #3533:
URL: https://github.com/apache/brpc/pull/3533
### What problem does this PR solve?
Issue Number: N/A
Problem Summary:
The Discovery naming-service parser looks up members in several JSON values
without first checking that those values are objects. Malformed Discovery
responses can therefore violate the RapidJSON object-access precondition.
### What is changed and the side effects?
Changed:
- Skip non-object entries in the Discovery node list.
- Reject non-object data, service, and instance values in service responses.
- Cover malformed values through the existing Discovery client/server test.
Side effects:
- Performance effects: only constant-time JSON type checks are added.
- Breaking backward compatibility: malformed responses that previously reached
object-only accessors are now skipped or rejected.
---
### Check List:
- [x] The changes are compilable.
- [x] Related unit tests are included.
- [x] The Contributor Covenant Code of Conduct is followed.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Validate Discovery response object fields (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on PR #3533:
URL: https://github.com/apache/brpc/pull/3533#issuecomment-5601767426
LGTM
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Validate Discovery response object fields (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3533:
URL: https://github.com/apache/brpc/pull/3533#discussion_r3959692080
##########
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:
The new error logs are missing key context needed for debugging in production (e.g., which request/endpoint failed and, for instances, which index failed). Consider including `service_name` consistently and the failing instance index (`i`), and (if available in this function) the Discovery API address/URL to disambiguate multi-cluster/multi-service failures.
##########
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 new error logs are missing key context needed for debugging in production (e.g., which request/endpoint failed and, for instances, which index failed). Consider including `service_name` consistently and the failing instance index (`i`), and (if available in this function) the Discovery API address/URL to disambiguate multi-cluster/multi-service failures.
##########
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:
This change makes a single malformed element in `instances[]` cause the entire `GetServers()` call to fail (return -1), potentially dropping otherwise valid instances in mixed-quality responses. If the intent is robustness similar to `ListDiscoveryNodes()` (which skips invalid entries), consider skipping non-object instances (and continuing) instead of hard-failing—or document/justify the stricter behavior here so operators understand why partial results are not returned.
##########
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:
The new error logs are missing key context needed for debugging in production (e.g., which request/endpoint failed and, for instances, which index failed). Consider including `service_name` consistently and the failing instance index (`i`), and (if available in this function) the Discovery API address/URL to disambiguate multi-cluster/multi-service failures.
##########
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:
When this loop fails, it can be hard to see which specific `service_name` caused the assertion. Add a `SCOPED_TRACE(service_name)` (or equivalent) inside the loop so test output clearly identifies the failing case.
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Validate Discovery response object fields (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Validate Discovery response object fields (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm merged PR #3533:
URL: https://github.com/apache/brpc/pull/3533
--
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
Re: [PR] Validate Discovery response object fields (brpc)
Posted by "wasphin (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@brpc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org