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<FrameEncryptor>
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 <benwright@webrtc.org>
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25374}diff --git a/api/BUILD.gn b/api/BUILD.gn
index 13ab187..579f883 100644
--- a/api/BUILD.gn
+++ b/api/BUILD.gn
@@ -453,13 +453,50 @@
]
}
- 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 0000000..f4b54f9
--- /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 0000000..e803965
--- /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 <vector>
+
+#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<uint32_t>&,
+ rtc::ArrayView<const uint8_t>,
+ rtc::ArrayView<const uint8_t>,
+ rtc::ArrayView<uint8_t>,
+ 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 0000000..0972259
--- /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 0000000..c4fb1fd
--- /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<const uint8_t>,
+ rtc::ArrayView<const uint8_t>,
+ rtc::ArrayView<uint8_t>,
+ 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 3046d61..35e10e6 100644
--- a/audio/BUILD.gn
+++ b/audio/BUILD.gn
@@ -135,6 +135,8 @@
":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 97c42c4..8d9c166 100644
--- a/audio/audio_receive_stream_unittest.cc
+++ b/audio/audio_receive_stream_unittest.cc
@@ -13,6 +13,7 @@
#include <vector>
#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 @@
recv_stream->Reconfigure(new_config);
}
+
+TEST(AudioReceiveStreamTest, ReconfigureWithFrameDecryptor) {
+ ConfigHelper helper;
+ auto recv_stream = helper.CreateAudioReceiveStream();
+
+ auto new_config_0 = helper.config();
+ rtc::scoped_refptr<FrameDecryptorInterface> mock_frame_decryptor_0(
+ new rtc::RefCountedObject<MockFrameDecryptor>());
+ new_config_0.frame_decryptor = mock_frame_decryptor_0;
+
+ recv_stream->Reconfigure(new_config_0);
+
+ auto new_config_1 = helper.config();
+ rtc::scoped_refptr<FrameDecryptorInterface> mock_frame_decryptor_1(
+ new rtc::RefCountedObject<MockFrameDecryptor>());
+ 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 c5e4060..31c10ec 100644
--- a/audio/audio_send_stream_unittest.cc
+++ b/audio/audio_send_stream_unittest.cc
@@ -13,6 +13,7 @@
#include <vector>
#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 @@
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 @@
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<FrameEncryptorInterface> mock_frame_encryptor_0(
+ new rtc::RefCountedObject<MockFrameEncryptor>());
+ 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<FrameEncryptorInterface> mock_frame_encryptor_1(
+ new rtc::RefCountedObject<MockFrameEncryptor>());
+ 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 859a3ba..fbe1500 100644
--- a/audio/channel_receive.cc
+++ b/audio/channel_receive.cc
@@ -207,7 +207,7 @@
bool jitter_buffer_fast_playout,
rtc::scoped_refptr<AudioDecoderFactory> decoder_factory,
absl::optional<AudioCodecPairId> codec_pair_id,
- FrameDecryptorInterface* frame_decryptor,
+ rtc::scoped_refptr<FrameDecryptorInterface> 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 ac359bb..4862ab0 100644
--- a/audio/channel_receive.h
+++ b/audio/channel_receive.h
@@ -115,7 +115,7 @@
bool jitter_buffer_fast_playout,
rtc::scoped_refptr<AudioDecoderFactory> decoder_factory,
absl::optional<AudioCodecPairId> codec_pair_id,
- FrameDecryptorInterface* frame_decryptor,
+ rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor,
const webrtc::CryptoOptions& crypto_options);
virtual ~ChannelReceive();
@@ -260,7 +260,7 @@
rtc::ThreadChecker construction_thread_;
// E2EE Audio Frame Decryption
- FrameDecryptorInterface* frame_decryptor_ = nullptr;
+ rtc::scoped_refptr<FrameDecryptorInterface> frame_decryptor_;
webrtc::CryptoOptions crypto_options_;
};
diff --git a/audio/channel_send.cc b/audio/channel_send.cc
index 83350c4..7d01a4d 100644
--- a/audio/channel_send.cc
+++ b/audio/channel_send.cc
@@ -993,14 +993,15 @@
return rtt;
}
-void ChannelSend::SetFrameEncryptor(FrameEncryptorInterface* frame_encryptor) {
+void ChannelSend::SetFrameEncryptor(
+ rtc::scoped_refptr<FrameEncryptorInterface> 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 7c3f1cd..54da0f5 100644
--- a/audio/channel_send.h
+++ b/audio/channel_send.h
@@ -228,7 +228,8 @@
int64_t GetRTT() const;
// E2EE Custom Audio Frame Encryption
- void SetFrameEncryptor(FrameEncryptorInterface* frame_encryptor);
+ void SetFrameEncryptor(
+ rtc::scoped_refptr<FrameEncryptorInterface> frame_encryptor);
private:
class ProcessAndEncodeAudioTask;
@@ -300,7 +301,7 @@
rtc::TaskQueue* encoder_queue_ = nullptr;
// E2EE Audio Frame Encryption
- FrameEncryptorInterface* frame_encryptor_ = nullptr;
+ rtc::scoped_refptr<FrameEncryptorInterface> 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 097ee9e..33bed43 100644
--- a/audio/channel_send_proxy.cc
+++ b/audio/channel_send_proxy.cc
@@ -12,6 +12,7 @@
#include <utility>
+#include "api/crypto/frameencryptorinterface.h"
#include "call/rtp_transport_controller_send_interface.h"
#include "rtc_base/checks.h"
@@ -199,7 +200,7 @@
}
void ChannelSendProxy::SetFrameEncryptor(
- FrameEncryptorInterface* frame_encryptor) {
+ rtc::scoped_refptr<FrameEncryptorInterface> 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 55cf3e2..9eae628 100644
--- a/audio/channel_send_proxy.h
+++ b/audio/channel_send_proxy.h
@@ -87,7 +87,8 @@
virtual ChannelSend* GetChannel() const;
// E2EE Custom Audio Frame Encryption (Optional)
- virtual void SetFrameEncryptor(FrameEncryptorInterface* frame_encryptor);
+ virtual void SetFrameEncryptor(
+ rtc::scoped_refptr<FrameEncryptorInterface> 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 50b202e..adf85db 100644
--- a/audio/mock_voe_channel_proxy.h
+++ b/audio/mock_voe_channel_proxy.h
@@ -16,6 +16,7 @@
#include <string>
#include <vector>
+#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 @@
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<FrameEncryptorInterface> frame_encryptor));
};
} // namespace test
} // namespace webrtc
diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index c6884b9..bdbcb38 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -497,7 +497,8 @@
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",