Repository navigation
Reject deeply nested JSON replies in naming services - #3592
Merged
Merged
Conversation
The consul, nacos and discovery naming services parsed registry replies with rapidjson's default recursive parser, so a deeply nested reply crashed the consuming process with stack exhaustion. rapidjson also builds, visits and destroys its DOM recursively, so iterative parsing alone is not enough. Route all the reply parsing through a shared ParseNamingServiceJson() which parses iteratively (kParseIterativeFlag) and rejects replies nested deeper than kMaxNamingServiceJsonDepth (100, same default as json2pb) before parsing, bounding every recursion on the document. Legit replies of these services are only a few levels deep.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
What changed in this PR
Hardens consul/nacos/discovery naming-service JSON parsing against stack-exhaustion from deeply nested replies by introducing a shared depth-limiting, iterative parsing helper and adding regression tests to ensure deeply nested inputs are rejected instead of crashing.
Changes:
- Added
ParseNamingServiceJson()helper with a pre-parse depth scan and RapidJSON iterative parsing. - Updated naming-service implementations (consul/nacos/discovery) to use the shared helper and fail cleanly on malformed/overly-deep JSON.
- Added unit + end-to-end regression tests covering depth limits, string-literal bracket handling, and extremely deep replies over HTTP.
| File | Description |
|---|---|
| test/brpc_naming_service_unittest.cpp | Adds depth-limit unit test and E2E regressions proving deeply nested replies no longer crash GetServers(). |
| src/brpc/policy/naming_service_json.h | Introduces shared depth-scanning + iterative JSON parsing helper for naming-service replies. |
| src/brpc/policy/nacos_naming_service.cpp | Routes Nacos auth and instance-list parsing through the hardened helper. |
| src/brpc/policy/discovery_naming_service.cpp | Routes Discovery parsing through the hardened helper with explicit failure handling. |
| src/brpc/policy/consul_naming_service.cpp | Routes Consul service-list parsing through the hardened helper with clean error handling. |
🧠 Review effort: Lite
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Copilot stopped reviewing on behalf of
wwbmmm due to an error
October 8, 2026 13:18
- A raw NUL is invalid JSON anywhere and rapidjson treats it as end-of-input, so reject replies containing one up front: the depth scan and the parser now accept exactly the same bytes. - Don't let the depth counter go negative on malformed text with more closing than opening brackets. - Reword the comment about the state of `doc' when the depth limit is exceeded: it is untouched rather than cleared.
zchuango
pushed a commit
to LinQuickDev/brpc
that referenced
this pull request
Oct 9, 2026
* Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar (apache#3557) * Support bvar::Histogram and fix Prometheus name/label conflicts between bvar and mbvar * Fix MVariableBase::dump_exposed return type * Fix some issues * Fix json output * Move the deduplication into PrometheusMetricsDumper * Bound total array allocation of one redis reply by -redis_max_allocation_size (apache#3560) * Bound total array allocation of one redis reply by -redis_max_allocation_size The redis reply parser capped each array allocation individually and the nesting depth separately, but nothing bounded the two together: nesting a maximally-sized array inside another committed depth * cap bytes from a tiny reply. Thread a per-reply byte budget through RedisReply::ConsumePartialIOBuf, save it in the root array when the parsing suspends and restore it on resume so that withholding data cannot reset it. Also stop pre-resizing RedisCommandParser::_args from a declared RESP array count; grow it as the arguments actually arrive so that a ~12-byte incomplete command no longer commits up to the cap per connection. * Initialize used_bytes in RedisReply::Reset() Arrays built by user code (e.g. SetArray) never went through the reply parser, so the new used_bytes field could hold an indeterminate value that CopyFromDifferentArena would read. Initialize it in Reset(), which every construction path goes through. --------- Co-authored-by: brpc-oncall <brpc-oncall@localhost> * Stabilize butex stop-after-running test (apache#3562) * Support higher performance histogram (apache#3561) * Support higher performance histogram * Fix some issues * Fix Maxer default value for floating-point types (apache#3563) * Fix wrong project and target names in auto_concurrency_limiter example CMakeLists (apache#3565) * Stabilize timing-sensitive bthread tests and fix the butex interruption race (apache#3545) * Stabilize timing-sensitive bthread tests and fix the butex interruption race * Fix pthread_kill get ESRCH * Use a non-fatal assertion * Fix some issues * Check and set task->interrupted under task->version_lock * Fix butex_requeue UT * Fix port conflicts in parallel unit tests (apache#3568) * Fix port conflicts in parallel unit tests * Isolate naming service test state * Fix flaky histogram window test (apache#3567) * Stabilize backup request policy test cleanup (apache#3564) * Fix use-after-free of response header in ParseMemcacheMessage (apache#3572) * Fix bthread_id lock leak in ProcessNsheadMcpackResponse (apache#3574) * Fix bthread_id lock leak in ProcessNsheadMcpackResponse After ProcessNsheadMcpackResponse acquires the bthread_id lock, its two early-return paths (response object missing, mcpack parse failure) skip accessor.OnResponse(), the only unlock entry. The leaked lock makes Join(correlation_id) hang forever and the RPC timeout cannot rescue it since the timeout error is only consumed during unlocking. A server returning a valid nshead header plus a malformed mcpack body reliably triggers this. Adopt the do { ... break ... } while(0) pattern (consistent with ProcessRpcResponse) so that OnResponse() is always reached. * Close unused pipe read end in nshead_mcpack unittest fixture * Fix compile error with new protobuf in nshead_mcpack unittest Descriptor::full_name() returns absl::string_view since protobuf 35.x, which cannot be implicitly converted to the const std::string& parameter of mcpack2pb::register_message_handler_or_die. Wrap it into std::string explicitly. --------- Co-authored-by: brpc-oncall <brpc-oncall@localhost> * fix(redis): avoid formatted command overread (apache#3570) Retry formatted values in dynamically sized storage when vsnprintf reports an output larger than the stack buffer. Add a vector-based RedisRequest component overload and use it for authentication and cluster command construction while retaining the existing format APIs. Add coverage for wide numeric formatting, binary-safe components, and AUTH/SELECT credential encoding. Verified with RedisCommandFormatTest.* and RedisClusterChannelTest.basic_routing_and_multi_key_commands. * Validate RPC response sockets (apache#3580) * Validate RPC response sockets Check that responses arrive on the socket used by the matching request attempt before updating the controller. Account for current attempts and unfinished backup requests across the four protobuf RPC protocols. Add Hulu response tests for matching and mismatched sockets while keeping direct response injection compatible with existing internal callers. * Expand RPC response socket test coverage Verify rejection and subsequent completion on the sending socket for baidu_std, sofa_pbrpc, and public_pbrpc response handlers. Exercise real channel/server calls across all four affected protocols and their supported connection types using an ephemeral server port. * Tighten rejected RPC response handling Accept the base correlation ID only for direct response injection without a sending socket. Real requests must match a versioned attempt ID. Reset all advertised streams on the arrival socket when rejecting a baidu_std response. Extend the regression test to check both rejection paths and successful completion by the valid response. * Fix timing assumptions in multidimensional latency recorder test (apache#3581) * Fix timing assumptions in multidimensional latency recorder test Wait for asynchronous sampling with a bounded five-second timeout instead of assuming a one-second sleep is sufficient. Check positive QPS without assuming a minimum rate of seven requests per second. Use nonfatal assertions for independent metrics and restore modified flags automatically on all test exit paths. * Preserve sampled metrics in latency recorder test Cache each nonzero latency, maximum latency, and QPS observation while polling. Assert the cached values so a sampler tick cannot clear the one-second windows between polling and the assertions. Include QPS in the readiness check and preserve successful observations independently when sampler ticks occur between metric reads. * fix(fuzzing): return 0 (not 1) from the size guard in every harness (apache#3579) LLVMFuzzerTestOneInput may only return 0 (input consumed) or -1 (input rejected); libFuzzer asserts this after every call (compiler-rt/lib/fuzzer/FuzzerLoop.cpp:622) and runs the empty input first, so every harness here aborts an assert-enabled libFuzzer at startup. Returning 0 keeps out-of-range inputs out of the corpus (the harness body is still skipped) and matches the convention newer harnesses already use. All 18 test/fuzzing/fuzz_*.cpp harnesses share the same guard. Signed-off-by: li-lizhe <147392333@qq.com> * Reject malformed mcpack input instead of CHECK-fatal in parser (apache#3576) * Reject malformed mcpack input instead of CHECK-fatal in parser The mcpack2pb parser used CHECK(false) to handle malformed input in unbox(), the object/array/isoarray iterators and the value conversion functions. CHECK(false) logs at FATAL level and aborts the process when -crash_on_fatal_log is on, so a single malformed nshead+mcpack request could terminate a server and drop all in-flight requests (CWE-617). Replace the wire-controlled CHECK(false) sites with LOG(ERROR) plus the existing error propagation (set_bad()/return 0, which the generated parsing code checks), and mark the stream bad in unbox() and the truncated as_string()/as_binary() paths so failed parses are reported by stream()->good() as before. The serializer and code-generator CHECKs are untouched since they are not driven by network input. * Avoid wire-controlled eager resize in as_string/as_binary Follow-up to the CHECK(false) hardening: the claimed value size of string/binary fields comes from the wire and can be close to UINT32_MAX. resize(size) before reading allocates that amount up front, so a tiny request could trigger an uncaught std::bad_alloc/std::length_error and still terminate the process. Read in bounded chunks and grow the output only for bytes that are actually present; a short read marks the stream bad as before. * Reject truncated primitive reads in cut_packed_pod The value-returning InputStream::cut_packed_pod<T>() left its result uninitialized when cutn() read fewer than sizeof(T) bytes, which is reachable when the enclosing object declares enough bytes but the real input ends inside a primitive value. The indeterminate value was then consumed or logged (UB), and the parse continued as if the input were complete. Zero-initialize the result, return a zero value and mark the stream bad on a short read, so the generated code fails the parse. The pointer version is unchanged: its callers already check the returned size. * Validate the trailing NUL when reading mcpack strings as_string() skipped the trailing byte with popn(1), which returns 0 on an exhausted stream and leaves it good. A string whose content was present but whose stream ended exactly before the required trailing '\0' (or ended with a non-NUL byte) was therefore accepted as valid. Read the terminator and reject the input when it is missing or not a NUL, marking the stream bad and clearing the output. * Avoid double copy of valid string/binary payload cut_bytes_to_string() copied every byte twice on valid input: once from the input into a stack buffer and once into the destination string. Grow the destination by at most one 8 KiB chunk at a time and cut directly into it, restoring the single-copy behavior of the original resize+cutn path while keeping the bounded allocation. --------- Co-authored-by: brpc-oncall <brpc-oncall@localhost> * Add latency recorder window statistics test (apache#3582) Exercise latency observations split across sampler ticks using a window longer than the bounded wait budget. Confirm the sampled count is complete before checking the sum and average from the saved snapshot, and verify the maximum through its independent sampler. * Fix GCC 16 compile Error (apache#3583) * Fix GCC 16 compile Error: 1.The -D__STRICT_ANSI__ in CMakeLists.txt conflicts with libstdc++ 16's __int128 specialization 2. H2StreamContext is incomplete, so sizeof throws an error, causing unique_ptr to fail * Fix GCC 16 compile Error: 1. Remove __STRICT_ANSI__ definition instead of probing it --------- Co-authored-by: panyfx <8242454+panyfx@user.noreply.gitee.com> * Validate Consul response object types (apache#3584) Check that each node and its Service value are JSON objects before using object accessors. Skip invalid entries while retaining valid services and reject responses containing only invalid entries. Add HTTP client/server regression coverage for non-object values at both boundaries, mixed valid and invalid entries, and stale result clearing. * Reject truncated length-encoded integers in MySQL reply parser (apache#3575) * Reject truncated length-encoded integers in MySQL reply parser parse_encode_length did not check the return value of IOBuf::cutn, so a truncated 0xFC/0xFD/0xFE prefix left part of the stack buffer uninitialized and mysql_uint*korr interpreted that garbage as the value. A malicious server could turn it into bogus column counts, field lengths or loop counts. Check cut1/cutn and return -1 on truncated or invalid input; all call sites now fail with PARSE_ERROR_ABSOLUTELY_WRONG and the six duplicated column-string blocks are folded into one helper. * Bound MySQL packet parsing to the packet payload Address review feedback on the length-encoded integer hardening: 1. parse_header now optionally cuts the packet payload into a separate IOBuf and every sub-parser decodes from that bounded buffer, so a truncated field can no longer borrow bytes from a coalesced next packet and silently desync the stream. 2. parse_encode_length returns bool with a uint64_t out-param instead of an int64_t sentinel, which rejected legitimate values above INT64_MAX (e.g. a 2^64-1 affected-rows count in OK packets). Add regression tests for a truncated prefix followed by a coalesced packet and for the maximum encodable affected-rows value. * Fix MySQL reply dispatch for multi-byte column counts Address review feedback: the dispatcher matched the first payload byte against synthetic MysqlRspType values, so a result set whose column count used the multi-byte 0xFC form (252..65535 columns) was misclassified as a prepare-ok and fed to unchecked fixed-width header reads; 0xFB/0xFD counts hit the unknown-type fallback; and every 0xFE-leading packet was treated as EOF although MySQL only treats a SHORT (< 9 payload bytes) 0xFE packet as EOF. - Dispatch fresh wire bytes 0x01..0xFE to the result-set parser; prepare-ok resume is keyed on |_type| instead of the wire byte. - is_an_eof and the dispatcher now require payload < 9 for EOF, so a row starting with an 8-byte length-encoded value is a row. - All remaining unchecked fixed-width cuts (prepare-ok header, column fixed fields, OK status/warnings, EOF, ERR sql-state, auth fixed fields and NUL-terminated strings, binary row/field values, binary TIME/DATETIME) go through a parse_fixed helper that rejects short reads instead of using uninitialized stack memory. Tests: 0xFC/251-column result sets parse; truncated 0xFC count, huge 0xFE count, 0xFE-leading row and truncated prepare-ok are rejected; standalone EOF and a full prepare-ok still parse. * Add truncated greeting tests for MySQL auth parser Cover the review scenario directly: a greeting ending before the 4-byte thread id (previously left tmp partially uninitialized and continued) and a greeting with a non-NUL-terminated server version are both rejected. * Reject bare 0xFB header and zero column count in MySQL result sets A bare 0xFB is the length-encoded NULL marker, never a legal column count (251 is encoded as FC FB 00). The widened fresh-dispatch range accepted it as a zero-column result set when followed by two EOF packets. Exclude 0xFB from result-set dispatch (it falls to the unknown-type rejection) and reject an explicitly encoded zero column count in ResultSetHeader::Parse; a result set always carries at least one column. * Fix greeting test NUL terminator and prefix-failure diagnostics - RejectTruncatedGreeting copied only 11 of the 12 bytes of "5.7.99-fake\x00", so it failed at the cut_until delimiter check instead of exercising the truncated thread-id parse_fixed path it was written for. Copy the terminating NUL. - parse_column_string now distinguishes a truncated/invalid length prefix from an oversized length in its log message instead of reporting a misleading "length 0 exceeds ...". - Rename AcceptSingleByte251ColumnCount to AcceptMultiByte251ColumnCount to match the three-byte encoding it actually verifies. * Validate column/auth/ERR/prepare-ok filler bytes in MySQL parser Remaining unchecked pop_front calls on fixed filler fields silently removed zero bytes from a truncated packet and accepted it as parsed: the column definition's filler-length byte and final 2 reserved bytes, the auth greeting's 10 reserved bytes, the ERR packet's '#' sql_state marker (now also validated) and the prepare-ok header's filler byte. Consume them through parse_fixed so a truncated tail is rejected. Add tests for a column definition missing 1..3 tail bytes and for an ERR packet without the '#' marker. * Accept pre-4.1 initial-handshake ERR packets in MySQL parser An ERR packet sent before capabilities are negotiated (e.g. the 'Too many connections' error) carries no '#' and sql_state; the unconditional marker check rejected these legitimate packets during authentication. Peek for the '#' marker instead (as MySQL clients do): when present, parse the protocol-4.1 layout and keep rejecting a truncated 5-byte sql_state; otherwise treat the whole tail as the message with an empty sql_state. This also drops the pre-existing payload_size >= 9 guard, which rejected short pre-4.1 errors, and skips the message allocation for an empty tail. Fixes a fetch misuse found on the way: IOBuf::fetch returns a pointer into its own storage when the range fits one block, so the marker must be read through the returned pointer, not the aux buffer. Tests: initial-handshake ERR parses with the full message, 4.1 ERR keeps its sql_state, and a truncated sql_state is still rejected. * Select MySQL ERR layout from connection phase, not message content The '#' sniff could not distinguish a pre-4.1 initial-handshake error from a 4.1 error whose message starts with '#': '#quota exceeded' was misparsed as sql_state 'quota' + message ' exceeded', and a short '#bad' message was rejected as a truncated sql_state. Thread a protocol41 flag from ParseMysqlMessage through ConsumePartialIOBuf into Error::Parse: it is false only while the server greeting has not been processed yet (per-connection AuthContext group still empty), which is exactly when ERR packets use the pre-4.1 layout ('Too many connections'); after the HandshakeResponse41 and in the command phase the 4.1 layout ('#' + sql_state) is required. Tests: legacy messages starting with '#' (long and short) keep their message intact with an empty sql_state, a 4.1 error whose message starts with '#' still parses marker + sql_state, plus the existing initial-handshake / truncated-sql-state / plain 4.1 controls. * Keep legacy MySQL ConsumePartialIOBuf symbols for ABI compatibility A defaulted protocol41 parameter preserves source compatibility but changes the mangled symbol, breaking prebuilt clients that link against the old signatures. Restore both legacy signatures as out-of-line wrappers that forward protocol41=true, and drop the default argument from the new overloads so overload resolution stays unambiguous. * Add binary-row truncation regression tests for MySQL parser The short-read hardening of the binary protocol path (NULL bitmap, fixed-width values, string/TIME/DATETIME lengths) had no coverage: all unit tests used the text protocol and the prepared-statement integration tests need a live server. Add synthetic MYSQL_PREPARED_STATEMENT result sets with a valid control row plus truncated NULL bitmap, truncated LONGLONG value, truncated string length prefix/value and truncated TIME/DATETIME values, each followed by a coalesced EOF packet to verify that decoding never borrows bytes from the next packet. * Fix hex-escape over-read in binary string-field test The \x00ab escape greedily consumes the following hex digits, so the literal held 4 bytes while std::string(str, 5) copied five -- a global-buffer-overflow caught by the ASan CI build. Split the literal after \x00 so the escape terminates at the quote; the adjacent-literal concatenation yields the intended 5 bytes fc 05 00 'a' 'b'. --------- Co-authored-by: brpc-oncall <brpc-oncall@localhost> * Reject truncated consistent hashing replica keys (apache#3585) Check snprintf results before hashing replica keys in the default and Ketama policies. Reject formatting errors and truncated keys without partially updating the hash ring. Keep existing mappings for keys that fit the buffer. Cover key-length boundaries, batch additions, and disabled tag hashing for Murmur3, MD5, and Ketama. * Fix empty %b/%s argument dropped in redis command formatting (apache#3566) * Fix empty %b/%s argument dropped in redis command formatting (apache#2275) * Fix %s/%b variadic argument type mismatch in RedisCommandFormatV * Fix %b binary test to compare full payload bytes instead of ASSERT_STREQ * Tighten va_arg const char* comment in RedisCommandFormatV * Return false instead of CHECK-abort on AddCommand format errors * Post small RDMA messages inline (apache#3569) * Post small RDMA messages inline CutFromIOBufList() posts every message by reference, so the NIC has to read a small RPC from host memory before sending it. Request 236 bytes of inline data at QP creation, retrying without it if the device refuses, and inline a message that fits the granted size and comes entirely from the RDMA block pool. 236 bytes is the largest Send whose WQE fits the 256-byte BlueFlame buffer of current mlx5 NICs. Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Refine the inline size limit and the QP creation fallback The 236-byte limit keeps an inlined Send within the 256-byte BlueFlame buffer of mlx5 NICs. Apply it only on Mellanox/NVIDIA devices (vendor ID 0x02c9, kept from the device query GlobalRdmaInitializeOrDie already does); on other devices the inline size the QP was granted is the limit. If QP creation with 236 bytes of inline data fails, try 64 bytes before falling back to none. irdma (Intel E810) rejects requests above 101 bytes, so on those NICs nothing was inlined. Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Try known inline limits, then step down After a refused 236-byte inline request, QP creation was retried with 64 bytes, then with none. Devices take less than 236 by different amounts: Intel's irdma refuses more than 216, 101 or 48 bytes depending on the generation (101 on an E810, 48 on an X722), Alibaba's erdma more than 96. With 64 as the only middle step, an E810 or erdma QP got 64 bytes, a newer irdma QP 64 instead of 216, and an X722 QP none. The verbs API cannot report the limit, so step down to it. A device whose kernel driver has a fixed limit, found by the vendor ID kept from the device query GlobalRdmaInitializeOrDie already does, starts there: 216, then 101, then 48 on Intel, 96 on Alibaba, each capped at 236. Other devices start at 236. Each refused size is followed by the next one 16 bytes smaller, down to 0, so a failure unrelated to inline data still ends as before, after a few more attempts at QP creation. Devices that accept 236 (mlx5, rxe) are unchanged. libfabric's verbs provider also probes the limit by trial QP creation (vrb_find_max_inline()). Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Step down the inline size only on EINVAL, and add unit tests AllocateQp() retried with a smaller inline size after any ibv_create_qp failure, so a failure unrelated to inline data, such as ENOMEM or the device's QP limit, led to up to 16 failed calls, and the caller's PLOG reported the errno of the last attempt. Step down only when the failed call left errno == EINVAL, which is how providers refuse an inline size; any other failure is returned at once with its errno. Each step is logged under FLAGS_rdma_trace_verbose. QP creation moves into CreateQpWithInlineData(), which takes the vendor ID as a parameter. It and InlineDataToRequest() are no longer static, so that brpc_rdma_unittest can test them with a stubbed IbvCreateQp: the first request per vendor, the Mellanox cap at 236 bytes, Intel 216, 101 and 48, Alibaba 96, an unknown device stepping down to 92, a non-EINVAL failure stopping at once, and a device refusing every size ending at 0. Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Factor the inline send decision out and test it CutFromIOBufList() inlines a message only when it fits the inline size the QP was granted and all of its blocks come from the block pool; user registered memory may be device memory, which the CPU cannot copy from. Move that decision into ShouldPostInline(), exposed for UT, and test it in brpc_rdma_unittest: a pool-backed message at the granted size is inlined, one byte more is not, and a message in user registered memory never is, however small. CutFromIOBufList() itself needs an initialized RDMA device: it returns early in the unit test binary, sizes its SGE list from the device and looks blocks up in the registered block pool. So the decision is tested on its own; the send path is otherwise unchanged. Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Reuse the inline size of the last QP for new QPs All QPs are created on the same device with the same attributes apart from their queue sizes, so a device that refuses the first inline size made every one of the rdma_prepared_qp_cnt QPs repeat the same step-down (216, 101, 48 on an Intel device limited to 48 bytes). Remember the size the last QP was created with and start there. If that size is refused, the EINVAL step-down continues from it as before. Each QP still takes its own granted size from attr.cap.max_inline_data. Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> * Reject deeply nested JSON replies in naming services (apache#3592) * Reject deeply nested JSON replies in naming services The consul, nacos and discovery naming services parsed registry replies with rapidjson's default recursive parser, so a deeply nested reply crashed the consuming process with stack exhaustion. rapidjson also builds, visits and destroys its DOM recursively, so iterative parsing alone is not enough. Route all the reply parsing through a shared ParseNamingServiceJson() which parses iteratively (kParseIterativeFlag) and rejects replies nested deeper than kMaxNamingServiceJsonDepth (100, same default as json2pb) before parsing, bounding every recursion on the document. Legit replies of these services are only a few levels deep. * Address review: reject raw NUL and clamp depth in JSON pre-scan - A raw NUL is invalid JSON anywhere and rapidjson treats it as end-of-input, so reject replies containing one up front: the depth scan and the parser now accept exactly the same bytes. - Don't let the depth counter go negative on malformed text with more closing than opening brackets. - Reword the comment about the state of `doc' when the depth limit is exceeded: it is untouched rather than cleared. --------- Co-authored-by: brpc-oncall <brpc-oncall@localhost> * reject CR/LF in SetHttpURL and SetH2Path (apache#3588) Both parsers copied control characters into _host/_path/_query/_fragment, which MakeRawHttpRequest and GenerateH2Path then wrote verbatim into the Host header, the request line and the HTTP/2 :path. * Unify transport handshakes on current upstream master * Retry transport handshake CI validation --------- Signed-off-by: li-lizhe <147392333@qq.com> Co-authored-by: Bright Chen <chenguangmingfe@foxmail.com> Co-authored-by: Weibing Wang <wwbmmm@gmail.com> Co-authored-by: brpc-oncall <brpc-oncall@localhost> Co-authored-by: Xiaofeng Wang <wasphin@gmail.com> Co-authored-by: darion-yaphet <darion.yaphets@gmail.com> Co-authored-by: UB <ubed@bugqore.com> Co-authored-by: li-lizhe <82501526+li-lizhe@users.noreply.github.com> Co-authored-by: pynj2026 <159674473+pynj2026@users.noreply.github.com> Co-authored-by: panyfx <8242454+panyfx@user.noreply.gitee.com> Co-authored-by: NeutralMilkHotel <wengshishuai@163.com> Co-authored-by: Andrei <46914650+alxrxs@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: sahvx655-wq <sahvx655@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
src/brpc/policy/naming_service_json.h:ParseNamingServiceJson()parses replies iteratively (kParseIterativeFlag) and rejects, before parsing, replies nested deeper thankMaxNamingServiceJsonDepth(100, the same default asjson2pb_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.Document::Parsecall 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).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.cppaddsnaming_service_json_depth_limit(valid/malformed replies, brackets inside strings, boundary at depth 100/101, 140000-deep reply) plus end-to-end regressionsconsul/nacos/discovery_deeply_nested_replythat 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