Copilot commented on code in PR #3452:
URL: https://github.com/apache/brpc/pull/3452#discussion_r3788975571
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -805,6 +807,52 @@ TEST(RtmpTest, flv_reader_rejects_zero_datasize_audio_tag)
{
ASSERT_EQ(before, buf.size());
}
+// A crafted Abort message that names the chunk stream currently being parsed
+// (itself) used to make ClearChunkStream delete the RtmpChunkStream while its
+// Feed() is still running, which caused a heap-use-after-free right after
+// OnMessage() returned.
+TEST(RtmpTest, abort_message_naming_own_chunk_stream) {
+ int pipe_fds[2];
+ ASSERT_EQ(0, pipe(pipe_fds));
+ butil::fd_guard guard0(pipe_fds[0]);
+ butil::fd_guard guard1(pipe_fds[1]);
+
+ brpc::SocketId id;
+ brpc::SocketOptions options;
+ options.fd = pipe_fds[1];
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
Review Comment:
`SocketOptions::fd` takes ownership of the fd and `Socket` will close it on
recycle/destruction. Keeping `fd_guard guard1(pipe_fds[1])` causes a
double-close (and can accidentally close an unrelated fd if the descriptor
number gets reused). Transfer ownership to `Socket` by releasing the guard when
assigning `options.fd`.
##########
test/brpc_rtmp_unittest.cpp:
##########
@@ -805,6 +807,52 @@ TEST(RtmpTest, flv_reader_rejects_zero_datasize_audio_tag)
{
ASSERT_EQ(before, buf.size());
}
+// A crafted Abort message that names the chunk stream currently being parsed
+// (itself) used to make ClearChunkStream delete the RtmpChunkStream while its
+// Feed() is still running, which caused a heap-use-after-free right after
+// OnMessage() returned.
+TEST(RtmpTest, abort_message_naming_own_chunk_stream) {
+ int pipe_fds[2];
+ ASSERT_EQ(0, pipe(pipe_fds));
+ butil::fd_guard guard0(pipe_fds[0]);
+ butil::fd_guard guard1(pipe_fds[1]);
+
+ brpc::SocketId id;
+ brpc::SocketOptions options;
+ options.fd = pipe_fds[1];
+ ASSERT_EQ(0, brpc::Socket::Create(options, &id));
+ brpc::SocketUniquePtr sock;
+ ASSERT_EQ(0, brpc::Socket::Address(id, &sock));
+
+ brpc::policy::RtmpContext ctx(NULL, NULL);
+ ctx.SetState(sock->remote_side(),
+ brpc::policy::RtmpContext::STATE_RECEIVED_C2);
+
+ // fmt0 chunk on chunk stream 2 carrying an Abort message (type 2) whose
+ // payload is the same chunk stream id (2).
+ std::string chunk;
+ chunk.push_back((char)0x02); // basic header: fmt=0, cs_id=2
+ chunk.append(3, '\0'); // timestamp = 0
+ chunk.push_back('\0'); // message_length (3 bytes) = 4
+ chunk.push_back('\0');
+ chunk.push_back((char)0x04);
+ chunk.push_back((char)0x02); // message_type = Abort
+ chunk.append(4, '\0'); // stream_id = 0 (little endian)
+ chunk.push_back('\0'); // payload: cs_id = 2 (big endian)
+ chunk.push_back('\0');
+ chunk.push_back('\0');
+ chunk.push_back((char)0x02);
+
+ butil::IOBuf buf;
+ buf.append(chunk);
+ const brpc::ParseResult pr = ctx.Feed(&buf, sock.get());
+ ASSERT_EQ(brpc::PARSE_OK, pr.error());
+
+ // The chunk stream being parsed must survive the abort message: it must
+ // not be deleted by ClearChunkStream while Feed() is still running.
+ ASSERT_TRUE(ctx.GetChunkStream(2) != NULL);
Review Comment:
`ASSERT_TRUE(ctx.GetChunkStream(2) != NULL)` is effectively tautological:
for a valid id, `GetChunkStream` will create and return a chunk stream even if
it was just deleted, so this doesn't actually verify the stream “survived” the
Abort message. Capture the chunk stream pointer before feeding and assert that
the same instance remains after `Feed()` returns.
--
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]
