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/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) {