Copilot commented on code in PR #3631:
URL: https://github.com/apache/brpc/pull/3631#discussion_r4356663356
##########
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();
Review Comment:
`MongoInputMessage` is allocated from an object pool, but
`ParseMongoMessage()` does not reset `msg->context` (nor clear the IOBufs)
before reuse. If `dynamic_cast` fails (or the context isn't set), a reused
message could carry a stale `context` from a previous request, leading to
incorrect session association and potential lifetime issues. Fix by explicitly
clearing `msg->meta`, `msg->payload`, and `msg->context.reset()` immediately
after `Get()` (or inside `MongoInputMessage::Get()`), before filling fields.
##########
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/or take other locks. That can
block parsing-context reset/release for the socket and risks lock-order
deadlocks if a `Describe()` implementation attempts to acquire locks held by
the parsing bthread while it waits on `_parsing_context_mutex`. Consider an
alternative that shortens the critical section: e.g., store parsing contexts in
a ref-counted handle (`intrusive_ptr`/shared ownership) so `DebugSocket()` can
grab a strong reference under the mutex, unlock, then call `Describe()` without
holding `_parsing_context_mutex`.
##########
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.
Review Comment:
On the early-return error path, `msg` is not destroyed/returned to the pool,
which leaks the pooled message object (and may also leak a held `intrusive_ptr`
if `context` was set earlier in a future refactor). Before returning
`MakeParseError(...)`, ensure the just-allocated `msg` is properly destroyed
(e.g., call `msg->Destroy()` or otherwise return it to the pool) to avoid leaks
even on 'unlikely' paths.
##########
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:
`MongoInputMessage` is allocated from an object pool, but
`ParseMongoMessage()` does not reset `msg->context` (nor clear the IOBufs)
before reuse. If `dynamic_cast` fails (or the context isn't set), a reused
message could carry a stale `context` from a previous request, leading to
incorrect session association and potential lifetime issues. Fix by explicitly
clearing `msg->meta`, `msg->payload`, and `msg->context.reset()` immediately
after `Get()` (or inside `MongoInputMessage::Get()`), before filling fields.
--
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]
