From 92d94898814c2a027bda3993da6510340145720c Mon Sep 17 00:00:00 2001 From: Wan-Teh Chang Date: Thu, 28 May 2015 13:36:06 -0700 Subject: [PATCH] Miscellaneous cleanups in VCMReceiver and its unit tests. The most important change is to prevent a potential buffer overflow in NackList(). It cannot happen if the |size| argument passed to NackList() is consistent with the |max_nack_list_size| argument passed to SetNackSettings(), and there is an assertion to check that. But it is good to defend against this in the release build because assert() is compiled away in the release build. Remove the unused |master| parameter to the VCMReceiver constructor. Remove the unused State() getter method and the corresponding state_ member. Remove the declarations for the nonexistent GenerateReceiverId() method and the receiver_id_counter_ member. Remove the unneeded data_buffer_ member of TestVCMReceiver. It was assigned to packet.dataPtr and then immediately overwritten by stream_generator_->GetPacket() or stream_generator_->PopPacket(). R=stefan@webrtc.org BUG=none TEST=none Review URL: https://webrtc-codereview.appspot.com/51119004 Cr-Commit-Position: refs/heads/master@{#9318} --- .../modules/video_coding/main/source/receiver.cc | 13 ++++--------- .../modules/video_coding/main/source/receiver.h | 15 +-------------- .../video_coding/main/source/receiver_unittest.cc | 10 ++-------- .../video_coding/main/source/video_receiver.cc | 2 +- 4 files changed, 8 insertions(+), 32 deletions(-) diff --git a/webrtc/modules/video_coding/main/source/receiver.cc b/webrtc/modules/video_coding/main/source/receiver.cc index a2b70b980..9efd4cd83 100644 --- a/webrtc/modules/video_coding/main/source/receiver.cc +++ b/webrtc/modules/video_coding/main/source/receiver.cc @@ -27,14 +27,12 @@ enum { kMaxReceiverDelayMs = 10000 }; VCMReceiver::VCMReceiver(VCMTiming* timing, Clock* clock, - EventFactory* event_factory, - bool master) + EventFactory* event_factory) : crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), clock_(clock), jitter_buffer_(clock_, event_factory), timing_(timing), render_wait_event_(event_factory->CreateEvent()), - state_(kPassive), max_video_delay_ms_(kMaxVideoDelayMs) { Reset(); } @@ -51,7 +49,6 @@ void VCMReceiver::Reset() { } else { jitter_buffer_.Flush(); } - state_ = kReceiving; } void VCMReceiver::UpdateRtt(int64_t rtt) { @@ -219,6 +216,9 @@ VCMNackStatus VCMReceiver::NackList(uint16_t* nack_list, uint16_t* internal_nack_list = jitter_buffer_.GetNackList( nack_list_length, &request_key_frame); assert(*nack_list_length <= size); + if (*nack_list_length > size) { + *nack_list_length = size; + } if (internal_nack_list != NULL && *nack_list_length > 0) { memcpy(nack_list, internal_nack_list, *nack_list_length * sizeof(uint16_t)); } @@ -228,11 +228,6 @@ VCMNackStatus VCMReceiver::NackList(uint16_t* nack_list, return kNackOk; } -VCMReceiverState VCMReceiver::State() const { - CriticalSectionScoped cs(crit_sect_); - return state_; -} - void VCMReceiver::SetDecodeErrorMode(VCMDecodeErrorMode decode_error_mode) { jitter_buffer_.SetDecodeErrorMode(decode_error_mode); } diff --git a/webrtc/modules/video_coding/main/source/receiver.h b/webrtc/modules/video_coding/main/source/receiver.h index fd48afcc7..049fb84af 100644 --- a/webrtc/modules/video_coding/main/source/receiver.h +++ b/webrtc/modules/video_coding/main/source/receiver.h @@ -28,18 +28,11 @@ enum VCMNackStatus { kNackKeyFrameRequest }; -enum VCMReceiverState { - kReceiving, - kPassive, - kWaitForPrimaryDecode -}; - class VCMReceiver { public: VCMReceiver(VCMTiming* timing, Clock* clock, - EventFactory* event_factory, - bool master); + EventFactory* event_factory); ~VCMReceiver(); void Reset(); @@ -64,7 +57,6 @@ class VCMReceiver { VCMNackMode NackMode() const; VCMNackStatus NackList(uint16_t* nackList, uint16_t size, uint16_t* nack_list_length); - VCMReceiverState State() const; // Receiver video delay. int SetMinReceiverDelay(int desired_delay_ms); @@ -83,17 +75,12 @@ class VCMReceiver { void TriggerDecoderShutdown(); private: - static int32_t GenerateReceiverId(); - CriticalSectionWrapper* crit_sect_; Clock* const clock_; VCMJitterBuffer jitter_buffer_; VCMTiming* timing_; rtc::scoped_ptr render_wait_event_; - VCMReceiverState state_; int max_video_delay_ms_; - - static int32_t receiver_id_counter_; }; } // namespace webrtc diff --git a/webrtc/modules/video_coding/main/source/receiver_unittest.cc b/webrtc/modules/video_coding/main/source/receiver_unittest.cc index e5b68047d..84a9527d1 100644 --- a/webrtc/modules/video_coding/main/source/receiver_unittest.cc +++ b/webrtc/modules/video_coding/main/source/receiver_unittest.cc @@ -24,17 +24,15 @@ namespace webrtc { class TestVCMReceiver : public ::testing::Test { protected: - enum { kDataBufferSize = 10 }; enum { kWidth = 640 }; enum { kHeight = 480 }; TestVCMReceiver() : clock_(new SimulatedClock(0)), timing_(clock_.get()), - receiver_(&timing_, clock_.get(), &event_factory_, true) { + receiver_(&timing_, clock_.get(), &event_factory_) { stream_generator_.reset(new StreamGenerator(0, 0, clock_->TimeInMilliseconds())); - memset(data_buffer_, 0, kDataBufferSize); } virtual void SetUp() { @@ -43,18 +41,15 @@ class TestVCMReceiver : public ::testing::Test { int32_t InsertPacket(int index) { VCMPacket packet; - packet.dataPtr = data_buffer_; bool packet_available = stream_generator_->GetPacket(&packet, index); EXPECT_TRUE(packet_available); if (!packet_available) return kGeneralError; // Return here to avoid crashes below. - // Arbitrary width and height. - return receiver_.InsertPacket(packet, 640, 480); + return receiver_.InsertPacket(packet, kWidth, kHeight); } int32_t InsertPacketAndPop(int index) { VCMPacket packet; - packet.dataPtr = data_buffer_; bool packet_available = stream_generator_->PopPacket(&packet, index); EXPECT_TRUE(packet_available); if (!packet_available) @@ -94,7 +89,6 @@ class TestVCMReceiver : public ::testing::Test { NullEventFactory event_factory_; VCMReceiver receiver_; rtc::scoped_ptr stream_generator_; - uint8_t data_buffer_[kDataBufferSize]; }; TEST_F(TestVCMReceiver, RenderBufferSize_AllComplete) { diff --git a/webrtc/modules/video_coding/main/source/video_receiver.cc b/webrtc/modules/video_coding/main/source/video_receiver.cc index 88bf212b1..32fe1a950 100644 --- a/webrtc/modules/video_coding/main/source/video_receiver.cc +++ b/webrtc/modules/video_coding/main/source/video_receiver.cc @@ -29,7 +29,7 @@ VideoReceiver::VideoReceiver(Clock* clock, EventFactory* event_factory) process_crit_sect_(CriticalSectionWrapper::CreateCriticalSection()), _receiveCritSect(CriticalSectionWrapper::CreateCriticalSection()), _timing(clock_), - _receiver(&_timing, clock_, event_factory, true), + _receiver(&_timing, clock_, event_factory), _decodedFrameCallback(_timing, clock_), _frameTypeCallback(NULL), _receiveStatsCallback(NULL),