From 421ab6c3fba268bf91a30c147171c79b2c4033b8 Mon Sep 17 00:00:00 2001 From: Winlin Date: Wed, 3 Sep 2025 19:45:24 -0400 Subject: [PATCH] WebRTC: Fix race condition in RTC publish timer callbacks. v7.0.76 (#4470) WebRTC RTC publish streams use timer callbacks (`SrsRtcPublishRtcpTimer` and `SrsRtcPublishTwccTimer`) that can cause race conditions in SRS's coroutine-based architecture. The timer callbacks are heavy functions that may trigger coroutine switches, during which the timer object can be freed by another coroutine, leading to use-after-free crashes. The race condition occurs because: 1. Timer callbacks (`on_timer`) perform heavy operations that can yield control 2. During coroutine switches, other coroutines may destroy the timer object 3. When control returns, the callback continues executing on a freed object Fixes potential crashes in WebRTC RTC publish streams under high concurrency. --- trunk/doc/CHANGELOG.md | 1 + trunk/src/app/srs_app_rtc_conn.cpp | 26 +++++- trunk/src/app/srs_app_rtc_conn.hpp | 2 + trunk/src/core/srs_core_version7.hpp | 2 +- .../src/protocol/srs_protocol_http_stack.hpp | 84 +++++++++---------- 5 files changed, 70 insertions(+), 45 deletions(-) diff --git a/trunk/doc/CHANGELOG.md b/trunk/doc/CHANGELOG.md index 5cee3e22c..c77ee1de5 100644 --- a/trunk/doc/CHANGELOG.md +++ b/trunk/doc/CHANGELOG.md @@ -7,6 +7,7 @@ The changelog for SRS. ## SRS 7.0 Changelog +* v7.0, 2025-09-03, Merge [#4470](https://github.com/ossrs/srs/pull/4470): RTX: Fix race condition for timer. v7.0.76 (#4470) * v7.0, 2025-09-02, Merge [#4466](https://github.com/ossrs/srs/pull/4466): AI: GB28181: Remove embedded SIP server and enforce external SIP usage. v7.0.75 (#4466) * v7.0, 2025-09-01, Merge [#4465](https://github.com/ossrs/srs/pull/4465): AI: Replace SrsSharedPtrMessage with SrsMediaPacket for unified media packet handling. v7.0.74 (#4465) * v7.0, 2025-09-01, Merge [#4464](https://github.com/ossrs/srs/pull/4464): AI: Use shared ptr in RTMP message. v7.0.73 (#4464) diff --git a/trunk/src/app/srs_app_rtc_conn.cpp b/trunk/src/app/srs_app_rtc_conn.cpp index 0f6622c2f..07adce7d0 100644 --- a/trunk/src/app/srs_app_rtc_conn.cpp +++ b/trunk/src/app/srs_app_rtc_conn.cpp @@ -928,18 +928,29 @@ srs_error_t SrsRtcPlayStream::do_request_keyframe(uint32_t ssrc, SrsContextId ci SrsRtcPublishRtcpTimer::SrsRtcPublishRtcpTimer(SrsRtcPublishStream *p) : p_(p) { + lock_ = srs_mutex_new(); _srs_shared_timer->timer1s()->subscribe(this); } SrsRtcPublishRtcpTimer::~SrsRtcPublishRtcpTimer() { - _srs_shared_timer->timer1s()->unsubscribe(this); + if (true) { + SrsLocker(lock_); + _srs_shared_timer->timer1s()->unsubscribe(this); + } + srs_mutex_destroy(lock_); } srs_error_t SrsRtcPublishRtcpTimer::on_timer(srs_utime_t interval) { srs_error_t err = srs_success; + // This is a very heavy function, and it may potentially cause a coroutine switch. + // Therefore, during this function, the 'this' pointer might become invalid because + // the object could be freed by another thread. As a result, we must lock the object + // to prevent it from being freed. + SrsLocker(lock_); + ++_srs_pps_pub->sugar; if (!p_->is_started) { @@ -964,18 +975,29 @@ srs_error_t SrsRtcPublishRtcpTimer::on_timer(srs_utime_t interval) SrsRtcPublishTwccTimer::SrsRtcPublishTwccTimer(SrsRtcPublishStream *p) : p_(p) { + lock_ = srs_mutex_new(); _srs_shared_timer->timer100ms()->subscribe(this); } SrsRtcPublishTwccTimer::~SrsRtcPublishTwccTimer() { - _srs_shared_timer->timer100ms()->unsubscribe(this); + if (true) { + SrsLocker(lock_); + _srs_shared_timer->timer100ms()->unsubscribe(this); + } + srs_mutex_destroy(lock_); } srs_error_t SrsRtcPublishTwccTimer::on_timer(srs_utime_t interval) { srs_error_t err = srs_success; + // This is a very heavy function, and it may potentially cause a coroutine switch. + // Therefore, during this function, the 'this' pointer might become invalid because + // the object could be freed by another thread. As a result, we must lock the object + // to prevent it from being freed. + SrsLocker(lock_); + ++_srs_pps_pub->sugar; if (!p_->is_started) { diff --git a/trunk/src/app/srs_app_rtc_conn.hpp b/trunk/src/app/srs_app_rtc_conn.hpp index 03eca542d..3842bf6f6 100644 --- a/trunk/src/app/srs_app_rtc_conn.hpp +++ b/trunk/src/app/srs_app_rtc_conn.hpp @@ -306,6 +306,7 @@ class SrsRtcPublishRtcpTimer : public ISrsFastTimer { private: SrsRtcPublishStream *p_; + srs_mutex_t lock_; public: SrsRtcPublishRtcpTimer(SrsRtcPublishStream *p); @@ -320,6 +321,7 @@ class SrsRtcPublishTwccTimer : public ISrsFastTimer { private: SrsRtcPublishStream *p_; + srs_mutex_t lock_; public: SrsRtcPublishTwccTimer(SrsRtcPublishStream *p); diff --git a/trunk/src/core/srs_core_version7.hpp b/trunk/src/core/srs_core_version7.hpp index b2c448982..796ec5e5d 100644 --- a/trunk/src/core/srs_core_version7.hpp +++ b/trunk/src/core/srs_core_version7.hpp @@ -9,6 +9,6 @@ #define VERSION_MAJOR 7 #define VERSION_MINOR 0 -#define VERSION_REVISION 75 +#define VERSION_REVISION 76 #endif \ No newline at end of file diff --git a/trunk/src/protocol/srs_protocol_http_stack.hpp b/trunk/src/protocol/srs_protocol_http_stack.hpp index a12ada741..d059b0bd1 100644 --- a/trunk/src/protocol/srs_protocol_http_stack.hpp +++ b/trunk/src/protocol/srs_protocol_http_stack.hpp @@ -863,48 +863,48 @@ enum http_status { }; /* Request Methods */ -#define HTTP_METHOD_MAP(XX) \ - XX(0, DELETE, DELETE) \ - XX(1, GET, GET) \ - XX(2, HEAD, HEAD) \ - XX(3, POST, POST) \ - XX(4, PUT, PUT) \ - /* pathological */ \ - XX(5, CONNECT, CONNECT) \ - XX(6, OPTIONS, OPTIONS) \ - XX(7, TRACE, TRACE) \ - /* WebDAV */ \ - XX(8, COPY, COPY) \ - XX(9, LOCK, LOCK) \ - XX(10, MKCOL, MKCOL) \ - XX(11, MOVE, MOVE) \ - XX(12, PROPFIND, PROPFIND) \ - XX(13, PROPPATCH, PROPPATCH) \ - XX(14, SEARCH, SEARCH) \ - XX(15, UNLOCK, UNLOCK) \ - XX(16, BIND, BIND) \ - XX(17, REBIND, REBIND) \ - XX(18, UNBIND, UNBIND) \ - XX(19, ACL, ACL) \ - /* subversion */ \ - XX(20, REPORT, REPORT) \ - XX(21, MKACTIVITY, MKACTIVITY) \ - XX(22, CHECKOUT, CHECKOUT) \ - XX(23, MERGE, MERGE) \ - /* upnp */ \ - XX(24, MSEARCH, M - SEARCH) \ - XX(25, NOTIFY, NOTIFY) \ - XX(26, SUBSCRIBE, SUBSCRIBE) \ - XX(27, UNSUBSCRIBE, UNSUBSCRIBE) \ - /* RFC-5789 */ \ - XX(28, PATCH, PATCH) \ - XX(29, PURGE, PURGE) \ - /* CalDAV */ \ - XX(30, MKCALENDAR, MKCALENDAR) \ - /* RFC-2068, section 19.6.1.2 */ \ - XX(31, LINK, LINK) \ - XX(32, UNLINK, UNLINK) \ - /* icecast */ \ +#define HTTP_METHOD_MAP(XX) \ + XX(0, DELETE, DELETE) \ + XX(1, GET, GET) \ + XX(2, HEAD, HEAD) \ + XX(3, POST, POST) \ + XX(4, PUT, PUT) \ + /* pathological */ \ + XX(5, CONNECT, CONNECT) \ + XX(6, OPTIONS, OPTIONS) \ + XX(7, TRACE, TRACE) \ + /* WebDAV */ \ + XX(8, COPY, COPY) \ + XX(9, LOCK, LOCK) \ + XX(10, MKCOL, MKCOL) \ + XX(11, MOVE, MOVE) \ + XX(12, PROPFIND, PROPFIND) \ + XX(13, PROPPATCH, PROPPATCH) \ + XX(14, SEARCH, SEARCH) \ + XX(15, UNLOCK, UNLOCK) \ + XX(16, BIND, BIND) \ + XX(17, REBIND, REBIND) \ + XX(18, UNBIND, UNBIND) \ + XX(19, ACL, ACL) \ + /* subversion */ \ + XX(20, REPORT, REPORT) \ + XX(21, MKACTIVITY, MKACTIVITY) \ + XX(22, CHECKOUT, CHECKOUT) \ + XX(23, MERGE, MERGE) \ + /* upnp */ \ + XX(24, MSEARCH, M - SEARCH) \ + XX(25, NOTIFY, NOTIFY) \ + XX(26, SUBSCRIBE, SUBSCRIBE) \ + XX(27, UNSUBSCRIBE, UNSUBSCRIBE) \ + /* RFC-5789 */ \ + XX(28, PATCH, PATCH) \ + XX(29, PURGE, PURGE) \ + /* CalDAV */ \ + XX(30, MKCALENDAR, MKCALENDAR) \ + /* RFC-2068, section 19.6.1.2 */ \ + XX(31, LINK, LINK) \ + XX(32, UNLINK, UNLINK) \ + /* icecast */ \ XX(33, SOURCE, SOURCE) enum http_method {