wwbmmm opened a new pull request, #3560: URL: https://github.com/apache/brpc/pull/3560
### What problem does this PR solve? Issue Number: resolve #N/A (reported directly to [email protected]) Problem Summary: The redis reply parser enforced `-redis_max_allocation_size` (default 64 MiB) per array allocation and `-redis_max_reply_depth` (default 128) per nesting level, but nothing bounded the two together. A reply nesting one maximally-sized array inside another committed `depth * 64 MiB` of resident memory from a few bytes of wire data (measured ~8 GiB from 1,280 bytes on master), and since the parse ends in `PARSE_ERROR_NOT_ENOUGH_DATA`, the half-built tree is retained on the socket as long as the peer keeps the connection open. A related issue in `RedisCommandParser::ConsumeImpl()`: a declared RESP array count (`*<count>\r\n`, ~12 bytes) immediately `resize()`d `_args`, committing up to `-redis_max_allocation_size` per connection before any argument data arrived. ### What is changed and the side effects? Changed: - `RedisReply::ConsumePartialIOBuf()` now threads a per-reply byte budget through the recursion: every array allocation is charged against `-redis_max_allocation_size` for the whole reply tree, so nested arrays draw from one shared budget instead of each getting a full one. When the parsing of a reply suspends (`PARSE_ERROR_NOT_ENOUGH_DATA`), the budget consumed so far is saved in the root array and restored on resume, so a peer cannot reset the budget by withholding data and resuming with a fresh one. The saved counter lives in previously-unused padding of the array struct, so `sizeof(RedisReply)` and the wire format are unchanged. - `RedisCommandParser` no longer pre-resizes `_args` from a declared count; the vector grows as arguments actually arrive, so an incomplete command only costs memory proportional to the data received. Complete commands produce exactly the same argument list as before. Side effects: - Performance effects: one extra pointer parameter and an integer add per array allocation; negligible. - Breaking backward compatibility: replies whose *total* array storage exceeds `-redis_max_allocation_size` are now rejected with `PARSE_ERROR_ABSOLUTELY_WRONG` (previously each nesting level was allowed a full cap). This matches the flag's documented meaning ("for a single redis request or reply"); deployments that legitimately need larger replies can raise the flag. No public API or wire-format change. ### Check List: - Compilable: yes, built with `cmake -DBUILD_UNIT_TESTS=ON` and `make -j6`. - Tests added in `test/brpc_redis_unittest.cpp`: - `redis_reply_rejects_nested_array_memory_amplification`: nested maximal arrays rejected (single-shot, across a suspend/resume, and fed one header per call); valid nested replies and a flat array exactly at the cap still parse. - `command_parser_does_not_preallocate_declared_args`: a declared count no longer commits the full `_args` vector; complete commands unchanged. - All cases in `test/brpc_redis_unittest` (20, including live redis-server integration) and `test/brpc_redis_cluster_unittest` (40) pass. --- 🤖 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: [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]
