From 67f5440b9a0c2057fb9f990fa0e1c51b5b7d6c3c Mon Sep 17 00:00:00 2001 From: sauwming Date: Mon, 9 Oct 2023 09:15:47 +0800 Subject: [PATCH 1/9] Fixed DTLS handshake issue when RTCP address changes --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 26 +++++++++++++++++------ 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index 183d16e008..3b0f34ee3e 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -1267,8 +1267,17 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, /* Check remote address info, reattach member tp if changed */ if (idx == RTP_CHANNEL && !ds->use_ice && !ds->nego_completed[idx]) { pjmedia_transport_info info; + pj_bool_t use_rtcp_mux; + pjmedia_transport_get_info(ds->srtp->member_tp, &info); - if (pj_sockaddr_cmp(&ds->rem_addr, &info.src_rtp_name)) { + use_rtcp_mux = (pj_sockaddr_cmp(&info.sock_info.rtp_addr_name, + &info.sock_info.rtcp_addr_name))? PJ_FALSE: PJ_TRUE; + if (pj_sockaddr_cmp(&ds->rem_addr, &info.src_rtp_name) || + (!use_rtcp_mux && + pj_sockaddr_has_addr(&info.src_rtcp_name) && + (!pj_sockaddr_has_addr(&ds->rem_rtcp) || + pj_sockaddr_cmp(&ds->rem_rtcp, &info.src_rtcp_name)))) + { pjmedia_transport_attach_param ap; pj_status_t status; @@ -1277,13 +1286,12 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, pj_sockaddr_cp(&ds->rem_addr, &info.src_rtp_name); pj_sockaddr_cp(&ap.rem_addr, &ds->rem_addr); ap.addr_len = pj_sockaddr_get_len(&ap.rem_addr); - if (pj_sockaddr_cmp(&info.sock_info.rtp_addr_name, - &info.sock_info.rtcp_addr_name) == 0) - { + if (use_rtcp_mux) { /* Using RTP & RTCP multiplexing */ pj_sockaddr_cp(&ds->rem_rtcp, &ds->rem_addr); pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_rtcp); - } else if (pj_sockaddr_has_addr(&ds->rem_rtcp)) { + } else if (pj_sockaddr_has_addr(&info.src_rtcp_name)) { + pj_sockaddr_cp(&ds->rem_rtcp, &info.src_rtcp_name); pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_rtcp); } else { pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_addr); @@ -1300,11 +1308,15 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; + char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Re-attached transport to update " - "remote addr=%s:%d", + "remote addr=%s:%d remote rtcp=%s:%d", pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr))); + pj_sockaddr_get_port(&ap.rem_addr), + pj_sockaddr_print(&ap.rem_rtcp, addr2, + sizeof(addr2), 2), + pj_sockaddr_get_port(&ap.rem_rtcp))); } #endif } From 93785a3bf575978de7b83f3a3c5dfc91263d8e28 Mon Sep 17 00:00:00 2001 From: sauwming Date: Mon, 9 Oct 2023 09:39:37 +0800 Subject: [PATCH 2/9] Fixed bug in SRTP ROC checkingwhen using ICE --- pjsip/src/pjsua-lib/pjsua_media.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/pjsip/src/pjsua-lib/pjsua_media.c b/pjsip/src/pjsua-lib/pjsua_media.c index 2fe61b5ad1..2e7e055729 100644 --- a/pjsip/src/pjsua-lib/pjsua_media.c +++ b/pjsip/src/pjsua-lib/pjsua_media.c @@ -3400,8 +3400,9 @@ static pj_bool_t is_ice_running(pjmedia_transport *tp) static void check_srtp_roc(pjsua_call *call, unsigned med_idx, const pjsua_stream_info *new_si_, - const pjmedia_sdp_media *local_sdp, - const pjmedia_sdp_media *remote_sdp) + const pjmedia_sdp_media *local_m, + const pjmedia_sdp_session *rem_sdp, + const pjmedia_sdp_media *rem_m) { pjsua_call_media *call_med = &call->media[med_idx]; pjmedia_transport_info tpinfo; @@ -3528,8 +3529,8 @@ static void check_srtp_roc(pjsua_call *call, pjmedia_sdp_attr *attr; if (local_change) { - attr = pjmedia_sdp_attr_find(local_sdp->attr_count, - local_sdp->attr, &STR_ICE_UFRAG, + attr = pjmedia_sdp_attr_find(local_m->attr_count, + local_m->attr, &STR_ICE_UFRAG, NULL); if (!pj_strcmp(&call_med->prev_ice_info.loc_ufrag, &attr->value)) @@ -3541,11 +3542,17 @@ static void check_srtp_roc(pjsua_call *call, } if (rem_change) { - attr = pjmedia_sdp_attr_find(remote_sdp->attr_count, - remote_sdp->attr, &STR_ICE_UFRAG, + attr = pjmedia_sdp_attr_find(rem_m->attr_count, + rem_m->attr, &STR_ICE_UFRAG, NULL); - if (!pj_strcmp(&call_med->prev_ice_info.rem_ufrag, - &attr->value)) + if (attr == NULL) { + /* Find ice-ufrag attribute in session descriptor */ + attr = pjmedia_sdp_attr_find(rem_sdp->attr_count, + rem_sdp->attr, &STR_ICE_UFRAG, + NULL); + } + if (attr && !pj_strcmp(&call_med->prev_ice_info.rem_ufrag, + &attr->value)) { PJ_LOG(4, (THIS_FILE, "ICE unchanged, SRTP RX ROC " "maintained")); @@ -4003,8 +4010,8 @@ pj_status_t pjsua_media_channel_update(pjsua_call_id call_id, #if PJSUA_MEDIA_HAS_PJMEDIA || PJSUA_THIRD_PARTY_STREAM_HAS_GET_INFO #if defined(PJMEDIA_HAS_SRTP) && (PJMEDIA_HAS_SRTP != 0) /* Check if we need to reset or maintain SRTP ROC */ - check_srtp_roc(call, mi, &stream_info, - local_sdp->media[mi], remote_sdp->media[mi]); + check_srtp_roc(call, mi, &stream_info, local_sdp->media[mi], + remote_sdp, remote_sdp->media[mi]); #endif #endif @@ -4260,8 +4267,8 @@ pj_status_t pjsua_media_channel_update(pjsua_call_id call_id, #if PJSUA_MEDIA_HAS_PJMEDIA || PJSUA_THIRD_PARTY_STREAM_HAS_GET_INFO #if defined(PJMEDIA_HAS_SRTP) && (PJMEDIA_HAS_SRTP != 0) /* Check if we need to reset or maintain SRTP ROC */ - check_srtp_roc(call, mi, &stream_info, - local_sdp->media[mi], remote_sdp->media[mi]); + check_srtp_roc(call, mi, &stream_info, local_sdp->media[mi], + remote_sdp, remote_sdp->media[mi]); #endif #endif From 45856dfee4469ca35c4968deedc0c7d27f4c959a Mon Sep 17 00:00:00 2001 From: sauwming Date: Mon, 9 Oct 2023 14:56:45 +0800 Subject: [PATCH 3/9] Also check the same for local SDP --- pjsip/src/pjsua-lib/pjsua_media.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/pjsip/src/pjsua-lib/pjsua_media.c b/pjsip/src/pjsua-lib/pjsua_media.c index 2e7e055729..b7dde8bdd3 100644 --- a/pjsip/src/pjsua-lib/pjsua_media.c +++ b/pjsip/src/pjsua-lib/pjsua_media.c @@ -3400,9 +3400,8 @@ static pj_bool_t is_ice_running(pjmedia_transport *tp) static void check_srtp_roc(pjsua_call *call, unsigned med_idx, const pjsua_stream_info *new_si_, - const pjmedia_sdp_media *local_m, - const pjmedia_sdp_session *rem_sdp, - const pjmedia_sdp_media *rem_m) + const pjmedia_sdp_session *local_sdp, + const pjmedia_sdp_session *rem_sdp) { pjsua_call_media *call_med = &call->media[med_idx]; pjmedia_transport_info tpinfo; @@ -3529,11 +3528,19 @@ static void check_srtp_roc(pjsua_call *call, pjmedia_sdp_attr *attr; if (local_change) { + pjmedia_sdp_media *local_m = local_sdp->media[med_idx]; + attr = pjmedia_sdp_attr_find(local_m->attr_count, local_m->attr, &STR_ICE_UFRAG, NULL); - if (!pj_strcmp(&call_med->prev_ice_info.loc_ufrag, - &attr->value)) + if (attr == NULL) { + /* Find ice-ufrag attribute in session level */ + attr = pjmedia_sdp_attr_find(local_sdp->attr_count, + local_sdp->attr, &STR_ICE_UFRAG, + NULL); + } + if (attr && !pj_strcmp(&call_med->prev_ice_info.loc_ufrag, + &attr->value)) { PJ_LOG(4, (THIS_FILE, "ICE unchanged, SRTP TX ROC " "maintained")); @@ -3542,11 +3549,13 @@ static void check_srtp_roc(pjsua_call *call, } if (rem_change) { + pjmedia_sdp_media *rem_m = rem_sdp->media[med_idx]; + attr = pjmedia_sdp_attr_find(rem_m->attr_count, rem_m->attr, &STR_ICE_UFRAG, NULL); if (attr == NULL) { - /* Find ice-ufrag attribute in session descriptor */ + /* Find ice-ufrag attribute in session level */ attr = pjmedia_sdp_attr_find(rem_sdp->attr_count, rem_sdp->attr, &STR_ICE_UFRAG, NULL); @@ -4010,8 +4019,7 @@ pj_status_t pjsua_media_channel_update(pjsua_call_id call_id, #if PJSUA_MEDIA_HAS_PJMEDIA || PJSUA_THIRD_PARTY_STREAM_HAS_GET_INFO #if defined(PJMEDIA_HAS_SRTP) && (PJMEDIA_HAS_SRTP != 0) /* Check if we need to reset or maintain SRTP ROC */ - check_srtp_roc(call, mi, &stream_info, local_sdp->media[mi], - remote_sdp, remote_sdp->media[mi]); + check_srtp_roc(call, mi, &stream_info, local_sdp, remote_sdp); #endif #endif @@ -4267,8 +4275,7 @@ pj_status_t pjsua_media_channel_update(pjsua_call_id call_id, #if PJSUA_MEDIA_HAS_PJMEDIA || PJSUA_THIRD_PARTY_STREAM_HAS_GET_INFO #if defined(PJMEDIA_HAS_SRTP) && (PJMEDIA_HAS_SRTP != 0) /* Check if we need to reset or maintain SRTP ROC */ - check_srtp_roc(call, mi, &stream_info, local_sdp->media[mi], - remote_sdp, remote_sdp->media[mi]); + check_srtp_roc(call, mi, &stream_info, local_sdp, remote_sdp); #endif #endif From 3d9cb5ba86b823948fc55601eb4b45c076eb2cda Mon Sep 17 00:00:00 2001 From: sauwming Date: Wed, 11 Oct 2023 11:28:37 +0800 Subject: [PATCH 4/9] Fixed deadlock and tp reattachment --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 97 ++++++++++++++++------- 1 file changed, 70 insertions(+), 27 deletions(-) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index 3b0f34ee3e..d1f6ea16a9 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -314,6 +314,12 @@ static void DTLS_LOCK(dtls_srtp *ds) { pj_lock_acquire(ds->ossl_lock); } +static pj_status_t DTLS_TRY_LOCK(dtls_srtp *ds) { + if (ds->base.grp_lock) + return pj_grp_lock_tryacquire(ds->base.grp_lock); + else + return pj_lock_tryacquire(ds->ossl_lock); +} static void DTLS_UNLOCK(dtls_srtp *ds) { if (ds->base.grp_lock) @@ -894,10 +900,27 @@ static void clock_cb(const pj_timestamp *ts, void *user_data) dtls_srtp_channel *ds_ch = (dtls_srtp_channel*)user_data; dtls_srtp *ds = ds_ch->dtls_srtp; unsigned idx = ds_ch->channel; + pj_status_t status; PJ_UNUSED_ARG(ts); - DTLS_LOCK(ds); + while (1) { + /* Check if we should quit before trying to acquire the lock. */ + if (ds->nego_completed[idx]) + return; + + /* To avoid deadlock, we must use TRY_LOCK here. */ + status = DTLS_TRY_LOCK(ds); + if (status == PJ_SUCCESS) + break; + + /* Acquiring lock failed, check if we have been signaled to quit. */ + if (ds->nego_completed[idx]) + return; + + pj_thread_sleep(20); + } + if (!ds->ossl_ssl[idx]) { DTLS_UNLOCK(ds); @@ -982,6 +1005,7 @@ static pj_status_t ssl_handshake_channel(dtls_srtp *ds, unsigned idx) on_return: if (status != PJ_SUCCESS) { + ds->nego_completed[idx] = PJ_TRUE; if (ds->clock[idx]) pjmedia_clock_stop(ds->clock[idx]); } @@ -1265,40 +1289,50 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, */ /* Check remote address info, reattach member tp if changed */ - if (idx == RTP_CHANNEL && !ds->use_ice && !ds->nego_completed[idx]) { + if (!ds->use_ice && !ds->nego_completed[idx]) { pjmedia_transport_info info; - pj_bool_t use_rtcp_mux; + pj_bool_t reattach_tp = PJ_FALSE; pjmedia_transport_get_info(ds->srtp->member_tp, &info); - use_rtcp_mux = (pj_sockaddr_cmp(&info.sock_info.rtp_addr_name, - &info.sock_info.rtcp_addr_name))? PJ_FALSE: PJ_TRUE; - if (pj_sockaddr_cmp(&ds->rem_addr, &info.src_rtp_name) || - (!use_rtcp_mux && - pj_sockaddr_has_addr(&info.src_rtcp_name) && - (!pj_sockaddr_has_addr(&ds->rem_rtcp) || - pj_sockaddr_cmp(&ds->rem_rtcp, &info.src_rtcp_name)))) + + if (idx == RTP_CHANNEL && + pj_sockaddr_cmp(&ds->rem_addr, &info.src_rtp_name)) + { + pj_sockaddr_cp(&ds->rem_addr, &info.src_rtp_name); + reattach_tp = PJ_TRUE; + } else if (idx == RTCP_CHANNEL && !ds->srtp->use_rtcp_mux && + pj_sockaddr_has_addr(&info.src_rtcp_name) && + pj_sockaddr_cmp(&ds->rem_rtcp, &info.src_rtcp_name)) { + pj_sockaddr_cp(&ds->rem_rtcp, &info.src_rtcp_name); + reattach_tp = PJ_TRUE; + } + + if (reattach_tp) { pjmedia_transport_attach_param ap; pj_status_t status; + /* Attach member transport */ pj_bzero(&ap, sizeof(ap)); ap.user_data = ds->srtp; - pj_sockaddr_cp(&ds->rem_addr, &info.src_rtp_name); - pj_sockaddr_cp(&ap.rem_addr, &ds->rem_addr); - ap.addr_len = pj_sockaddr_get_len(&ap.rem_addr); - if (use_rtcp_mux) { + if (pj_sockaddr_has_addr(&ds->rem_addr)) { + pj_sockaddr_cp(&ap.rem_addr, &ds->rem_addr); + } else { + pj_sockaddr_init(pj_AF_INET(), &ap.rem_addr, 0, 0); + } + if (ds->srtp->use_rtcp_mux) { /* Using RTP & RTCP multiplexing */ - pj_sockaddr_cp(&ds->rem_rtcp, &ds->rem_addr); - pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_rtcp); - } else if (pj_sockaddr_has_addr(&info.src_rtcp_name)) { - pj_sockaddr_cp(&ds->rem_rtcp, &info.src_rtcp_name); + pj_sockaddr_cp(&ap.rem_rtcp, &ap.rem_addr); + } else if (pj_sockaddr_has_addr(&ds->rem_rtcp)) { pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_rtcp); - } else { + } else if (pj_sockaddr_has_addr(&ds->rem_addr)) { pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_addr); pj_sockaddr_set_port(&ap.rem_rtcp, - pj_sockaddr_get_port(&ds->rem_addr)+1); + pj_sockaddr_get_port(&ap.rem_rtcp) + 1); + } else { + pj_sockaddr_init(pj_AF_INET(), &ap.rem_rtcp, 0, 0); } - + ap.addr_len = pj_sockaddr_get_len(&ap.rem_addr); status = pjmedia_transport_attach2(&ds->srtp->base, &ap); if (status != PJ_SUCCESS) { DTLS_UNLOCK(ds); @@ -1337,11 +1371,11 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, } } + DTLS_UNLOCK(ds); + /* Send it to OpenSSL */ ssl_on_recv_packet(ds, idx, pkt, size); - DTLS_UNLOCK(ds); - return PJ_SUCCESS; } @@ -1440,6 +1474,7 @@ static pj_status_t dtls_media_create( pjmedia_transport *tp, static void dtls_media_stop_channel(dtls_srtp *ds, unsigned idx) { + ds->nego_completed[idx] = PJ_TRUE; if (ds->clock[idx]) pjmedia_clock_stop(ds->clock[idx]); @@ -1640,9 +1675,13 @@ static pj_status_t dtls_encode_sdp( pjmedia_transport *tp, #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s:%d", + char addr2[PJ_INET6_ADDRSTRLEN]; + PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s:%d " + "remote rtcp=%s:%d", pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr))); + pj_sockaddr_get_port(&ap.rem_addr), + pj_sockaddr_print(&ap.rem_rtcp, addr2, sizeof(addr2), 2), + pj_sockaddr_get_port(&ap.rem_rtcp))); } #endif } @@ -1822,11 +1861,15 @@ static pj_status_t dtls_media_start( pjmedia_transport *tp, #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; + char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Attached transport, " - "remote addr=%s:%d", + "remote addr=%s:%d remote rtcp=%s:%d", pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr))); + pj_sockaddr_get_port(&ap.rem_addr), + pj_sockaddr_print(&ap.rem_rtcp, addr2, + sizeof(addr), 2), + pj_sockaddr_get_port(&ap.rem_rtcp))); } #endif From def45ce2572517f48a3bfe7bcb437c5e5564e5f1 Mon Sep 17 00:00:00 2001 From: sauwming Date: Thu, 12 Oct 2023 10:21:36 +0800 Subject: [PATCH 5/9] Use pj_sockaddr_print(.., 3) --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 31 +++++++++-------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index d1f6ea16a9..65312de063 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -1344,13 +1344,11 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, char addr[PJ_INET6_ADDRSTRLEN]; char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Re-attached transport to update " - "remote addr=%s:%d remote rtcp=%s:%d", + "remote addr=%s remote rtcp=%s", pj_sockaddr_print(&ap.rem_addr, addr, - sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr), + sizeof(addr), 3), pj_sockaddr_print(&ap.rem_rtcp, addr2, - sizeof(addr2), 2), - pj_sockaddr_get_port(&ap.rem_rtcp))); + sizeof(addr2), 3))); } #endif } @@ -1676,12 +1674,10 @@ static pj_status_t dtls_encode_sdp( pjmedia_transport *tp, { char addr[PJ_INET6_ADDRSTRLEN]; char addr2[PJ_INET6_ADDRSTRLEN]; - PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s:%d " - "remote rtcp=%s:%d", - pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr), - pj_sockaddr_print(&ap.rem_rtcp, addr2, sizeof(addr2), 2), - pj_sockaddr_get_port(&ap.rem_rtcp))); + PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s " + "remote rtcp=%s", + pj_sockaddr_print(&ap.rem_addr, addr2, sizeof(addr), 3), + pj_sockaddr_print(&ap.rem_rtcp, addr, sizeof(addr), 3))); } #endif } @@ -1863,13 +1859,11 @@ static pj_status_t dtls_media_start( pjmedia_transport *tp, char addr[PJ_INET6_ADDRSTRLEN]; char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Attached transport, " - "remote addr=%s:%d remote rtcp=%s:%d", + "remote addr=%s remote rtcp=%s", pj_sockaddr_print(&ap.rem_addr, addr, - sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr), + sizeof(addr), 3), pj_sockaddr_print(&ap.rem_rtcp, addr2, - sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_rtcp))); + sizeof(addr), 3))); } #endif @@ -2028,9 +2022,8 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_dtls_start_nego( #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s:%d", - pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), - pj_sockaddr_get_port(&ap.rem_addr))); + PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s", + pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 3))); } #endif From 1221e6ca4a74462cbfc65caab9ac755adc4ea769 Mon Sep 17 00:00:00 2001 From: sauwming Date: Thu, 12 Oct 2023 11:31:50 +0800 Subject: [PATCH 6/9] Add log in start_nego() --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index 65312de063..a9a96424e9 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -1676,7 +1676,7 @@ static pj_status_t dtls_encode_sdp( pjmedia_transport *tp, char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s " "remote rtcp=%s", - pj_sockaddr_print(&ap.rem_addr, addr2, sizeof(addr), 3), + pj_sockaddr_print(&ap.rem_addr, addr2, sizeof(addr2), 3), pj_sockaddr_print(&ap.rem_rtcp, addr, sizeof(addr), 3))); } #endif @@ -1863,7 +1863,7 @@ static pj_status_t dtls_media_start( pjmedia_transport *tp, pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 3), pj_sockaddr_print(&ap.rem_rtcp, addr2, - sizeof(addr), 3))); + sizeof(addr2), 3))); } #endif @@ -2022,8 +2022,11 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_dtls_start_nego( #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s", - pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 3))); + char addr2[PJ_INET6_ADDRSTRLEN]; + PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s " + "remote rtcp=%s", + pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 3), + pj_sockaddr_print(&ap.rem_addr, addr2, sizeof(addr2), 3))); } #endif From b9f506b31439edd3ce2243d1cf2f87134f108fbe Mon Sep 17 00:00:00 2001 From: sauwming Date: Fri, 13 Oct 2023 12:31:54 +0800 Subject: [PATCH 7/9] Fixed deadlock when destroying DTLS --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index a9a96424e9..3a3981981e 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -1905,6 +1905,7 @@ static pj_status_t dtls_media_stop(pjmedia_transport *tp) static void dtls_destroy_channel(dtls_srtp *ds, unsigned idx) { if (ds->clock[idx]) { + ds->nego_completed[idx] = PJ_TRUE; pjmedia_clock_destroy(ds->clock[idx]); ds->clock[idx] = NULL; } From ec50563059bc0d75d3c566bc9ddfcd643f2d4d69 Mon Sep 17 00:00:00 2001 From: sauwming Date: Fri, 13 Oct 2023 23:34:26 +0800 Subject: [PATCH 8/9] Revert changes in DTLS --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 110 ++++++---------------- 1 file changed, 29 insertions(+), 81 deletions(-) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index 3a3981981e..04a164c97a 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -293,7 +293,7 @@ static pj_status_t dtls_create(transport_srtp *srtp, pj_grp_lock_add_ref(grp_lock); pj_grp_lock_add_handler(grp_lock, pool, ds, &dtls_on_destroy); } else { - status = pj_lock_create_simple_mutex(ds->pool, "dtls_ssl_lock%p", + status = pj_lock_create_recursive_mutex(ds->pool, "dtls_ssl_lock%p", &ds->ossl_lock); if (status != PJ_SUCCESS) return status; @@ -314,12 +314,6 @@ static void DTLS_LOCK(dtls_srtp *ds) { pj_lock_acquire(ds->ossl_lock); } -static pj_status_t DTLS_TRY_LOCK(dtls_srtp *ds) { - if (ds->base.grp_lock) - return pj_grp_lock_tryacquire(ds->base.grp_lock); - else - return pj_lock_tryacquire(ds->ossl_lock); -} static void DTLS_UNLOCK(dtls_srtp *ds) { if (ds->base.grp_lock) @@ -900,27 +894,10 @@ static void clock_cb(const pj_timestamp *ts, void *user_data) dtls_srtp_channel *ds_ch = (dtls_srtp_channel*)user_data; dtls_srtp *ds = ds_ch->dtls_srtp; unsigned idx = ds_ch->channel; - pj_status_t status; PJ_UNUSED_ARG(ts); - while (1) { - /* Check if we should quit before trying to acquire the lock. */ - if (ds->nego_completed[idx]) - return; - - /* To avoid deadlock, we must use TRY_LOCK here. */ - status = DTLS_TRY_LOCK(ds); - if (status == PJ_SUCCESS) - break; - - /* Acquiring lock failed, check if we have been signaled to quit. */ - if (ds->nego_completed[idx]) - return; - - pj_thread_sleep(20); - } - + DTLS_LOCK(ds); if (!ds->ossl_ssl[idx]) { DTLS_UNLOCK(ds); @@ -1005,7 +982,6 @@ static pj_status_t ssl_handshake_channel(dtls_srtp *ds, unsigned idx) on_return: if (status != PJ_SUCCESS) { - ds->nego_completed[idx] = PJ_TRUE; if (ds->clock[idx]) pjmedia_clock_stop(ds->clock[idx]); } @@ -1289,50 +1265,32 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, */ /* Check remote address info, reattach member tp if changed */ - if (!ds->use_ice && !ds->nego_completed[idx]) { + if (idx == RTP_CHANNEL && !ds->use_ice && !ds->nego_completed[idx]) { pjmedia_transport_info info; - pj_bool_t reattach_tp = PJ_FALSE; - pjmedia_transport_get_info(ds->srtp->member_tp, &info); - - if (idx == RTP_CHANNEL && - pj_sockaddr_cmp(&ds->rem_addr, &info.src_rtp_name)) - { - pj_sockaddr_cp(&ds->rem_addr, &info.src_rtp_name); - reattach_tp = PJ_TRUE; - } else if (idx == RTCP_CHANNEL && !ds->srtp->use_rtcp_mux && - pj_sockaddr_has_addr(&info.src_rtcp_name) && - pj_sockaddr_cmp(&ds->rem_rtcp, &info.src_rtcp_name)) - { - pj_sockaddr_cp(&ds->rem_rtcp, &info.src_rtcp_name); - reattach_tp = PJ_TRUE; - } - - if (reattach_tp) { + if (pj_sockaddr_cmp(&ds->rem_addr, &info.src_rtp_name)) { pjmedia_transport_attach_param ap; pj_status_t status; - /* Attach member transport */ pj_bzero(&ap, sizeof(ap)); ap.user_data = ds->srtp; - if (pj_sockaddr_has_addr(&ds->rem_addr)) { - pj_sockaddr_cp(&ap.rem_addr, &ds->rem_addr); - } else { - pj_sockaddr_init(pj_AF_INET(), &ap.rem_addr, 0, 0); - } - if (ds->srtp->use_rtcp_mux) { + pj_sockaddr_cp(&ds->rem_addr, &info.src_rtp_name); + pj_sockaddr_cp(&ap.rem_addr, &ds->rem_addr); + ap.addr_len = pj_sockaddr_get_len(&ap.rem_addr); + if (pj_sockaddr_cmp(&info.sock_info.rtp_addr_name, + &info.sock_info.rtcp_addr_name) == 0) + { /* Using RTP & RTCP multiplexing */ - pj_sockaddr_cp(&ap.rem_rtcp, &ap.rem_addr); + pj_sockaddr_cp(&ds->rem_rtcp, &ds->rem_addr); + pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_rtcp); } else if (pj_sockaddr_has_addr(&ds->rem_rtcp)) { pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_rtcp); - } else if (pj_sockaddr_has_addr(&ds->rem_addr)) { + } else { pj_sockaddr_cp(&ap.rem_rtcp, &ds->rem_addr); pj_sockaddr_set_port(&ap.rem_rtcp, - pj_sockaddr_get_port(&ap.rem_rtcp) + 1); - } else { - pj_sockaddr_init(pj_AF_INET(), &ap.rem_rtcp, 0, 0); + pj_sockaddr_get_port(&ds->rem_addr)+1); } - ap.addr_len = pj_sockaddr_get_len(&ap.rem_addr); + status = pjmedia_transport_attach2(&ds->srtp->base, &ap); if (status != PJ_SUCCESS) { DTLS_UNLOCK(ds); @@ -1342,13 +1300,11 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Re-attached transport to update " - "remote addr=%s remote rtcp=%s", + "remote addr=%s:%d", pj_sockaddr_print(&ap.rem_addr, addr, - sizeof(addr), 3), - pj_sockaddr_print(&ap.rem_rtcp, addr2, - sizeof(addr2), 3))); + sizeof(addr), 2), + pj_sockaddr_get_port(&ap.rem_addr))); } #endif } @@ -1369,11 +1325,11 @@ static pj_status_t dtls_on_recv(pjmedia_transport *tp, unsigned idx, } } - DTLS_UNLOCK(ds); - /* Send it to OpenSSL */ ssl_on_recv_packet(ds, idx, pkt, size); + DTLS_UNLOCK(ds); + return PJ_SUCCESS; } @@ -1472,7 +1428,6 @@ static pj_status_t dtls_media_create( pjmedia_transport *tp, static void dtls_media_stop_channel(dtls_srtp *ds, unsigned idx) { - ds->nego_completed[idx] = PJ_TRUE; if (ds->clock[idx]) pjmedia_clock_stop(ds->clock[idx]); @@ -1673,11 +1628,9 @@ static pj_status_t dtls_encode_sdp( pjmedia_transport *tp, #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - char addr2[PJ_INET6_ADDRSTRLEN]; - PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s " - "remote rtcp=%s", - pj_sockaddr_print(&ap.rem_addr, addr2, sizeof(addr2), 3), - pj_sockaddr_print(&ap.rem_rtcp, addr, sizeof(addr), 3))); + PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s:%d", + pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), + pj_sockaddr_get_port(&ap.rem_addr))); } #endif } @@ -1857,13 +1810,11 @@ static pj_status_t dtls_media_start( pjmedia_transport *tp, #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - char addr2[PJ_INET6_ADDRSTRLEN]; PJ_LOG(2,(ds->base.name, "Attached transport, " - "remote addr=%s remote rtcp=%s", + "remote addr=%s:%d", pj_sockaddr_print(&ap.rem_addr, addr, - sizeof(addr), 3), - pj_sockaddr_print(&ap.rem_rtcp, addr2, - sizeof(addr2), 3))); + sizeof(addr), 2), + pj_sockaddr_get_port(&ap.rem_addr))); } #endif @@ -1905,7 +1856,6 @@ static pj_status_t dtls_media_stop(pjmedia_transport *tp) static void dtls_destroy_channel(dtls_srtp *ds, unsigned idx) { if (ds->clock[idx]) { - ds->nego_completed[idx] = PJ_TRUE; pjmedia_clock_destroy(ds->clock[idx]); ds->clock[idx] = NULL; } @@ -2023,11 +1973,9 @@ PJ_DEF(pj_status_t) pjmedia_transport_srtp_dtls_start_nego( #if DTLS_DEBUG { char addr[PJ_INET6_ADDRSTRLEN]; - char addr2[PJ_INET6_ADDRSTRLEN]; - PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s " - "remote rtcp=%s", - pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 3), - pj_sockaddr_print(&ap.rem_addr, addr2, sizeof(addr2), 3))); + PJ_LOG(2,(ds->base.name, "Attached transport, remote addr=%s:%d", + pj_sockaddr_print(&ap.rem_addr, addr, sizeof(addr), 2), + pj_sockaddr_get_port(&ap.rem_addr))); } #endif From 1677d6b70e8daa106ed6ae9012a218e13c6a64c1 Mon Sep 17 00:00:00 2001 From: sauwming Date: Fri, 13 Oct 2023 23:35:44 +0800 Subject: [PATCH 9/9] Revert changes in DTLS (2) --- pjmedia/src/pjmedia/transport_srtp_dtls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pjmedia/src/pjmedia/transport_srtp_dtls.c b/pjmedia/src/pjmedia/transport_srtp_dtls.c index 04a164c97a..183d16e008 100644 --- a/pjmedia/src/pjmedia/transport_srtp_dtls.c +++ b/pjmedia/src/pjmedia/transport_srtp_dtls.c @@ -293,7 +293,7 @@ static pj_status_t dtls_create(transport_srtp *srtp, pj_grp_lock_add_ref(grp_lock); pj_grp_lock_add_handler(grp_lock, pool, ds, &dtls_on_destroy); } else { - status = pj_lock_create_recursive_mutex(ds->pool, "dtls_ssl_lock%p", + status = pj_lock_create_simple_mutex(ds->pool, "dtls_ssl_lock%p", &ds->ossl_lock); if (status != PJ_SUCCESS) return status;