Improve SrtpSession thread safety and modernize sequence checking - Annotate SrtpSession internal state with RTC_GUARDED_BY. - Replaced RTC_DCHECK(thread_checker_.IsCurrent()) with RTC_DCHECK_RUN_ON. - Construct thread_checker_ detached to allow cross-thread creation. - Make dump_plain_rtp_ const. Bug: webrtc:361372443 Change-Id: Icc08c3e39579aa19c7c7fb612b800dd1ef966260 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/472380 Reviewed-by: Harald Alvestrand <hta@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47706}
diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 0e21649..e17db1e 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn
@@ -721,6 +721,7 @@ "../rtc_base:text2pcap", "../rtc_base:timeutils", "../rtc_base/synchronization:mutex", + "../rtc_base/system:no_unique_address", "../system_wrappers:metrics", "//third_party/abseil-cpp/absl/strings:string_view", ]
diff --git a/pc/srtp_session.cc b/pc/srtp_session.cc index def6bfa..613b4a6 100644 --- a/pc/srtp_session.cc +++ b/pc/srtp_session.cc
@@ -16,6 +16,7 @@ #include "absl/strings/string_view.h" #include "api/field_trials_view.h" +#include "api/sequence_checker.h" #include "modules/rtp_rtcp/source/rtp_util.h" #include "rtc_base/buffer.h" #include "rtc_base/byte_order.h" @@ -146,9 +147,8 @@ SrtpSession::SrtpSession() {} -SrtpSession::SrtpSession(const FieldTrialsView& field_trials) { - dump_plain_rtp_ = field_trials.IsEnabled("WebRTC-Debugging-RtpDump"); -} +SrtpSession::SrtpSession(const FieldTrialsView& field_trials) + : dump_plain_rtp_(field_trials.IsEnabled("WebRTC-Debugging-RtpDump")) {} SrtpSession::~SrtpSession() { if (session_) { @@ -185,7 +185,7 @@ } bool SrtpSession::ProtectRtp(CopyOnWriteBuffer& buffer) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); if (!session_) { RTC_LOG(LS_WARNING) << "Failed to protect SRTP packet: no SRTP Session"; return false; @@ -228,7 +228,7 @@ } bool SrtpSession::ProtectRtcp(CopyOnWriteBuffer& buffer) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); if (!session_) { RTC_LOG(LS_WARNING) << "Failed to protect SRTCP packet: no SRTP Session"; return false; @@ -262,7 +262,7 @@ bool SrtpSession::UnprotectRtp(CopyOnWriteBuffer& buffer) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); if (!session_) { RTC_LOG(LS_WARNING) << "Failed to unprotect SRTP packet: no SRTP Session"; return false; @@ -292,7 +292,7 @@ } bool SrtpSession::UnprotectRtcp(CopyOnWriteBuffer& buffer) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); if (!session_) { RTC_LOG(LS_WARNING) << "Failed to unprotect SRTCP packet: no SRTP Session"; return false; @@ -314,10 +314,12 @@ } int SrtpSession::GetSrtpOverhead() const { + RTC_DCHECK_RUN_ON(&thread_checker_); return rtp_auth_tag_len_; } bool SrtpSession::RemoveSsrcFromSession(uint32_t ssrc) { + RTC_DCHECK_RUN_ON(&thread_checker_); RTC_DCHECK(session_); // libSRTP expects the SSRC to be in network byte order. return srtp_remove_stream(session_, htonl(ssrc)) == srtp_err_status_ok; @@ -325,7 +327,7 @@ bool SrtpSession::GetSendStreamPacketIndex(CopyOnWriteBuffer& buffer, int64_t* index) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); uint32_t ssrc = ParseRtpSsrc(buffer); uint32_t roc; @@ -345,7 +347,7 @@ int crypto_suite, const ZeroOnFreeBuffer<uint8_t>& key, const std::vector<int>& extension_ids) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); srtp_policy_t policy; memset(&policy, 0, sizeof(policy)); @@ -403,7 +405,7 @@ int crypto_suite, const ZeroOnFreeBuffer<uint8_t>& key, const std::vector<int>& extension_ids) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); if (session_) { RTC_LOG(LS_ERROR) << "Failed to create SRTP session: " "SRTP session already created"; @@ -426,7 +428,7 @@ int crypto_suite, const ZeroOnFreeBuffer<uint8_t>& key, const std::vector<int>& extension_ids) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); if (!session_) { RTC_LOG(LS_ERROR) << "Failed to update non-existing SRTP session"; return false; @@ -440,7 +442,7 @@ } void SrtpSession::HandleEvent(const srtp_event_data_t* ev) { - RTC_DCHECK(thread_checker_.IsCurrent()); + RTC_DCHECK_RUN_ON(&thread_checker_); switch (ev->event) { case event_ssrc_collision: RTC_LOG(LS_INFO) << "SRTP event: SSRC collision";
diff --git a/pc/srtp_session.h b/pc/srtp_session.h index 3e4581c..4a27644 100644 --- a/pc/srtp_session.h +++ b/pc/srtp_session.h
@@ -20,6 +20,8 @@ #include "api/sequence_checker.h" #include "rtc_base/buffer.h" #include "rtc_base/copy_on_write_buffer.h" +#include "rtc_base/system/no_unique_address.h" +#include "rtc_base/thread_annotations.h" // Forward declaration to avoid pulling in libsrtp headers here struct srtp_event_data_t; @@ -104,20 +106,21 @@ void HandleEvent(const srtp_event_data_t* ev); static void HandleEventThunk(srtp_event_data_t* ev); - SequenceChecker thread_checker_; - srtp_ctx_t_* session_ = nullptr; + RTC_NO_UNIQUE_ADDRESS SequenceChecker thread_checker_{ + SequenceChecker::kDetached}; + srtp_ctx_t_* session_ RTC_GUARDED_BY(thread_checker_) = nullptr; // Overhead of the SRTP auth tag for RTP and RTCP in bytes. // Depends on the cipher suite used and is usually the same with the exception // of the kCsAesCm128HmacSha1_32 cipher suite. The additional four bytes // required for RTCP protection are not included. - int rtp_auth_tag_len_ = 0; - int rtcp_auth_tag_len_ = 0; + int rtp_auth_tag_len_ RTC_GUARDED_BY(thread_checker_) = 0; + int rtcp_auth_tag_len_ RTC_GUARDED_BY(thread_checker_) = 0; - bool inited_ = false; - int last_send_seq_num_ = -1; - int decryption_failure_count_ = 0; - bool dump_plain_rtp_ = false; + bool inited_ RTC_GUARDED_BY(thread_checker_) = false; + int last_send_seq_num_ RTC_GUARDED_BY(thread_checker_) = -1; + int decryption_failure_count_ RTC_GUARDED_BY(thread_checker_) = 0; + const bool dump_plain_rtp_ = false; }; } // namespace webrtc