From 78410ad41361f5f6ffa13ed3681b5eaf7199a271 Mon Sep 17 00:00:00 2001 From: Benjamin Wright Date: Thu, 25 Oct 2018 09:52:57 -0700 Subject: [PATCH] Fixes use after free error when setting a new FrameEncryptor on ChannelSend. This change corrects a potential race condition when updating a FrameEncryptor for the audio send channel. If a FrameEncryptor is set on an active audio stream it is possible for the current FrameEncryptor attached to the audio channel to be deallocated due to the FrameEncryptors reference count reaching zero before the new FrameEncryptor is set on the channel. To address this issue the ChannelSend is now holds a scoped_reftptr to only allow deallocation when it is actually set on the encoder queue. ChannelSend is unique in this respect as the Audio Receiver a long with the Video Sender and Video Receiver streams all recreate themselves when they have a configuration change. ChannelSend instead reconfigures itself using the existing channel object. Added Seth as TBR as this only introduces mocks. TBR=shampson@webrtc.org Bug: webrtc:9907 Change-Id: Ibf391dc9cecdbed1874e0252ff5c2cb92a5c64f4 Reviewed-on: https://webrtc-review.googlesource.com/c/107664 Commit-Queue: Benjamin Wright Reviewed-by: Fredrik Solenberg Reviewed-by: Qingsi Wang Cr-Commit-Position: refs/heads/master@{#25374} --- api/BUILD.gn | 43 ++++++++++++++++++++++++-- api/test/mock_frame_decryptor.cc | 18 +++++++++++ api/test/mock_frame_decryptor.h | 40 ++++++++++++++++++++++++ api/test/mock_frame_encryptor.cc | 19 ++++++++++++ api/test/mock_frame_encryptor.h | 38 +++++++++++++++++++++++ audio/BUILD.gn | 2 ++ audio/audio_receive_stream_unittest.cc | 21 +++++++++++++ audio/audio_send_stream_unittest.cc | 29 ++++++++++++++++- audio/channel_receive.cc | 2 +- audio/channel_receive.h | 4 +-- audio/channel_send.cc | 7 +++-- audio/channel_send.h | 5 +-- audio/channel_send_proxy.cc | 3 +- audio/channel_send_proxy.h | 3 +- audio/mock_voe_channel_proxy.h | 6 ++-- pc/BUILD.gn | 3 +- 16 files changed, 226 insertions(+), 17 deletions(-) create mode 100644 api/test/mock_frame_decryptor.cc create mode 100644 api/test/mock_frame_decryptor.h create mode 100644 api/test/mock_frame_encryptor.cc create mode 100644 api/test/mock_frame_encryptor.h diff --git a/api/BUILD.gn b/api/BUILD.gn index 13ab1878ff..579f8835f5 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -453,13 +453,50 @@ if (rtc_include_tests) { ] } - rtc_source_set("fake_frame_crypto") { + rtc_source_set("mock_frame_encryptor") { + testonly = true + sources = [ + "test/mock_frame_encryptor.cc", + "test/mock_frame_encryptor.h", + ] + deps = [ + ":libjingle_peerconnection_api", + "../test:test_support", + ] + } + + rtc_source_set("mock_frame_decryptor") { + testonly = true + sources = [ + "test/mock_frame_decryptor.cc", + "test/mock_frame_decryptor.h", + ] + deps = [ + ":libjingle_peerconnection_api", + "../test:test_support", + ] + } + + rtc_source_set("fake_frame_encryptor") { + testonly = true + sources = [ + "test/fake_frame_encryptor.cc", + "test/fake_frame_encryptor.h", + ] + deps = [ + ":array_view", + ":libjingle_peerconnection_api", + "..:webrtc_common", + "../rtc_base:checks", + "../rtc_base:rtc_base_approved", + ] + } + + rtc_source_set("fake_frame_decryptor") { testonly = true sources = [ "test/fake_frame_decryptor.cc", "test/fake_frame_decryptor.h", - "test/fake_frame_encryptor.cc", - "test/fake_frame_encryptor.h", ] deps = [ ":array_view", diff --git a/api/test/mock_frame_decryptor.cc b/api/test/mock_frame_decryptor.cc new file mode 100644 index 0000000000..f4b54f966c --- /dev/null +++ b/api/test/mock_frame_decryptor.cc @@ -0,0 +1,18 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/test/mock_frame_decryptor.h" + +namespace webrtc { + +MockFrameDecryptor::MockFrameDecryptor() = default; +MockFrameDecryptor::~MockFrameDecryptor() = default; + +} // namespace webrtc diff --git a/api/test/mock_frame_decryptor.h b/api/test/mock_frame_decryptor.h new file mode 100644 index 0000000000..e80396554f --- /dev/null +++ b/api/test/mock_frame_decryptor.h @@ -0,0 +1,40 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef API_TEST_MOCK_FRAME_DECRYPTOR_H_ +#define API_TEST_MOCK_FRAME_DECRYPTOR_H_ + +#include + +#include "api/crypto/framedecryptorinterface.h" +#include "test/gmock.h" + +namespace webrtc { + +class MockFrameDecryptor : public FrameDecryptorInterface { + public: + MockFrameDecryptor(); + ~MockFrameDecryptor() override; + + MOCK_METHOD6(Decrypt, + int(cricket::MediaType, + const std::vector&, + rtc::ArrayView, + rtc::ArrayView, + rtc::ArrayView, + size_t*)); + + MOCK_METHOD2(GetMaxPlaintextByteSize, + size_t(cricket::MediaType, size_t encrypted_frame_size)); +}; + +} // namespace webrtc + +#endif // API_TEST_MOCK_FRAME_DECRYPTOR_H_ diff --git a/api/test/mock_frame_encryptor.cc b/api/test/mock_frame_encryptor.cc new file mode 100644 index 0000000000..0972259f05 --- /dev/null +++ b/api/test/mock_frame_encryptor.cc @@ -0,0 +1,19 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "api/test/mock_frame_encryptor.h" +#include "test/gmock.h" + +namespace webrtc { + +MockFrameEncryptor::MockFrameEncryptor() = default; +MockFrameEncryptor::~MockFrameEncryptor() = default; + +} // namespace webrtc diff --git a/api/test/mock_frame_encryptor.h b/api/test/mock_frame_encryptor.h new file mode 100644 index 0000000000..c4fb1fde87 --- /dev/null +++ b/api/test/mock_frame_encryptor.h @@ -0,0 +1,38 @@ +/* + * Copyright 2018 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef API_TEST_MOCK_FRAME_ENCRYPTOR_H_ +#define API_TEST_MOCK_FRAME_ENCRYPTOR_H_ + +#include "api/crypto/frameencryptorinterface.h" +#include "test/gmock.h" + +namespace webrtc { + +class MockFrameEncryptor : public FrameEncryptorInterface { + public: + MockFrameEncryptor(); + ~MockFrameEncryptor() override; + + MOCK_METHOD6(Encrypt, + int(cricket::MediaType, + uint32_t, + rtc::ArrayView, + rtc::ArrayView, + rtc::ArrayView, + size_t*)); + + MOCK_METHOD2(GetMaxCiphertextByteSize, + size_t(cricket::MediaType media_type, size_t frame_size)); +}; + +} // namespace webrtc + +#endif // API_TEST_MOCK_FRAME_ENCRYPTOR_H_ diff --git a/audio/BUILD.gn b/audio/BUILD.gn index 3046d619f7..35e10e6736 100644 --- a/audio/BUILD.gn +++ b/audio/BUILD.gn @@ -135,6 +135,8 @@ if (rtc_include_tests) { ":audio", ":audio_end_to_end_test", "../api:mock_audio_mixer", + "../api:mock_frame_decryptor", + "../api:mock_frame_encryptor", "../api/audio:audio_frame_api", "../api/units:time_delta", "../call:mock_call_interfaces", diff --git a/audio/audio_receive_stream_unittest.cc b/audio/audio_receive_stream_unittest.cc index 97c42c4383..8d9c166030 100644 --- a/audio/audio_receive_stream_unittest.cc +++ b/audio/audio_receive_stream_unittest.cc @@ -13,6 +13,7 @@ #include #include "api/test/mock_audio_mixer.h" +#include "api/test/mock_frame_decryptor.h" #include "audio/audio_receive_stream.h" #include "audio/conversion.h" #include "audio/mock_voe_channel_proxy.h" @@ -373,5 +374,25 @@ TEST(AudioReceiveStreamTest, ReconfigureWithUpdatedConfig) { recv_stream->Reconfigure(new_config); } + +TEST(AudioReceiveStreamTest, ReconfigureWithFrameDecryptor) { + ConfigHelper helper; + auto recv_stream = helper.CreateAudioReceiveStream(); + + auto new_config_0 = helper.config(); + rtc::scoped_refptr mock_frame_decryptor_0( + new rtc::RefCountedObject()); + new_config_0.frame_decryptor = mock_frame_decryptor_0; + + recv_stream->Reconfigure(new_config_0); + + auto new_config_1 = helper.config(); + rtc::scoped_refptr mock_frame_decryptor_1( + new rtc::RefCountedObject()); + new_config_1.frame_decryptor = mock_frame_decryptor_1; + new_config_1.crypto_options.sframe.require_frame_encryption = true; + recv_stream->Reconfigure(new_config_1); +} + } // namespace test } // namespace webrtc diff --git a/audio/audio_send_stream_unittest.cc b/audio/audio_send_stream_unittest.cc index c5e4060c92..31c10ec199 100644 --- a/audio/audio_send_stream_unittest.cc +++ b/audio/audio_send_stream_unittest.cc @@ -13,6 +13,7 @@ #include #include "absl/memory/memory.h" +#include "api/test/mock_frame_encryptor.h" #include "api/units/time_delta.h" #include "audio/audio_send_stream.h" #include "audio/audio_state.h" @@ -196,7 +197,7 @@ struct ConfigHelper { EXPECT_CALL(*channel_proxy_, SetLocalSSRC(kSsrc)).Times(1); EXPECT_CALL(*channel_proxy_, SetRTCP_CNAME(StrEq(kCName))).Times(1); EXPECT_CALL(*channel_proxy_, SetNACKStatus(true, 10)).Times(1); - EXPECT_CALL(*channel_proxy_, SetFrameEncryptor(nullptr)).Times(1); + EXPECT_CALL(*channel_proxy_, SetFrameEncryptor(_)).Times(1); EXPECT_CALL(*channel_proxy_, SetSendAudioLevelIndicationStatus(true, kAudioLevelId)) .Times(1); @@ -528,6 +529,32 @@ TEST(AudioSendStreamTest, ReconfigureTransportCcResetsFirst) { send_stream->Reconfigure(new_config); } +// Validates that reconfiguring the AudioSendStream with a Frame encryptor +// correctly reconfigures on the object without crashing. +TEST(AudioSendStreamTest, ReconfigureWithFrameEncryptor) { + ConfigHelper helper(false, true); + auto send_stream = helper.CreateAudioSendStream(); + auto new_config = helper.config(); + + rtc::scoped_refptr mock_frame_encryptor_0( + new rtc::RefCountedObject()); + new_config.frame_encryptor = mock_frame_encryptor_0; + EXPECT_CALL(*helper.channel_proxy(), SetFrameEncryptor(Ne(nullptr))).Times(1); + send_stream->Reconfigure(new_config); + + // Not updating the frame encryptor shouldn't force it to reconfigure. + EXPECT_CALL(*helper.channel_proxy(), SetFrameEncryptor(_)).Times(0); + send_stream->Reconfigure(new_config); + + // Updating frame encryptor to a new object should force a call to the proxy. + rtc::scoped_refptr mock_frame_encryptor_1( + new rtc::RefCountedObject()); + new_config.frame_encryptor = mock_frame_encryptor_1; + new_config.crypto_options.sframe.require_frame_encryption = true; + EXPECT_CALL(*helper.channel_proxy(), SetFrameEncryptor(Ne(nullptr))).Times(1); + send_stream->Reconfigure(new_config); +} + // Checks that AudioSendStream logs the times at which RTP packets are sent // through its interface. TEST(AudioSendStreamTest, UpdateLifetime) { diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc index 859a3bae83..fbe15003bb 100644 --- a/audio/channel_receive.cc +++ b/audio/channel_receive.cc @@ -207,7 +207,7 @@ ChannelReceive::ChannelReceive( bool jitter_buffer_fast_playout, rtc::scoped_refptr decoder_factory, absl::optional codec_pair_id, - FrameDecryptorInterface* frame_decryptor, + rtc::scoped_refptr frame_decryptor, const webrtc::CryptoOptions& crypto_options) : event_log_(rtc_event_log), rtp_receive_statistics_( diff --git a/audio/channel_receive.h b/audio/channel_receive.h index ac359bb62f..4862ab051e 100644 --- a/audio/channel_receive.h +++ b/audio/channel_receive.h @@ -115,7 +115,7 @@ class ChannelReceive : public RtpData { bool jitter_buffer_fast_playout, rtc::scoped_refptr decoder_factory, absl::optional codec_pair_id, - FrameDecryptorInterface* frame_decryptor, + rtc::scoped_refptr frame_decryptor, const webrtc::CryptoOptions& crypto_options); virtual ~ChannelReceive(); @@ -260,7 +260,7 @@ class ChannelReceive : public RtpData { rtc::ThreadChecker construction_thread_; // E2EE Audio Frame Decryption - FrameDecryptorInterface* frame_decryptor_ = nullptr; + rtc::scoped_refptr frame_decryptor_; webrtc::CryptoOptions crypto_options_; }; diff --git a/audio/channel_send.cc b/audio/channel_send.cc index 83350c4a4b..7d01a4d948 100644 --- a/audio/channel_send.cc +++ b/audio/channel_send.cc @@ -993,14 +993,15 @@ int64_t ChannelSend::GetRTT() const { return rtt; } -void ChannelSend::SetFrameEncryptor(FrameEncryptorInterface* frame_encryptor) { +void ChannelSend::SetFrameEncryptor( + rtc::scoped_refptr frame_encryptor) { rtc::CritScope cs(&encoder_queue_lock_); if (encoder_queue_is_active_) { encoder_queue_->PostTask([this, frame_encryptor]() { - this->frame_encryptor_ = frame_encryptor; + this->frame_encryptor_ = std::move(frame_encryptor); }); } else { - frame_encryptor_ = frame_encryptor; + frame_encryptor_ = std::move(frame_encryptor); } } diff --git a/audio/channel_send.h b/audio/channel_send.h index 7c3f1cd31f..54da0f5f5c 100644 --- a/audio/channel_send.h +++ b/audio/channel_send.h @@ -228,7 +228,8 @@ class ChannelSend int64_t GetRTT() const; // E2EE Custom Audio Frame Encryption - void SetFrameEncryptor(FrameEncryptorInterface* frame_encryptor); + void SetFrameEncryptor( + rtc::scoped_refptr frame_encryptor); private: class ProcessAndEncodeAudioTask; @@ -300,7 +301,7 @@ class ChannelSend rtc::TaskQueue* encoder_queue_ = nullptr; // E2EE Audio Frame Encryption - FrameEncryptorInterface* frame_encryptor_ = nullptr; + rtc::scoped_refptr frame_encryptor_; // E2EE Frame Encryption Options webrtc::CryptoOptions crypto_options_; int configured_bitrate_bps_ = 0; diff --git a/audio/channel_send_proxy.cc b/audio/channel_send_proxy.cc index 097ee9e1b4..33bed43b5a 100644 --- a/audio/channel_send_proxy.cc +++ b/audio/channel_send_proxy.cc @@ -12,6 +12,7 @@ #include +#include "api/crypto/frameencryptorinterface.h" #include "call/rtp_transport_controller_send_interface.h" #include "rtc_base/checks.h" @@ -199,7 +200,7 @@ ChannelSend* ChannelSendProxy::GetChannel() const { } void ChannelSendProxy::SetFrameEncryptor( - FrameEncryptorInterface* frame_encryptor) { + rtc::scoped_refptr frame_encryptor) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); channel_->SetFrameEncryptor(frame_encryptor); } diff --git a/audio/channel_send_proxy.h b/audio/channel_send_proxy.h index 55cf3e2588..9eae6288a1 100644 --- a/audio/channel_send_proxy.h +++ b/audio/channel_send_proxy.h @@ -87,7 +87,8 @@ class ChannelSendProxy { virtual ChannelSend* GetChannel() const; // E2EE Custom Audio Frame Encryption (Optional) - virtual void SetFrameEncryptor(FrameEncryptorInterface* frame_encryptor); + virtual void SetFrameEncryptor( + rtc::scoped_refptr frame_encryptor); private: // Thread checkers document and lock usage of some methods on voe::Channel to diff --git a/audio/mock_voe_channel_proxy.h b/audio/mock_voe_channel_proxy.h index 50b202e542..adf85dbfea 100644 --- a/audio/mock_voe_channel_proxy.h +++ b/audio/mock_voe_channel_proxy.h @@ -16,6 +16,7 @@ #include #include +#include "api/test/mock_frame_encryptor.h" #include "audio/channel_receive_proxy.h" #include "audio/channel_send_proxy.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" @@ -105,8 +106,9 @@ class MockChannelSendProxy : public voe::ChannelSendProxy { void(float recoverable_packet_loss_rate)); MOCK_METHOD0(StartSend, void()); MOCK_METHOD0(StopSend, void()); - MOCK_METHOD1(SetFrameEncryptor, - void(FrameEncryptorInterface* frame_encryptor)); + MOCK_METHOD1( + SetFrameEncryptor, + void(rtc::scoped_refptr frame_encryptor)); }; } // namespace test } // namespace webrtc diff --git a/pc/BUILD.gn b/pc/BUILD.gn index c6884b952e..bdbcb38964 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -497,7 +497,8 @@ if (rtc_include_tests) { deps = [ ":peerconnection", ":rtc_pc_base", - "../api:fake_frame_crypto", + "../api:fake_frame_decryptor", + "../api:fake_frame_encryptor", "../api:libjingle_peerconnection_api", "../api:mock_rtp", "../api/units:time_delta",