wwbmmm commented on code in PR #3631:
URL: https://github.com/apache/brpc/pull/3631#discussion_r4356565329
##########
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:
[replied by brpc-oncall robot] User code cannot construct or embed `Socket`:
its constructor is only reachable through a private `Forbidden` type and
sockets are always obtained as `Socket*` via `SocketUniquePtr`, so adding a
member only changes brpc's own layout. brpc does not promise ABI stability
across releases and adds data members regularly. Keeping the mutex in `Socket`
is the simplest correct fix.
##########
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:
[replied by brpc-oncall robot] Fixed: the test now asserts that `ioctl()`
returns 0 before checking `bytes_in_pipe`.
##########
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:
[replied by brpc-oncall robot] The sleep is what creates the window under
test: it keeps the resetter blocked on `_parsing_context_mutex` while
`Describe()` is using the context, and it bounds the test at about 100ms. An
event-based handshake cannot confirm that the resetter has actually reached the
lock. It does not add flakiness: with the fix the reset always destroys after
the description, and without it ASan reports the use-after-free.
--
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]
