{{ message }}
Repository navigation
Conversation
added 2 commits
October 20, 2026 05:45
Socket::DebugSocket() (/sockets/<SocketId>) describes the parsing context from the bthread serving the request, while the bthread parsing messages of the socket may reset or release the context at the same time, e.g. InputMessenger resetting it before trying other protocols. The context could be destroyed while being described. Guard reset_parsing_context()/release_parsing_context() and the describing in DebugSocket() with a mutex, so that a context is taken out of the socket only after DebugSocket() is done with it.
ProcessMongoRequest() runs in a different bthread from the one parsing messages of the socket, but took the MongoContext from the parsing context of the socket, which the parsing bthread may reset in the meantime, e.g. before trying other protocols. Reference the context in ParseMongoMessage() and carry it with the request instead.
Contributor
There was a problem hiding this comment.
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.
Copilot review overview
4 open issues
MongoInputMessagenow owns the only explicit strong reference (intrusive_ptr) to the… · Newpolicy::MostCommonMessage::Get()(which is commonly used as a… · NewDebugSocket()holds_parsing_context_mutexwhile calling a virtualDescribe()implementation,… · NewDestroyImplshould be markedoverrideto ensure it correctly matches theInputMessageBase… · New
What changed in this PR
Fixes use-after-free races around socket parsing contexts by synchronizing Socket::DebugSocket() with parsing-context teardown and by decoupling Mongo request processing from the socket’s mutable parsing context.
Changes:
- Add a mutex to
Socketto prevent parsing contexts from being destroyed while/sockets/<id>is describing them. - Introduce
MongoInputMessageto carry a referencedMongoContextfrom parse-time to process-time across bthreads. - Add unit tests covering both the debug-socket race and the Mongo processing-after-reset scenario.
Review effort: Lite
Comment on lines
+232
to
+238
| 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()); | ||
| mongo_done->cntl.set_mongo_session_data(msg->context.get()); |
| socket->reset_parsing_context(socket_context_msg); | ||
| } | ||
| policy::MostCommonMessage* msg = policy::MostCommonMessage::Get(); | ||
| MongoInputMessage* msg = new MongoInputMessage; |
Comment on lines
+2430
to
2444
| { | ||
| 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); | ||
| } | ||
| } |
Comment on lines
+121
to
+122
Copilot stopped reviewing on behalf of
wwbmmm due to an error
October 20, 2026 09:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: N/A
Problem Summary:
Socket::DebugSocket()(the builtin/sockets/<SocketId>page) described the socket's parsing context from the bthread serving the HTTP request, while the bthread parsing messages of that socket could reset or release the context at the same time (e.g.InputMessengerresetting it before trying other protocols). The context, including anH2Contextwith its mutexes and HPACK state, could be destroyed while being described.ProcessMongoRequest()runs in a different bthread from the one parsing messages of the socket, but read theMongoContextfrom the socket's parsing context, which the parsing bthread may reset in the meantime.What is changed and the side effects?
Changed:
Socket::_parsing_context_mutex.reset_parsing_context()andrelease_parsing_context()hold it while taking the context out of the socket (Destroy()still runs after unlocking), andDebugSocket()holds it while loading and describing the context, so a context is destroyed or handed off only after the description is done.MongoInputMessage, which carries anintrusive_ptr<MongoContext>referenced inParseMongoMessage()on the parsing bthread.ProcessMongoRequest()uses that reference instead of reading the socket's parsing context.SocketTest.debug_socket_keeps_parsing_context_aliveandMongoTest.process_request_after_parsing_context_reset.Side effects:
Performance effects: none on hot paths. The new lock is only contended while
/socketsdescribes that very socket.Breaking backward compatibility: none.
Check List:
Tests run:
cmake -S . -B build -DBUILD_UNIT_TESTS=ON -DDOWNLOAD_GTEST=OFF -DBRPC_SYSTEM_GTEST_SOURCE_DIR=/usr/src/googletest && make -j6 brpc_socket_unittest brpc_mongo_protocol_unittest./test/brpc_socket_unittest(24 tests passed)./test/brpc_mongo_protocol_unittest(6 tests passed)🤖 This PR was automatically created by brpc-oncall