From c462a6ef9bcd55ca40fed3489b0c13ca9c788e5c Mon Sep 17 00:00:00 2001 From: Benjamin Wright Date: Fri, 26 Oct 2018 13:16:16 -0700 Subject: [PATCH] Prevent the frame decryptor being set if the channel is stopped. This change deals with a race condition if the media channel has been stopped and is in the process of changing while we get a call to set a FrameDecryptor or FrameEncryptor. Bug: webrtc:9926, webrtc:9932 Change-Id: Ie2da2fa1f31f5cb5eb0b481861a7008e635f562d Reviewed-on: https://webrtc-review.googlesource.com/c/107986 Commit-Queue: Benjamin Wright Reviewed-by: Qingsi Wang Reviewed-by: Seth Hampson Cr-Commit-Position: refs/heads/master@{#25398} --- pc/rtpreceiver.cc | 17 ++++---- pc/rtpsender.cc | 17 ++++---- pc/rtpsenderreceiver_unittest.cc | 68 ++++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 16 deletions(-) diff --git a/pc/rtpreceiver.cc b/pc/rtpreceiver.cc index d2d7206a8d..acea930be3 100644 --- a/pc/rtpreceiver.cc +++ b/pc/rtpreceiver.cc @@ -50,8 +50,9 @@ void MaybeAttachFrameDecryptorToMediaChannel( const absl::optional& ssrc, rtc::Thread* worker_thread, rtc::scoped_refptr frame_decryptor, - cricket::MediaChannel* media_channel) { - if (media_channel && frame_decryptor && ssrc.has_value()) { + cricket::MediaChannel* media_channel, + bool stopped) { + if (media_channel && frame_decryptor && ssrc.has_value() && !stopped) { worker_thread->Invoke(RTC_FROM_HERE, [&] { media_channel->SetFrameDecryptor(*ssrc, frame_decryptor); }); @@ -157,7 +158,7 @@ void AudioRtpReceiver::SetFrameDecryptor( rtc::scoped_refptr frame_decryptor) { frame_decryptor_ = std::move(frame_decryptor); // Special Case: Set the frame decryptor to any value on any existing channel. - if (media_channel_ && ssrc_.has_value()) { + if (media_channel_ && ssrc_.has_value() && !stopped_) { worker_thread_->Invoke(RTC_FROM_HERE, [&] { media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); }); @@ -255,8 +256,8 @@ void AudioRtpReceiver::Reconfigure() { RTC_NOTREACHED(); } // Reattach the frame decryptor if we were reconfigured. - MaybeAttachFrameDecryptorToMediaChannel(ssrc_, worker_thread_, - frame_decryptor_, media_channel_); + MaybeAttachFrameDecryptorToMediaChannel( + ssrc_, worker_thread_, frame_decryptor_, media_channel_, stopped_); } void AudioRtpReceiver::SetObserver(RtpReceiverObserverInterface* observer) { @@ -351,7 +352,7 @@ void VideoRtpReceiver::SetFrameDecryptor( rtc::scoped_refptr frame_decryptor) { frame_decryptor_ = std::move(frame_decryptor); // Special Case: Set the frame decryptor to any value on any existing channel. - if (media_channel_ && ssrc_.has_value()) { + if (media_channel_ && ssrc_.has_value() && !stopped_) { worker_thread_->Invoke(RTC_FROM_HERE, [&] { media_channel_->SetFrameDecryptor(*ssrc_, frame_decryptor_); }); @@ -393,8 +394,8 @@ void VideoRtpReceiver::SetupMediaChannel(uint32_t ssrc) { ssrc_ = ssrc; SetSink(source_->sink()); // Attach any existing frame decryptor to the media channel. - MaybeAttachFrameDecryptorToMediaChannel(ssrc_, worker_thread_, - frame_decryptor_, media_channel_); + MaybeAttachFrameDecryptorToMediaChannel( + ssrc_, worker_thread_, frame_decryptor_, media_channel_, stopped_); } void VideoRtpReceiver::set_stream_ids(std::vector stream_ids) { diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc index dadd13d91b..3196641166 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -72,8 +72,9 @@ void MaybeAttachFrameEncryptorToMediaChannel( const uint32_t ssrc, rtc::Thread* worker_thread, rtc::scoped_refptr frame_encryptor, - cricket::MediaChannel* media_channel) { - if (media_channel && frame_encryptor && ssrc) { + cricket::MediaChannel* media_channel, + bool stopped) { + if (media_channel && frame_encryptor && ssrc && !stopped) { worker_thread->Invoke(RTC_FROM_HERE, [&] { media_channel->SetFrameEncryptor(ssrc, frame_encryptor); }); @@ -307,7 +308,7 @@ void AudioRtpSender::SetFrameEncryptor( rtc::scoped_refptr frame_encryptor) { frame_encryptor_ = std::move(frame_encryptor); // Special Case: Set the frame encryptor to any value on any existing channel. - if (media_channel_ && ssrc_) { + if (media_channel_ && ssrc_ && !stopped_) { worker_thread_->Invoke(RTC_FROM_HERE, [&] { media_channel_->SetFrameEncryptor(ssrc_, frame_encryptor_); }); @@ -361,8 +362,8 @@ void AudioRtpSender::SetSsrc(uint32_t ssrc) { }); } // Each time there is an ssrc update. - MaybeAttachFrameEncryptorToMediaChannel(ssrc_, worker_thread_, - frame_encryptor_, media_channel_); + MaybeAttachFrameEncryptorToMediaChannel( + ssrc_, worker_thread_, frame_encryptor_, media_channel_, stopped_); } void AudioRtpSender::Stop() { @@ -563,7 +564,7 @@ void VideoRtpSender::SetFrameEncryptor( rtc::scoped_refptr frame_encryptor) { frame_encryptor_ = std::move(frame_encryptor); // Special Case: Set the frame encryptor to any value on any existing channel. - if (media_channel_ && ssrc_) { + if (media_channel_ && ssrc_ && !stopped_) { worker_thread_->Invoke(RTC_FROM_HERE, [&] { media_channel_->SetFrameEncryptor(ssrc_, frame_encryptor_); }); @@ -610,8 +611,8 @@ void VideoRtpSender::SetSsrc(uint32_t ssrc) { init_parameters_.encodings.clear(); }); } - MaybeAttachFrameEncryptorToMediaChannel(ssrc_, worker_thread_, - frame_encryptor_, media_channel_); + MaybeAttachFrameEncryptorToMediaChannel( + ssrc_, worker_thread_, frame_encryptor_, media_channel_, stopped_); } void VideoRtpSender::Stop() { diff --git a/pc/rtpsenderreceiver_unittest.cc b/pc/rtpsenderreceiver_unittest.cc index 70b1601bcb..fa42615454 100644 --- a/pc/rtpsenderreceiver_unittest.cc +++ b/pc/rtpsenderreceiver_unittest.cc @@ -1424,6 +1424,18 @@ TEST_F(RtpSenderReceiverTest, AudioSenderCanSetFrameEncryptor) { audio_rtp_sender_->GetFrameEncryptor().get()); } +// Validate that setting a FrameEncryptor after the send stream is stopped does +// nothing. +TEST_F(RtpSenderReceiverTest, AudioSenderCannotSetFrameEncryptorAfterStop) { + CreateAudioRtpSender(); + rtc::scoped_refptr fake_frame_encryptor( + new FakeFrameEncryptor()); + EXPECT_EQ(nullptr, audio_rtp_sender_->GetFrameEncryptor()); + audio_rtp_sender_->Stop(); + audio_rtp_sender_->SetFrameEncryptor(fake_frame_encryptor); + // TODO(webrtc:9926) - Validate media channel not set once fakes updated. +} + // Validate that the default FrameEncryptor setting is nullptr. TEST_F(RtpSenderReceiverTest, AudioReceiverCanSetFrameDecryptor) { CreateAudioRtpReceiver(); @@ -1435,4 +1447,60 @@ TEST_F(RtpSenderReceiverTest, AudioReceiverCanSetFrameDecryptor) { audio_rtp_receiver_->GetFrameDecryptor().get()); } +// Validate that the default FrameEncryptor setting is nullptr. +TEST_F(RtpSenderReceiverTest, AudioReceiverCannotSetFrameDecryptorAfterStop) { + CreateAudioRtpReceiver(); + rtc::scoped_refptr fake_frame_decryptor( + new FakeFrameDecryptor()); + EXPECT_EQ(nullptr, audio_rtp_receiver_->GetFrameDecryptor()); + audio_rtp_receiver_->Stop(); + audio_rtp_receiver_->SetFrameDecryptor(fake_frame_decryptor); + // TODO(webrtc:9926) - Validate media channel not set once fakes updated. +} + +// Validate that the default FrameEncryptor setting is nullptr. +TEST_F(RtpSenderReceiverTest, VideoSenderCanSetFrameEncryptor) { + CreateVideoRtpSender(); + rtc::scoped_refptr fake_frame_encryptor( + new FakeFrameEncryptor()); + EXPECT_EQ(nullptr, video_rtp_sender_->GetFrameEncryptor()); + video_rtp_sender_->SetFrameEncryptor(fake_frame_encryptor); + EXPECT_EQ(fake_frame_encryptor.get(), + video_rtp_sender_->GetFrameEncryptor().get()); +} + +// Validate that setting a FrameEncryptor after the send stream is stopped does +// nothing. +TEST_F(RtpSenderReceiverTest, VideoSenderCannotSetFrameEncryptorAfterStop) { + CreateVideoRtpSender(); + rtc::scoped_refptr fake_frame_encryptor( + new FakeFrameEncryptor()); + EXPECT_EQ(nullptr, video_rtp_sender_->GetFrameEncryptor()); + video_rtp_sender_->Stop(); + video_rtp_sender_->SetFrameEncryptor(fake_frame_encryptor); + // TODO(webrtc:9926) - Validate media channel not set once fakes updated. +} + +// Validate that the default FrameEncryptor setting is nullptr. +TEST_F(RtpSenderReceiverTest, VideoReceiverCanSetFrameDecryptor) { + CreateVideoRtpReceiver(); + rtc::scoped_refptr fake_frame_decryptor( + new FakeFrameDecryptor()); + EXPECT_EQ(nullptr, video_rtp_receiver_->GetFrameDecryptor()); + video_rtp_receiver_->SetFrameDecryptor(fake_frame_decryptor); + EXPECT_EQ(fake_frame_decryptor.get(), + video_rtp_receiver_->GetFrameDecryptor().get()); +} + +// Validate that the default FrameEncryptor setting is nullptr. +TEST_F(RtpSenderReceiverTest, VideoReceiverCannotSetFrameDecryptorAfterStop) { + CreateVideoRtpReceiver(); + rtc::scoped_refptr fake_frame_decryptor( + new FakeFrameDecryptor()); + EXPECT_EQ(nullptr, video_rtp_receiver_->GetFrameDecryptor()); + video_rtp_receiver_->Stop(); + video_rtp_receiver_->SetFrameDecryptor(fake_frame_decryptor); + // TODO(webrtc:9926) - Validate media channel not set once fakes updated. +} + } // namespace webrtc