wwbmmm opened a new pull request, #3576: URL: https://github.com/apache/brpc/pull/3576
### What problem does this PR solve? Issue Number: resolve # Problem Summary: The mcpack2pb parser handled malformed input with `CHECK(false)` in `unbox()`, the object/array/isoarray iterators, and the `UnparsedValue` conversion functions. `CHECK(false)` logs at FATAL level and calls `abort()` when `-crash_on_fatal_log` is on, so a single malformed nshead+mcpack request could terminate the whole server process and drop all concurrently processed requests (CWE-617, reachable assertion). This violates the requirement in THREAT_MODEL.md that protocol parsers must not crash on malformed input, and is inconsistent with other brpc parsers (HTTP, baidu_std) which reject malformed input with `LOG(ERROR)` + an error code. ### What is changed and the side effects? Changed: - All wire-controlled `CHECK(false)` sites in `src/mcpack2pb/parser.cpp` and `src/mcpack2pb/parser-inl.h` (unbox, ObjectIterator/ArrayIterator/ISOArrayIterator error paths, as_int64/as_uint64/as_int32/as_uint32/as_bool/as_float/as_double type-mismatch and overflow paths, as_string/as_binary truncation paths) now use `LOG(ERROR)` and propagate the error through the existing `set_bad()` / return-0 mechanisms, which the generated parsing code already checks. `NsheadMcpackAdaptor::ParseRequestFromIOBuf` then fails the request with `EREQUEST` instead of killing the process. - `unbox()` and the truncated `as_string()`/`as_binary()` paths additionally mark the stream bad so failed parses are visible via `stream()->good()`, mirroring the other error paths. - The serializer (`serializer.cpp`) and code generator (`generator.cpp`) `CHECK`s are untouched: they are driven by valid protobuf messages and proto definitions, not by network input. Side effects: - Performance effects: none on the happy path; only error paths changed. - Breaking backward compatibility: none. Public interfaces are unchanged; input that previously aborted the process (or logged FATAL) is now rejected with `LOG(ERROR)` and a failed parse, which is the documented behavior for malformed input. ### Check List: - Added 10 unit tests in `test/brpc_mcpack2pb_unittest.cpp` covering each previously fatal path: truncated/non-object/named top-level object in `unbox`, object/array field or item beyond the declared buffer, truncated string data, array payload smaller than the items header, non-primitive and size-inconsistent isomorphic arrays, and float-read-as-integer. All 19 tests in the binary pass after the fix; the new tests hit the FATAL `Check failed: false` log before the fix. - Full `make -j6` with `BUILD_UNIT_TESTS=ON` succeeds; no new warnings in the touched files. --- 🤖 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]
