Reject malformed mcpack input instead of CHECK-fatal in parser by wwbmmm · Pull Request #3576 · apache/brpc

wwbmmm · 2026-10-01T02:51:45Z

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) CHECKs 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

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.

Copilot

Copilot review overview

🟡 Changes recommended

Wire-controlled string and binary sizes can still trigger process-ending allocation exceptions before truncation checks run.

Review effort: Balanced
Findings: 1

Open (1)

Avoid wire-controlled allocation before validating truncated input · New

What changed in this PR

Hardens mcpack parsing so malformed wire input produces parse errors instead of fatal assertions.

Changes:

Replaces wire-triggerable CHECK(false) paths with error propagation.
Marks truncated conversions as bad streams.
Adds malformed-input regression tests.

File	Description
`src/mcpack2pb/parser.cpp`	Converts parser failures to recoverable errors.
`src/mcpack2pb/parser-inl.h`	Hardens array iterator error paths.
`test/brpc_mcpack2pb_unittest.cpp`	Adds malformed-input coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

Copilot

Copilot review overview

🟡 Changes recommended

Truncated primitive reads and attacker-controlled string sizes can still cause undefined behavior or process-terminating allocations.

Review effort: Balanced
Findings: 1

Open (1)

Short primitive reads use uninitialized values · New

Resolved since last review (1)

Avoid wire-controlled allocation before validating truncated input

wwbmmm · 2026-10-01T04:08:50Z

+        LOG(ERROR) << "Can't set float=" << _stream->cut_packed_pod<float>()
                     << " to " << var;
        _stream->set_bad();
        return 0;
    case PRIMITIVE_FIELD_DOUBLE:
-        CHECK(false) << "Can't set double=" << _stream->cut_packed_pod<double>()
+        LOG(ERROR) << "Can't set double=" << _stream->cut_packed_pod<double>()
                     << " to " << var;
        _stream->set_bad();


[replied by brpc-oncall robot] Valid point — the value-returning cut_packed_pod<T>() returned an uninitialized local when cutn() short-read, so truncated primitive payloads were consumed as garbage values (and logged, which is UB) without failing the parse.

Fixed in the latest commit: cut_packed_pod() now zero-initializes its result, and on a short read it returns a zero value and marks the stream bad, so the generated code fails the parse via stream()->good() at all five call sites you listed (as_int64/as_uint64/as_int32/as_uint32/as_bool, plus as_float/as_double). The pointer version cut_packed_pod(T*) is unchanged since its callers (e.g. unbox(), the iterators) already check the returned size themselves. Added regression tests Int32FieldTruncatedPayloadIsRejected and FloatFieldTruncatedPayloadIsRejected: the int32 case fails on the previous code (a 1-byte truncated value was returned as 42 with good() == true) and passes now.

[replied by brpc-oncall robot] This finding is fixed in b0573d2: the value-returning cut_packed_pod<T>() in src/mcpack2pb/parser-inl.h now zero-initializes its result and, on a short read, returns a zero value and marks the stream bad (the latest review confirms approval with only this stale thread still open, since the diff hunk it anchors to in parser.cpp was not touched by the fix). Regression tests Int32FieldTruncatedPayloadIsRejected and FloatFieldTruncatedPayloadIsRejected cover it. Could you resolve the thread?

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.

Copilot

Copilot review overview

🔵 Needs a closer look

Truncated floating-point diagnostics can evaluate indeterminate values, and truncated string terminators can remain undetected.

Review effort: Balanced
Findings: 1

Open (1)

Short primitive reads use uninitialized values

Previously missed (1)

In code that hasn't changed since last review

Validate the trailing NUL when reading strings

src/mcpack2pb/parser.cpp:626

The helper verifies only the _size - 1 content bytes. If the stream ends exactly before the required trailing NUL, the unchecked popn(1) returns 0 and the stream remains good, so a truncated string is accepted. Read and validate the terminator, and mark the stream bad on either a short read or a non-NUL byte.

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.

Copilot

Copilot review overview

🔵 Needs a closer look

String terminator truncation remains unchecked, and valid string parsing now performs an avoidable extra copy.

Review effort: Balanced
Findings: 1

Open (1)

Short primitive reads use uninitialized values

Previously missed (2)

In code that hasn't changed since last review

Detect truncated terminator byte and clear output

src/mcpack2pb/parser.cpp:626

A string truncated only at its trailing byte is not marked bad here: the helper successfully reads _size - 1, then the unchecked popn(1) below returns 0 while InputStream::good() remains true. This leaves as_string() reporting a successful conversion until an enclosing iterator happens to advance, contrary to the new truncation behavior. Check the terminator-byte pop, mark the stream bad, and clear the output on a short read.

Avoid extra copy when appending parsed data

src/mcpack2pb/parser.cpp:610

This append adds an extra full copy on every valid string/binary parse: each byte is first copied from the input into the stack buffer and then copied again into the destination, whereas the previous path copied directly into the destination. The bounded-allocation hardening can retain one-copy behavior by growing the destination by at most one 8 KiB chunk and cutting directly into that chunk.

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.

Copilot

Copilot review overview

🟡 Changes recommended

A string missing only its terminator can still leave the stream marked good.

Review effort: Balanced
Findings: 1 · 1

Open (2)

Short primitive reads use uninitialized values
Detect missing string terminator after declared data · New

Copilot

Copilot review overview

🟢 Approval recommended

The parser now consistently propagates malformed-input failures, with comprehensive regression coverage for the affected paths.

Review effort: Balanced
Findings: 1

Open (1)

Short primitive reads use uninitialized values

Resolved since last review (1)

Detect missing string terminator after declared data

wasphin

LGTM

wwbmmm requested a balanced review from Copilot October 1, 2026 02:52

Copilot started reviewing on behalf of wwbmmm October 1, 2026 02:52 View session