From 759e0b7241edbaebf9a4506c32e82f25c8accf10 Mon Sep 17 00:00:00 2001 From: philipel Date: Wed, 30 Nov 2016 01:32:05 -0800 Subject: [PATCH] Fix memory leak in video_coding::PacketBuffer::InsertPacket. BUG=webrtc:6788 Review-Url: https://codereview.webrtc.org/2535203002 Cr-Commit-Position: refs/heads/master@{#15314} --- webrtc/modules/video_coding/packet_buffer.cc | 30 ++++++++---- webrtc/modules/video_coding/packet_buffer.h | 7 +-- .../rtp_frame_reference_finder_unittest.cc | 48 +++++++++---------- .../video_packet_buffer_unittest.cc | 34 ++++++++++--- webrtc/video/rtp_stream_receiver.cc | 2 +- 5 files changed, 77 insertions(+), 44 deletions(-) diff --git a/webrtc/modules/video_coding/packet_buffer.cc b/webrtc/modules/video_coding/packet_buffer.cc index c9d787b6e6..03aeb5a4a9 100644 --- a/webrtc/modules/video_coding/packet_buffer.cc +++ b/webrtc/modules/video_coding/packet_buffer.cc @@ -56,11 +56,11 @@ PacketBuffer::~PacketBuffer() { Clear(); } -bool PacketBuffer::InsertPacket(const VCMPacket& packet) { +bool PacketBuffer::InsertPacket(VCMPacket* packet) { std::vector> found_frames; { rtc::CritScope lock(&crit_); - uint16_t seq_num = packet.seqNum; + uint16_t seq_num = packet->seqNum; size_t index = seq_num % size_; if (!first_packet_received_) { @@ -70,16 +70,22 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) { } else if (AheadOf(first_seq_num_, seq_num)) { // If we have explicitly cleared past this packet then it's old, // don't insert it. - if (is_cleared_to_first_seq_num_) + if (is_cleared_to_first_seq_num_) { + delete[] packet->dataPtr; + packet->dataPtr = nullptr; return false; + } first_seq_num_ = seq_num; } if (sequence_buffer_[index].used) { - // Duplicate packet, do nothing. - if (data_buffer_[index].seqNum == packet.seqNum) + // Duplicate packet, just delete the payload. + if (data_buffer_[index].seqNum == packet->seqNum) { + delete[] packet->dataPtr; + packet->dataPtr = nullptr; return true; + } // The packet buffer is full, try to expand the buffer. while (ExpandBufferSize() && sequence_buffer_[seq_num % size_].used) { @@ -87,20 +93,24 @@ bool PacketBuffer::InsertPacket(const VCMPacket& packet) { index = seq_num % size_; // Packet buffer is still full. - if (sequence_buffer_[index].used) + if (sequence_buffer_[index].used) { + delete[] packet->dataPtr; + packet->dataPtr = nullptr; return false; + } } if (AheadOf(seq_num, last_seq_num_)) last_seq_num_ = seq_num; - sequence_buffer_[index].frame_begin = packet.isFirstPacket; - sequence_buffer_[index].frame_end = packet.markerBit; - sequence_buffer_[index].seq_num = packet.seqNum; + sequence_buffer_[index].frame_begin = packet->isFirstPacket; + sequence_buffer_[index].frame_end = packet->markerBit; + sequence_buffer_[index].seq_num = packet->seqNum; sequence_buffer_[index].continuous = false; sequence_buffer_[index].frame_created = false; sequence_buffer_[index].used = true; - data_buffer_[index] = packet; + data_buffer_[index] = *packet; + packet->dataPtr = nullptr; found_frames = FindFrames(seq_num); } diff --git a/webrtc/modules/video_coding/packet_buffer.h b/webrtc/modules/video_coding/packet_buffer.h index 36b3027158..da7e80ffaa 100644 --- a/webrtc/modules/video_coding/packet_buffer.h +++ b/webrtc/modules/video_coding/packet_buffer.h @@ -48,9 +48,10 @@ class PacketBuffer { virtual ~PacketBuffer(); - // Returns true if |packet| is inserted into the packet buffer, - // false otherwise. Made virtual for testing. - virtual bool InsertPacket(const VCMPacket& packet); + // Returns true if |packet| is inserted into the packet buffer, false + // otherwise. The PacketBuffer will always take ownership of the + // |packet.dataPtr| when this function is called. Made virtual for testing. + virtual bool InsertPacket(VCMPacket* packet); void ClearTo(uint16_t seq_num); void Clear(); diff --git a/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc b/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc index 551b06b265..d67e1baafe 100644 --- a/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc +++ b/webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc @@ -33,8 +33,8 @@ class FakePacketBuffer : public PacketBuffer { return packet_it == packets_.end() ? nullptr : &packet_it->second; } - bool InsertPacket(const VCMPacket& packet) override { - packets_[packet.seqNum] = packet; + bool InsertPacket(VCMPacket* packet) override { + packets_[packet->seqNum] = *packet; return true; } @@ -83,11 +83,11 @@ class TestRtpFrameReferenceFinder : public ::testing::Test, packet.codec = kVideoCodecGeneric; packet.seqNum = seq_num_start; packet.frameType = keyframe ? kVideoFrameKey : kVideoFrameDelta; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); packet.seqNum = seq_num_end; packet.markerBit = true; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame(new RtpFrameObject( ref_packet_buffer_, seq_num_start, seq_num_end, 0, 0, 0)); @@ -110,12 +110,12 @@ class TestRtpFrameReferenceFinder : public ::testing::Test, packet.video_header.codecHeader.VP8.temporalIdx = tid; packet.video_header.codecHeader.VP8.tl0PicIdx = tl0; packet.video_header.codecHeader.VP8.layerSync = sync; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); if (seq_num_start != seq_num_end) { packet.seqNum = seq_num_end; packet.markerBit = true; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); } std::unique_ptr frame(new RtpFrameObject( @@ -148,13 +148,13 @@ class TestRtpFrameReferenceFinder : public ::testing::Test, packet.video_header.codecHeader.VP9.ss_data_available = true; packet.video_header.codecHeader.VP9.gof = *ss; } - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); if (seq_num_start != seq_num_end) { packet.markerBit = true; packet.video_header.codecHeader.VP9.ss_data_available = false; packet.seqNum = seq_num_end; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); } std::unique_ptr frame(new RtpFrameObject( @@ -186,12 +186,12 @@ class TestRtpFrameReferenceFinder : public ::testing::Test, packet.video_header.codecHeader.VP9.num_ref_pics = refs.size(); for (size_t i = 0; i < refs.size(); ++i) packet.video_header.codecHeader.VP9.pid_diff[i] = refs[i]; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); if (seq_num_start != seq_num_end) { packet.seqNum = seq_num_end; packet.markerBit = true; - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); } std::unique_ptr frame(new RtpFrameObject( @@ -1270,7 +1270,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidJumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.gof = ss; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1281,7 +1281,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidJumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.picture_id = 0; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1291,7 +1291,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidJumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.picture_id = 5000; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1323,7 +1323,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_Tl0JumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.ss_data_available = true; packet.video_header.codecHeader.VP9.gof = ss; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1333,7 +1333,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_Tl0JumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.picture_id = 1; packet.video_header.codecHeader.VP9.tl0_pic_idx = 0; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1344,7 +1344,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_Tl0JumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.picture_id = 2; packet.video_header.codecHeader.VP9.tl0_pic_idx = 2; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1356,7 +1356,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_Tl0JumpsBackwardThenForward) { packet.video_header.codecHeader.VP9.picture_id = 3; packet.video_header.codecHeader.VP9.tl0_pic_idx = 129; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1389,7 +1389,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidSmallJumpForward) { packet.video_header.codecHeader.VP9.ss_data_available = true; packet.video_header.codecHeader.VP9.gof = ss; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1399,7 +1399,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidSmallJumpForward) { packet.video_header.codecHeader.VP9.picture_id = 2; packet.video_header.codecHeader.VP9.tl0_pic_idx = 2; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1409,7 +1409,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidSmallJumpForward) { packet.video_header.codecHeader.VP9.picture_id = 3; packet.video_header.codecHeader.VP9.tl0_pic_idx = 2; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1419,7 +1419,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_PidSmallJumpForward) { packet.video_header.codecHeader.VP9.picture_id = 4; packet.video_header.codecHeader.VP9.tl0_pic_idx = 1; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1452,7 +1452,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_DropOldFrame) { packet.video_header.codecHeader.VP9.ss_data_available = true; packet.video_header.codecHeader.VP9.gof = ss; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1462,7 +1462,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_DropOldFrame) { packet.video_header.codecHeader.VP9.picture_id = 0; packet.video_header.codecHeader.VP9.tl0_pic_idx = 2; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); @@ -1472,7 +1472,7 @@ TEST_F(TestRtpFrameReferenceFinder, Vp9PidFix_DropOldFrame) { packet.video_header.codecHeader.VP9.picture_id = 3; packet.video_header.codecHeader.VP9.tl0_pic_idx = 2; { - ref_packet_buffer_->InsertPacket(packet); + ref_packet_buffer_->InsertPacket(&packet); std::unique_ptr frame( new RtpFrameObject(ref_packet_buffer_, 0, 0, 0, 0, 0)); reference_finder_->ManageFrame(std::move(frame)); diff --git a/webrtc/modules/video_coding/video_packet_buffer_unittest.cc b/webrtc/modules/video_coding/video_packet_buffer_unittest.cc index a937af61ad..1e2ef3ec2a 100644 --- a/webrtc/modules/video_coding/video_packet_buffer_unittest.cc +++ b/webrtc/modules/video_coding/video_packet_buffer_unittest.cc @@ -64,7 +64,7 @@ class TestPacketBuffer : public ::testing::Test, packet.sizeBytes = data_size; packet.dataPtr = data; - return packet_buffer_->InsertPacket(packet); + return packet_buffer_->InsertPacket(&packet); } void CheckFrame(uint16_t first_seq_num) { @@ -142,21 +142,21 @@ TEST_F(TestPacketBuffer, NackCount) { packet.markerBit = false; packet.timesNacked = 0; - packet_buffer_->InsertPacket(packet); + packet_buffer_->InsertPacket(&packet); packet.seqNum++; packet.isFirstPacket = false; packet.timesNacked = 1; - packet_buffer_->InsertPacket(packet); + packet_buffer_->InsertPacket(&packet); packet.seqNum++; packet.timesNacked = 3; - packet_buffer_->InsertPacket(packet); + packet_buffer_->InsertPacket(&packet); packet.seqNum++; packet.markerBit = true; packet.timesNacked = 1; - packet_buffer_->InsertPacket(packet); + packet_buffer_->InsertPacket(&packet); ASSERT_EQ(1UL, frames_from_callback_.size()); RtpFrameObject* frame = frames_from_callback_.begin()->second.get(); @@ -365,7 +365,7 @@ TEST_F(TestPacketBuffer, GetBitstreamH264BufferPadding) { packet.sizeBytes = sizeof(data_data); packet.isFirstPacket = true; packet.markerBit = true; - packet_buffer_->InsertPacket(packet); + packet_buffer_->InsertPacket(&packet); ASSERT_EQ(1UL, frames_from_callback_.size()); EXPECT_EQ(frames_from_callback_[seq_num]->EncodedImage()._length, @@ -438,5 +438,27 @@ TEST_F(TestPacketBuffer, FramesAfterClear) { CheckFrame(9057); } +TEST_F(TestPacketBuffer, DontLeakPayloadData) { + // NOTE! Any eventual leak is suppose to be detected by valgrind + // or any other similar tool. + uint8_t* data1 = new uint8_t[5]; + uint8_t* data2 = new uint8_t[5]; + uint8_t* data3 = new uint8_t[5]; + uint8_t* data4 = new uint8_t[5]; + + // Expected to free data1 upon PacketBuffer destruction. + EXPECT_TRUE(Insert(2, kKeyFrame, kFirst, kNotLast, 5, data1)); + + // Expect to free data2 upon insertion. + EXPECT_TRUE(Insert(2, kKeyFrame, kFirst, kNotLast, 5, data2)); + + // Expect to free data3 upon insertion (old packet). + packet_buffer_->ClearTo(1); + EXPECT_FALSE(Insert(1, kKeyFrame, kFirst, kNotLast, 5, data3)); + + // Expect to free data4 upon insertion (packet buffer is full). + EXPECT_FALSE(Insert(2 + kMaxSize, kKeyFrame, kFirst, kNotLast, 5, data4)); +} + } // namespace video_coding } // namespace webrtc diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index bc4aa37366..2f27d8dc9c 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -274,7 +274,7 @@ int32_t RtpStreamReceiver::OnReceivedPayloadData( packet.dataPtr = data; } - packet_buffer_->InsertPacket(packet); + packet_buffer_->InsertPacket(&packet); } else { if (video_receiver_->IncomingPacket(payload_data, payload_size, rtp_header_with_ntp) != 0) {