Contain malformed Thrift T_EXCEPTION replies on the client - #3500
Conversation
The Thrift client decodes a T_EXCEPTION reply in ReadThriftException without catching what the underlying Thrift library throws. A reply carrying a malformed exception struct (e.g. a field with an oversized length) makes TBinaryProtocol raise an exception that unwinds through ProcessThriftResponse up to the bthread task frame, where std::terminate() is called, killing the whole process and every other in-flight RPC on it. Wrap the decode in the same try/catch that ReadThriftStruct already uses for the normal reply body, so a malformed T_EXCEPTION reply degrades to a failed parse instead of crashing the process. Add a unit test feeding a malformed T_EXCEPTION reply through the client response path.
There was a problem hiding this comment.
Pull request overview
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.
Prevents a bRPC Thrift client from terminating the process when receiving a malformed T_EXCEPTION reply by containing exceptions thrown during exception-struct decoding, and adds a regression test to verify the behavior.
Changes:
- Wraps
T_EXCEPTIONdecoding inReadThriftExceptionwithtry/catchto prevent uncaught exceptions from reaching the bthread frame. - Adds a unit test that injects a malformed
T_EXCEPTIONreply to ensure the client does not crash and the controller is marked failed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/brpc_thrift_protocol_unittest.cpp | Adds regression tests that feed a malformed T_EXCEPTION reply through the client response path. |
| src/brpc/policy/thrift_protocol.cpp | Adds exception containment around T_EXCEPTION parsing to avoid process termination on malformed replies. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Incorporate review feedback: - Catch std::exception by const ref and fix 'Catched' -> 'Caught' in the T_EXCEPTION reply parse log messages. - Add <unistd.h> and close the pipe fds / release the socket in TearDown() so the unit test does not leak OS resources across tests.
The test socket owns the pipe write end passed in SocketOptions::fd and closes it on destruction, so TearDown must only close the read end to avoid double-closing (and potentially closing a recycled fd).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
test/brpc_thrift_protocol_unittest.cpp:72
- The test fixture does setup work in the constructor using non-fatal EXPECT_* checks. If any of these steps fails (pipe/socket create/address), the test continues with partially-initialized state and can also leak
_pipe_fds[1](the write end) when Socket::Create fails (because TearDown assumes the socket owns it). Moving setup to SetUp() with ASSERT_* and only closing fds that are still owned makes failures deterministic and prevents fd leaks in partial-setup scenarios.
ThriftProtocolTest() {
_pipe_fds[0] = _pipe_fds[1] = -1;
EXPECT_EQ(0, pipe(_pipe_fds));
brpc::SocketId id;
brpc::SocketOptions options;
options.fd = _pipe_fds[1];
EXPECT_EQ(0, brpc::Socket::Create(options, &id));
EXPECT_EQ(0, brpc::Socket::Address(id, &_socket));
}
| ] + select({ | ||
| # The thrift framed protocol (and the unit test that exercises it) is only | ||
| # compiled when the build has Thrift support enabled, matching src/BUILD. | ||
| "//bazel/config:brpc_with_thrift": ["-DENABLE_THRIFT_FRAMED_PROTOCOL=1"], | ||
| "//conditions:default": [], | ||
| }) |
There was a problem hiding this comment.
It seems this select is not needed. unit test targets will inherit this definition of bRPC source targets automatically.
Lines 45 to 48 in 4a429cd
What problem does this PR solve?
Problem Summary:
A bRPC Thrift client can be brought down by a peer reply that carries a
malformed
T_EXCEPTION. The exception struct is decoded inReadThriftExceptionwithout a try/catch; a bad field or an oversizedlength makes the underlying thrift library throw (
TProtocolException,TTransportExceptionorstd::length_error). The exception is not caughtanywhere on the return path and eventually reaches the bthread task frame,
where
std::terminate()is called, terminating the whole process.This only affects builds with
WITH_THRIFTenabled (off by default).What is changed and the side effects?
Changed:
ReadThriftExceptionnow wraps the decode of the exception struct in thesame
try/catchthatReadThriftStructalready uses for the normal replybody. A malformed
T_EXCEPTIONreply is reported as a failed RPC insteadof crashing the process.
Side effects:
None. The fix only contains an otherwise-uncaught exception.
Performance effects: none.
Breaking backward compatibility: none.
Check List
test/brpc_thrift_protocol_unittest.cppthat feeds a malformed
T_EXCEPTIONreply through the clientresponse path and asserts the call does not crash and marks the
controller failed.