Copilot commented on code in PR #3631:
URL: https://github.com/apache/brpc/pull/3631#discussion_r4349120291
##########
test/brpc_mongo_protocol_unittest.cpp:
##########
@@ -240,6 +240,27 @@ TEST_F(MongoTest, verify_request) {
msg->Destroy();
}
+TEST_F(MongoTest, process_request_after_parsing_context_reset) {
+ brpc::mongo_head_t header = { 0, 0, 0, 0 };
+ header.op_code = brpc::MONGO_OPCODE_REPLY;
+ header.message_length = sizeof(header) + EXP_REQUEST.length();
+ butil::IOBuf total_buf;
+ total_buf.append(static_cast<const void*>(&header), sizeof(header));
+ total_buf.append(EXP_REQUEST);
+ brpc::ParseResult req_pr = brpc::policy::ParseMongoMessage(
+ &total_buf, _socket.get(), false, &_server);
+ ASSERT_EQ(brpc::PARSE_OK, req_pr.error());
+ // The bthread parsing messages may reset the parsing context, e.g. before
+ // trying other protocols, ahead of the request being processed.
+ _socket->reset_parsing_context(nullptr);
+ ProcessMessage(brpc::policy::ProcessMongoRequest, req_pr.message(), false);
+ // The request is still processed with the context it was parsed with.
+ ASSERT_EQ(0ll, _server._nerror_bvar.get_value());
+ int bytes_in_pipe = 0;
+ ioctl(_pipe_fds[0], FIONREAD, &bytes_in_pipe);
Review Comment:
The test doesn't check the return value of `ioctl()`. If `ioctl` fails, the
assertion on `bytes_in_pipe` will fail with a less-informative message and may
read an unchanged value. Consider asserting `ioctl(...) == 0` (and optionally
printing `errno`) before checking `bytes_in_pipe`.
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -212,15 +239,15 @@ void ProcessMongoRequest(InputMessageBase* msg_base) {
const Server::MethodProperty *mp =
server_accessor.FindMethodPropertyByFullName(
srv_des->method(0)->full_name());
- MongoContextMessage* context_msg =
- dynamic_cast<MongoContextMessage*>(socket->parsing_context());
- if (nullptr == context_msg) {
+ if (nullptr == msg->context) {
LOG(WARNING) << "socket context wasn't set correctly";
return;
}
SendMongoResponse* mongo_done = new SendMongoResponse(server);
- mongo_done->cntl.set_mongo_session_data(context_msg->context());
+ // The controller takes its own strong reference of the context, which
+ // outlives this message.
+ mongo_done->cntl.set_mongo_session_data(msg->context.get());
Review Comment:
The comment claims `Controller` takes a strong reference, but the code
passes a raw pointer from an `intrusive_ptr`. If `set_mongo_session_data()`
does not increment the refcount internally (or otherwise take ownership),
`msg->context` may be released at the end of `ProcessMongoRequest`, causing a
use-after-free later. Prefer passing an owning/reference-counted handle (if the
API supports it), or make the ownership/refcount contract explicit and enforced
(e.g., ensure `set_mongo_session_data()` increments the refcount or stores an
`intrusive_ptr`).
##########
src/brpc/policy/mongo_protocol.cpp:
##########
@@ -161,13 +183,18 @@ ParseResult ParseMongoMessage(butil::IOBuf* source,
socket_context_msg = new MongoContextMessage(context);
socket->reset_parsing_context(socket_context_msg);
}
- policy::MostCommonMessage* msg = policy::MostCommonMessage::Get();
+ MongoInputMessage* msg = MongoInputMessage::Get();
source->cutn(&msg->meta, sizeof(buf));
size_t act_body_len = source->cutn(&msg->payload, body_len - sizeof(buf));
if (act_body_len != body_len - sizeof(buf)) {
CHECK(false); // Very unlikely, unless memory is corrupted.
return MakeParseError(PARSE_ERROR_TRY_OTHERS);
}
+ MongoContextMessage* context_msg =
+ dynamic_cast<MongoContextMessage*>(socket_context_msg);
+ if (nullptr != context_msg) {
+ msg->context = context_msg->context();
+ }
Review Comment:
`msg->context` is only set when the socket parsing context is already a
`MongoContextMessage`. When it's not (e.g., stale/foreign context),
`ProcessMongoRequest` will warn and drop the request. To make the protocol more
robust and keep the new message-level context guarantee, consider treating a
non-mongo `socket_context_msg` as 'no context' here: create a new
`MongoContextMessage`, store it on the socket, and always attach `msg->context`
when returning `PARSE_OK`.
##########
test/brpc_socket_unittest.cpp:
##########
@@ -2130,6 +2132,95 @@ TEST_F(SocketTest, shutdown_write) {
}
}
+// A parsing context taking a while to be described, during which the bthread
+// parsing messages of the socket may reset the context.
+class SlowDescribedContext : public brpc::Destroyable, public
brpc::Describable {
+public:
+ SlowDescribedContext(bthread::CountdownEvent* describing,
+ butil::atomic<int>* ndestroyed)
+ : _describing(describing)
+ , _ndestroyed(ndestroyed)
+ , _name("SlowDescribedContext") {}
+
+ void Describe(std::ostream& os, const brpc::DescribeOptions&) const
override {
+ _describing->signal();
+ // Not bthread_usleep(): DebugSocket() describes under a pthread mutex.
+ usleep(100000);
Review Comment:
This hard-coded 100ms sleep slows the unit test suite and can be a source of
flakiness/overhead in CI. Since the test already uses a `CountdownEvent` to
coordinate, consider reducing the sleep significantly or using a deterministic
synchronization signal (e.g., a second event/flag that confirms the reset
thread is attempting to reset) to ensure overlap without a large fixed delay.
##########
src/brpc/socket.cpp:
##########
@@ -2427,17 +2427,20 @@ void Socket::DebugSocket(std::ostream& os, SocketId id)
{
os << idsizes[i];
}
os << '}';
- Destroyable* const parsing_context = ptr->parsing_context();
- Describable* parsing_context_desc =
dynamic_cast<Describable*>(parsing_context);
- if (parsing_context_desc) {
- os << "\nparsing_context=" << butil::class_name_str(*parsing_context)
<< '{';
- DescribeOptions opt;
- opt.verbose = true;
- IndentingOStream os2(os, 2);
- parsing_context_desc->Describe(os2, opt);
- os << '}';
- } else {
- os << "\nparsing_context=" << ShowObject(parsing_context);
+ {
+ BAIDU_SCOPED_LOCK(ptr->_parsing_context_mutex);
+ Destroyable* const parsing_context = ptr->parsing_context();
+ Describable* parsing_context_desc =
dynamic_cast<Describable*>(parsing_context);
+ if (parsing_context_desc) {
+ os << "\nparsing_context=" <<
butil::class_name_str(*parsing_context) << '{';
+ DescribeOptions opt;
+ opt.verbose = true;
+ IndentingOStream os2(os, 2);
+ parsing_context_desc->Describe(os2, opt);
+ os << '}';
+ } else {
+ os << "\nparsing_context=" << ShowObject(parsing_context);
+ }
Review Comment:
This holds `_parsing_context_mutex` while calling a virtual `Describe()`
implementation, which can be arbitrarily slow (and can block the parsing
bthread from resetting/releasing the context for the duration). Since the lock
exists primarily for lifetime safety, consider reducing lock hold time by
introducing a safe lifetime mechanism (e.g., refcounted parsing contexts for
describable contexts, or copying/snapshotting the describable state under the
lock and describing outside the lock) and/or explicitly documenting that
`Describe()` must not block or call back into socket code to avoid deadlocks.
##########
src/brpc/socket.h:
##########
@@ -950,6 +950,12 @@ friend class TransportFactory;
// Saved context for parsing, reset before trying other protocols.
butil::atomic<Destroyable*> _parsing_context;
+ // DebugSocket() describes _parsing_context outside the bthread parsing
+ // messages, which may reset or release the context at the same time.
+ // Both hold this mutex, so that the context is not destroyed while
+ // being described. initialize_parsing_context() does not need it: it
+ // never destroys a context set in the socket.
+ butil::Mutex _parsing_context_mutex;
Review Comment:
Adding a new data member to `Socket` changes its object
layout/`sizeof(Socket)` and is an ABI break for any external code that is built
against a different brpc version than it links against. If brpc intends to
preserve ABI compatibility, consider moving this mutex behind an indirection
(e.g., into an existing impl/shared struct) or otherwise updating the PR
description/release notes to reflect the ABI impact.
--
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]
