You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by "wwbmmm (via GitHub)" <gi...@apache.org> on 2026/10/08 12:47:37 UTC
[PR] Reject deeply nested JSON replies in naming services (brpc)
wwbmmm opened a new pull request, #3592:
URL: https://github.com/apache/brpc/pull/3592
### What problem does this PR solve?
Problem Summary: The consul, nacos and discovery naming services parsed registry replies with rapidjson's default recursive parser. A deeply nested JSON reply from the naming-service backend crashed the consuming process with stack exhaustion (SIGSEGV). Note that iterative parsing alone is not sufficient for this input class: rapidjson also builds, visits (Value::Accept) and destroys (~GenericValue) its DOM recursively, so any unbounded nesting must be rejected before parsing. json2pb was already hardened against the same input with kParseIterativeFlag plus a recursion-depth cap; the naming-service parsers were missed by that change.
### What is changed and the side effects?
Changed:
- Added a shared helper `src/brpc/policy/naming_service_json.h`: `ParseNamingServiceJson()` parses replies iteratively (`kParseIterativeFlag`) and rejects, before parsing, replies nested deeper than `kMaxNamingServiceJsonDepth` (100, the same default as `json2pb_max_recursion_depth`). Depth is counted by a string-aware linear scan, so brackets inside string literals don't count and the scan can stop as soon as the limit is exceeded.
- All six `Document::Parse` call sites now go through the helper and fail cleanly: consul_naming_service.cpp (service list), nacos_naming_service.cpp (auth and instance-list replies), discovery_naming_service.cpp (nodes, common result and fetchs replies).
- With the nesting bounded, the remaining recursions over the document (DOM destruction, consul's error stringification, discovery's metadata conversion) are bounded to 100 levels and safe.
Side effects:
- Performance effects: one extra linear scan over the reply before parsing; negligible. Overly deep replies are rejected early without building a large DOM.
- Breaking backward compatibility: No. Legit consul/nacos/discovery replies are only a few levels deep; replies nested deeper than 100 levels are now treated as malformed and rejected with an error log (they previously crashed the process).
Tests: `test/brpc_naming_service_unittest.cpp` adds `naming_service_json_depth_limit` (valid/malformed replies, brackets inside strings, boundary at depth 100/101, 140000-deep reply) plus end-to-end regressions `consul/nacos/discovery_deeply_nested_reply` that serve a 140000-deep reply over real HTTP; `GetServers()` now returns -1 instead of crashing. Verified these tests segfault with the previous parsing behavior and pass after the fix; the rest of the suite is unaffected (the only failures in this container are pre-existing, caused by no external DNS/network, and fail identically on the base commit).
---
🤖 This PR was automatically created by brpc-oncall
--
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] Reject deeply nested JSON replies in naming services (brpc)
Posted by "chenBright (via GitHub)" <gi...@apache.org>.
chenBright merged PR #3592:
URL: https://github.com/apache/brpc/pull/3592
--
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] Reject deeply nested JSON replies in naming services (brpc)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3592:
URL: https://github.com/apache/brpc/pull/3592#discussion_r4219267478
##########
src/brpc/policy/naming_service_json.h:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ --depth;
+ }
+ }
+ return max_depth;
+}
+
+// Parse a JSON reply of a naming service into `doc' iteratively. Returns
+// false if `text' is not valid JSON or is nested deeper than
+// kMaxNamingServiceJsonDepth, `doc' is left empty in the latter case.
+inline bool ParseNamingServiceJson(const std::string& text,
+ BUTIL_RAPIDJSON_NAMESPACE::Document* doc) {
+ if (MaxJsonDepth(text) > kMaxNamingServiceJsonDepth) {
Review Comment:
The comment says "`doc` is left empty" when the depth limit is exceeded, but the function returns `false` without clearing `*doc`, so callers may observe stale contents from a previous successful parse. Either clear `doc` before returning (e.g., `doc->SetNull()`), or adjust the comment to reflect current behavior.
##########
src/brpc/policy/naming_service_json.h:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ --depth;
+ }
+ }
+ return max_depth;
+}
+
+// Parse a JSON reply of a naming service into `doc' iteratively. Returns
+// false if `text' is not valid JSON or is nested deeper than
+// kMaxNamingServiceJsonDepth, `doc' is left empty in the latter case.
+inline bool ParseNamingServiceJson(const std::string& text,
+ BUTIL_RAPIDJSON_NAMESPACE::Document* doc) {
+ if (MaxJsonDepth(text) > kMaxNamingServiceJsonDepth) {
+ return false;
+ }
+ doc->Parse<BUTIL_RAPIDJSON_NAMESPACE::kParseIterativeFlag>(text.c_str());
+ return !doc->HasParseError();
+}
Review Comment:
`Document::Parse(text.c_str())` parses as a null-terminated C-string; if `text` ever contains embedded `\0`, the parser will stop early while `MaxJsonDepth(text)` scans the full buffer, creating an inconsistency (and potentially allowing tail content to bypass the depth guard). Prefer parsing with an explicit length (e.g., `text.data()` + `text.size()`) so the same bytes are scanned and parsed.
##########
src/brpc/policy/naming_service_json.h:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ --depth;
+ }
+ }
+ return max_depth;
+}
Review Comment:
`depth` is decremented unconditionally on `}`/`]`. For malformed inputs with many closing brackets early, `depth` can go negative; on sufficiently large inputs this can underflow `int`, which is avoidable even if the subsequent JSON parse would fail. Clamping `depth` at 0 (or only decrementing when `depth > 0`) makes the pre-scan more robust without affecting valid JSON behavior.
--
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] Reject deeply nested JSON replies in naming services (brpc)
Posted by "wwbmmm (via GitHub)" <gi...@apache.org>.
wwbmmm commented on code in PR #3592:
URL: https://github.com/apache/brpc/pull/3592#discussion_r4226100842
##########
src/brpc/policy/naming_service_json.h:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ --depth;
+ }
+ }
+ return max_depth;
+}
+
+// Parse a JSON reply of a naming service into `doc' iteratively. Returns
+// false if `text' is not valid JSON or is nested deeper than
+// kMaxNamingServiceJsonDepth, `doc' is left empty in the latter case.
+inline bool ParseNamingServiceJson(const std::string& text,
+ BUTIL_RAPIDJSON_NAMESPACE::Document* doc) {
+ if (MaxJsonDepth(text) > kMaxNamingServiceJsonDepth) {
+ return false;
+ }
+ doc->Parse<BUTIL_RAPIDJSON_NAMESPACE::kParseIterativeFlag>(text.c_str());
+ return !doc->HasParseError();
+}
Review Comment:
[replied by brpc-oncall robot] Good point — note that rapidjson's parser treats any `'\0'` from `Peek()` as end-of-input, so even an explicit-length stream would silently accept a prefix before an embedded NUL. Fixed in 9ab7fb81 by rejecting replies that contain a raw NUL up front: a raw NUL is invalid JSON anywhere (RFC 8259), so the pre-scan and the parser now accept exactly the same bytes. Added a unit test covering the NUL + deep-suffix case.
##########
src/brpc/policy/naming_service_json.h:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ --depth;
+ }
+ }
+ return max_depth;
+}
Review Comment:
[replied by brpc-oncall robot] Fixed in 9ab7fb81: the depth counter is now clamped at zero, so extra closing brackets on malformed input can't drive it negative. Covered by a unit test.
##########
src/brpc/policy/naming_service_json.h:
##########
@@ -0,0 +1,86 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+
+#ifndef BRPC_POLICY_NAMING_SERVICE_JSON_H
+#define BRPC_POLICY_NAMING_SERVICE_JSON_H
+
+#include <string>
+#include "butil/third_party/rapidjson/document.h"
+
+namespace brpc {
+namespace policy {
+
+// Maximum nesting depth of JSON replies accepted from naming-service
+// backends. Legit replies of consul/nacos/discovery are only a few levels
+// deep, deeper replies are treated as malformed and rejected before
+// parsing: rapidjson builds, visits and destroys its DOM recursively, so
+// unbounded nesting could exhaust the stack even though the parsing itself
+// is iterative (same rationale as json2pb's kParseIterativeFlag plus
+// json2pb_max_recursion_depth).
+const int kMaxNamingServiceJsonDepth = 100;
+
+// Count the maximum nesting depth of '{'/'[' in `text', ignoring brackets
+// inside string literals.
+inline int MaxJsonDepth(const std::string& text) {
+ int depth = 0;
+ int max_depth = 0;
+ bool in_string = false;
+ bool escaped = false;
+ for (size_t i = 0; i < text.size(); ++i) {
+ const char c = text[i];
+ if (in_string) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '"') {
+ in_string = false;
+ }
+ } else if (c == '"') {
+ in_string = true;
+ } else if (c == '{' || c == '[') {
+ if (++depth > max_depth) {
+ max_depth = depth;
+ if (max_depth > kMaxNamingServiceJsonDepth) {
+ // Too deep, no need to scan further.
+ return max_depth;
+ }
+ }
+ } else if (c == '}' || c == ']') {
+ --depth;
+ }
+ }
+ return max_depth;
+}
+
+// Parse a JSON reply of a naming service into `doc' iteratively. Returns
+// false if `text' is not valid JSON or is nested deeper than
+// kMaxNamingServiceJsonDepth, `doc' is left empty in the latter case.
+inline bool ParseNamingServiceJson(const std::string& text,
+ BUTIL_RAPIDJSON_NAMESPACE::Document* doc) {
+ if (MaxJsonDepth(text) > kMaxNamingServiceJsonDepth) {
Review Comment:
[replied by brpc-oncall robot] Reworded the comment in 9ab7fb81: `doc' is untouched (not cleared) when the depth limit is exceeded. All callers pass a freshly constructed Document, so no stale state is observable.
--
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