From de65a331f14207968fee662680a77473cad81417 Mon Sep 17 00:00:00 2001 From: winlin Date: Mon, 8 Mar 2021 12:39:25 +0800 Subject: [PATCH 1/2] SquashSRS4: Fix DTLS config bug, dup Alert bug. 4.0.83 --- README.md | 2 ++ trunk/src/app/srs_app_rtc_conn.cpp | 2 +- trunk/src/app/srs_app_rtc_dtls.cpp | 30 +++++++++++++++++++++------- trunk/src/app/srs_app_rtc_dtls.hpp | 2 +- trunk/src/app/srs_app_rtc_sdp.hpp | 3 ++- trunk/src/app/srs_app_rtc_server.cpp | 13 ++++++++---- trunk/src/app/srs_app_rtc_source.cpp | 2 ++ trunk/src/app/srs_app_source.cpp | 2 ++ trunk/src/core/srs_core_version4.hpp | 2 +- 9 files changed, 43 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index 6dcd230ee..a487d349d 100755 --- a/README.md +++ b/README.md @@ -186,6 +186,8 @@ Other documents: ## V4 changes +* v4.0, 2021-03-08, DTLS: Fix dead loop by duplicated Alert message. 4.0.83 +* v4.0, 2021-03-08, Fix bug when client DTLS is passive. 4.0.82 * v4.0, 2021-03-03, Fix [#2106][bug #2106], [#2011][bug #2011], RTMP/AAC transcode to Opus bug. 4.0.81 * v4.0, 2021-03-02, Refine build script for FFmpeg and SRTP. 4.0.80 * v4.0, 2021-03-02, Upgrade libsrtp from 2.0.0 to 2.3.0, with source code. 4.0.79 diff --git a/trunk/src/app/srs_app_rtc_conn.cpp b/trunk/src/app/srs_app_rtc_conn.cpp index 27dd2a903..c82241052 100644 --- a/trunk/src/app/srs_app_rtc_conn.cpp +++ b/trunk/src/app/srs_app_rtc_conn.cpp @@ -1920,7 +1920,7 @@ srs_error_t SrsRtcConnection::initialize(SrsRequest* r, bool dtls, bool srtp, st } } - SrsSessionConfig* cfg = &local_sdp.session_config_; + SrsSessionConfig* cfg = &local_sdp.session_negotiate_; if ((err = transport_->initialize(cfg)) != srs_success) { return srs_error_wrap(err, "init"); } diff --git a/trunk/src/app/srs_app_rtc_dtls.cpp b/trunk/src/app/srs_app_rtc_dtls.cpp index d2d6bc364..0d813132c 100644 --- a/trunk/src/app/srs_app_rtc_dtls.cpp +++ b/trunk/src/app/srs_app_rtc_dtls.cpp @@ -471,17 +471,33 @@ srs_error_t SrsDtlsImpl::do_on_dtls(char* data, int nb_data) return srs_error_wrap(err, "do handshake"); } - while (BIO_ctrl_pending(bio_in) > 0) { + // If there is data in bio_in, read it to let SSL consume it. + // @remark Limit the max loop, to avoid the dead loop. + for (int i = 0; i < 1024 && BIO_ctrl_pending(bio_in) > 0; i++) { char buf[8092]; - int nb = SSL_read(dtls, buf, sizeof(buf)); - if (nb <= 0) { + int r0 = SSL_read(dtls, buf, sizeof(buf)); + int r1 = SSL_get_error(dtls, r0); + + if (r0 <= 0) { + // SSL_ERROR_ZERO_RETURN + // + // The TLS/SSL connection has been closed. If the protocol version is SSL 3.0 or higher, + // this result code is returned only if a closure alert has occurred in the protocol, + // i.e. if the connection has been closed cleanly. + // @see https://www.openssl.org/docs/man1.1.0/man3/SSL_get_error.html + // @remark Already close, never read again, because padding always exsists. + if (r1 != SSL_ERROR_WANT_READ && r1 != SSL_ERROR_WANT_WRITE) { + break; + } continue; } - srs_trace("DTLS: read nb=%d, data=[%s]", nb, srs_string_dumps_hex(buf, nb, 32).c_str()); - if ((err = callback_->on_dtls_application_data(buf, nb)) != srs_success) { - return srs_error_wrap(err, "on DTLS data, size=%u, data=[%s]", nb, - srs_string_dumps_hex(buf, nb, 32).c_str()); + srs_trace("DTLS: read r0=%d, r1=%d, padding=%d, done=%d, data=[%s]", + r0, r1, BIO_ctrl_pending(bio_in), handshake_done_for_us, srs_string_dumps_hex(buf, r0, 32).c_str()); + + if ((err = callback_->on_dtls_application_data(buf, r0)) != srs_success) { + return srs_error_wrap(err, "on DTLS data, done=%d, r1=%d, size=%u, data=[%s]", handshake_done_for_us, + r1, r0, srs_string_dumps_hex(buf, r0, 32).c_str()); } } diff --git a/trunk/src/app/srs_app_rtc_dtls.hpp b/trunk/src/app/srs_app_rtc_dtls.hpp index 75e099f83..f72f6ef0d 100644 --- a/trunk/src/app/srs_app_rtc_dtls.hpp +++ b/trunk/src/app/srs_app_rtc_dtls.hpp @@ -118,7 +118,7 @@ protected: // @remark: dtls_version_ default value is SrsDtlsVersionAuto. SrsDtlsVersion version_; protected: - // Whether the handhshake is done, for us only. + // Whether the handshake is done, for us only. // @remark For us only, means peer maybe not done, we also need to handle the DTLS packet. bool handshake_done_for_us; // DTLS packet cache, only last out-going packet. diff --git a/trunk/src/app/srs_app_rtc_sdp.hpp b/trunk/src/app/srs_app_rtc_sdp.hpp index 372c893a1..16c3f8e01 100644 --- a/trunk/src/app/srs_app_rtc_sdp.hpp +++ b/trunk/src/app/srs_app_rtc_sdp.hpp @@ -37,7 +37,7 @@ const std::string kTWCCExt = "http://www.ietf.org/id/draft-holmer-rmcat-transpor // TDOO: FIXME: Rename it, and add utest. extern std::vector split_str(const std::string& str, const std::string& delim); -struct SrsSessionConfig +class SrsSessionConfig { public: std::string dtls_role; @@ -237,6 +237,7 @@ public: SrsSessionInfo session_info_; SrsSessionConfig session_config_; + SrsSessionConfig session_negotiate_; std::vector groups_; std::string group_policy_; diff --git a/trunk/src/app/srs_app_rtc_server.cpp b/trunk/src/app/srs_app_rtc_server.cpp index 617337889..9b0300682 100644 --- a/trunk/src/app/srs_app_rtc_server.cpp +++ b/trunk/src/app/srs_app_rtc_server.cpp @@ -590,18 +590,23 @@ srs_error_t SrsRtcServer::do_create_session( srs_trace("RTC: Use candidates %s", srs_join_vector_string(candidate_ips, ", ").c_str()); } + // Setup the negotiate DTLS by config. + local_sdp.session_negotiate_ = local_sdp.session_config_; + + // Setup the negotiate DTLS role. if (remote_sdp.get_dtls_role() == "active") { - local_sdp.set_dtls_role("passive"); + local_sdp.session_negotiate_.dtls_role = "passive"; } else if (remote_sdp.get_dtls_role() == "passive") { - local_sdp.set_dtls_role("active"); + local_sdp.session_negotiate_.dtls_role = "active"; } else if (remote_sdp.get_dtls_role() == "actpass") { - local_sdp.set_dtls_role(local_sdp.session_config_.dtls_role); + local_sdp.session_negotiate_.dtls_role = local_sdp.session_config_.dtls_role; } else { // @see: https://tools.ietf.org/html/rfc4145#section-4.1 // The default value of the setup attribute in an offer/answer exchange // is 'active' in the offer and 'passive' in the answer. - local_sdp.set_dtls_role("passive"); + local_sdp.session_negotiate_.dtls_role = "passive"; } + local_sdp.set_dtls_role(local_sdp.session_negotiate_.dtls_role); session->set_remote_sdp(remote_sdp); // We must setup the local SDP, then initialize the session object. diff --git a/trunk/src/app/srs_app_rtc_source.cpp b/trunk/src/app/srs_app_rtc_source.cpp index 96a6bcc18..d111d5e57 100644 --- a/trunk/src/app/srs_app_rtc_source.cpp +++ b/trunk/src/app/srs_app_rtc_source.cpp @@ -433,6 +433,8 @@ void SrsRtcStream::on_consumer_destroy(SrsRtcConsumer* consumer) bool SrsRtcStream::can_publish() { + // TODO: FIXME: Should check the status of bridger. + return !is_created_; } diff --git a/trunk/src/app/srs_app_source.cpp b/trunk/src/app/srs_app_source.cpp index 82b2177a1..103d8e20d 100755 --- a/trunk/src/app/srs_app_source.cpp +++ b/trunk/src/app/srs_app_source.cpp @@ -2123,6 +2123,8 @@ void SrsSource::update_auth(SrsRequest* r) bool SrsSource::can_publish(bool is_edge) { + // TODO: FIXME: Should check the status of bridger. + if (is_edge) { return publish_edge->can_publish(); } diff --git a/trunk/src/core/srs_core_version4.hpp b/trunk/src/core/srs_core_version4.hpp index 63540429b..c67ed23d7 100644 --- a/trunk/src/core/srs_core_version4.hpp +++ b/trunk/src/core/srs_core_version4.hpp @@ -24,6 +24,6 @@ #ifndef SRS_CORE_VERSION4_HPP #define SRS_CORE_VERSION4_HPP -#define SRS_VERSION4_REVISION 81 +#define SRS_VERSION4_REVISION 83 #endif From dc93836489633c977d31d3f8a9093b4b2053ddfb Mon Sep 17 00:00:00 2001 From: winlin Date: Tue, 9 Mar 2021 12:01:23 +0800 Subject: [PATCH 2/2] SquashSRS4: Refine DTLS init, use specified API by role --- trunk/src/app/srs_app_conn.hpp | 3 +++ trunk/src/app/srs_app_rtc_dtls.cpp | 33 +++++++++++++++++++++--------- trunk/src/app/srs_app_rtc_dtls.hpp | 6 +++--- trunk/src/app/srs_app_st.cpp | 8 ++++++-- trunk/src/utest/srs_utest_rtc.cpp | 4 ++-- 5 files changed, 37 insertions(+), 17 deletions(-) diff --git a/trunk/src/app/srs_app_conn.hpp b/trunk/src/app/srs_app_conn.hpp index db10ee423..ac95ca52d 100644 --- a/trunk/src/app/srs_app_conn.hpp +++ b/trunk/src/app/srs_app_conn.hpp @@ -47,8 +47,11 @@ public: virtual ~ISrsDisposingHandler(); public: // When before disposing resource, trigger when manager.remove(c), sync API. + // @remark Recommend to unref c, after this, no other objects refs to c. virtual void on_before_dispose(ISrsResource* c) = 0; // When disposing resource, async API, c is freed after it. + // @remark Recommend to stop any thread/timer of c, after this, fields of c is able + // to be deleted in any order. virtual void on_disposing(ISrsResource* c) = 0; }; diff --git a/trunk/src/app/srs_app_rtc_dtls.cpp b/trunk/src/app/srs_app_rtc_dtls.cpp index 0d813132c..daa894fc1 100644 --- a/trunk/src/app/srs_app_rtc_dtls.cpp +++ b/trunk/src/app/srs_app_rtc_dtls.cpp @@ -96,16 +96,24 @@ void ssl_on_info(const SSL* dtls, int where, int ret) } } -SSL_CTX* srs_build_dtls_ctx(SrsDtlsVersion version) +SSL_CTX* srs_build_dtls_ctx(SrsDtlsVersion version, std::string role) { SSL_CTX* dtls_ctx; #if OPENSSL_VERSION_NUMBER < 0x10002000L // v1.0.2 dtls_ctx = SSL_CTX_new(DTLSv1_method()); #else if (version == SrsDtlsVersion1_0) { - dtls_ctx = SSL_CTX_new(DTLSv1_method()); + if (role == "active") { + dtls_ctx = SSL_CTX_new(DTLSv1_client_method()); + } else { + dtls_ctx = SSL_CTX_new(DTLSv1_server_method()); + } } else if (version == SrsDtlsVersion1_2) { - dtls_ctx = SSL_CTX_new(DTLSv1_2_method()); + if (role == "active") { + dtls_ctx = SSL_CTX_new(DTLS_client_method()); + } else { + dtls_ctx = SSL_CTX_new(DTLS_server_method()); + } } else { // SrsDtlsVersionAuto, use version-flexible DTLS methods dtls_ctx = SSL_CTX_new(DTLS_method()); @@ -397,7 +405,7 @@ SrsDtlsImpl::~SrsDtlsImpl() srs_freepa(last_outgoing_packet_cache); } -srs_error_t SrsDtlsImpl::initialize(std::string version) +srs_error_t SrsDtlsImpl::initialize(std::string version, std::string role) { srs_error_t err = srs_success; @@ -409,7 +417,7 @@ srs_error_t SrsDtlsImpl::initialize(std::string version) version_ = SrsDtlsVersionAuto; } - dtls_ctx = srs_build_dtls_ctx(version_); + dtls_ctx = srs_build_dtls_ctx(version_, role); if ((dtls = SSL_new(dtls_ctx)) == NULL) { return srs_error_new(ERROR_OpenSslCreateSSL, "SSL_new dtls"); @@ -418,6 +426,11 @@ srs_error_t SrsDtlsImpl::initialize(std::string version) SSL_set_ex_data(dtls, 0, this); SSL_set_info_callback(dtls, ssl_on_info); + // set dtls fragment + // @see https://stackoverflow.com/questions/62413602/openssl-server-packets-get-fragmented-into-270-bytes-per-packet + SSL_set_options(dtls, SSL_OP_NO_QUERY_MTU); + SSL_set_mtu(dtls, kRtpPacketSize); + if ((bio_in = BIO_new(BIO_s_mem())) == NULL) { return srs_error_new(ERROR_OpenSslBIONew, "BIO_new in"); } @@ -643,11 +656,11 @@ SrsDtlsClientImpl::~SrsDtlsClientImpl() srs_freep(trd); } -srs_error_t SrsDtlsClientImpl::initialize(std::string version) +srs_error_t SrsDtlsClientImpl::initialize(std::string version, std::string role) { srs_error_t err = srs_success; - if ((err = SrsDtlsImpl::initialize(version)) != srs_success) { + if ((err = SrsDtlsImpl::initialize(version, role)) != srs_success) { return err; } @@ -819,11 +832,11 @@ SrsDtlsServerImpl::~SrsDtlsServerImpl() { } -srs_error_t SrsDtlsServerImpl::initialize(std::string version) +srs_error_t SrsDtlsServerImpl::initialize(std::string version, std::string role) { srs_error_t err = srs_success; - if ((err = SrsDtlsImpl::initialize(version)) != srs_success) { + if ((err = SrsDtlsImpl::initialize(version, role)) != srs_success) { return err; } @@ -892,7 +905,7 @@ srs_error_t SrsDtls::initialize(std::string role, std::string version) impl = new SrsDtlsServerImpl(callback_); } - return impl->initialize(version); + return impl->initialize(version, role); } srs_error_t SrsDtls::start_active_handshake() diff --git a/trunk/src/app/srs_app_rtc_dtls.hpp b/trunk/src/app/srs_app_rtc_dtls.hpp index f72f6ef0d..1e28eaf7d 100644 --- a/trunk/src/app/srs_app_rtc_dtls.hpp +++ b/trunk/src/app/srs_app_rtc_dtls.hpp @@ -130,7 +130,7 @@ public: SrsDtlsImpl(ISrsDtlsCallback* callback); virtual ~SrsDtlsImpl(); public: - virtual srs_error_t initialize(std::string version); + virtual srs_error_t initialize(std::string version, std::string role); virtual srs_error_t start_active_handshake() = 0; virtual srs_error_t on_dtls(char* data, int nb_data); protected: @@ -162,7 +162,7 @@ public: SrsDtlsClientImpl(ISrsDtlsCallback* callback); virtual ~SrsDtlsClientImpl(); public: - virtual srs_error_t initialize(std::string version); + virtual srs_error_t initialize(std::string version, std::string role); virtual srs_error_t start_active_handshake(); virtual srs_error_t on_dtls(char* data, int nb_data); protected: @@ -183,7 +183,7 @@ public: SrsDtlsServerImpl(ISrsDtlsCallback* callback); virtual ~SrsDtlsServerImpl(); public: - virtual srs_error_t initialize(std::string version); + virtual srs_error_t initialize(std::string version, std::string role); virtual srs_error_t start_active_handshake(); protected: virtual void on_ssl_out_data(uint8_t*& data, int& size, bool& cached); diff --git a/trunk/src/app/srs_app_st.cpp b/trunk/src/app/srs_app_st.cpp index 5a1954a4b..911680df7 100755 --- a/trunk/src/app/srs_app_st.cpp +++ b/trunk/src/app/srs_app_st.cpp @@ -163,6 +163,8 @@ SrsFastCoroutine::SrsFastCoroutine(string n, ISrsCoroutineHandler* h, SrsContext SrsFastCoroutine::~SrsFastCoroutine() { stop(); + + // TODO: FIXME: We must assert the cycle is done. srs_freep(trd_err); } @@ -213,7 +215,7 @@ void SrsFastCoroutine::stop() interrupt(); - // When not started, the rd is NULL. + // When not started, the trd is NULL. if (trd) { void* res = NULL; int r0 = st_thread_join((st_thread_t)trd, &res); @@ -245,7 +247,9 @@ void SrsFastCoroutine::interrupt() if (trd_err == srs_success) { trd_err = srs_error_new(ERROR_THREAD_INTERRUPED, "interrupted"); } - + + // Note that if another thread is stopping thread and waiting in st_thread_join, + // the interrupt will make the st_thread_join fail. st_thread_interrupt((st_thread_t)trd); } diff --git a/trunk/src/utest/srs_utest_rtc.cpp b/trunk/src/utest/srs_utest_rtc.cpp index 845f03b53..bf839aca3 100644 --- a/trunk/src/utest/srs_utest_rtc.cpp +++ b/trunk/src/utest/srs_utest_rtc.cpp @@ -563,7 +563,7 @@ VOID TEST(KernelRTCTest, StringDumpHexTest) } } -extern SSL_CTX* srs_build_dtls_ctx(SrsDtlsVersion version); +extern SSL_CTX* srs_build_dtls_ctx(SrsDtlsVersion version, std::string role); class MockDtls { @@ -625,7 +625,7 @@ srs_error_t MockDtls::initialize(std::string role, std::string version) version_ = SrsDtlsVersionAuto; } - dtls_ctx = srs_build_dtls_ctx(version_); + dtls_ctx = srs_build_dtls_ctx(version_, role); dtls = SSL_new(dtls_ctx); srs_assert(dtls);