From 153f9949eeb68c2ccdc349bde3d8d1dbf8647701 Mon Sep 17 00:00:00 2001 From: sauwming Date: Tue, 12 Dec 2023 22:10:48 +0800 Subject: [PATCH 1/3] Fixed deadlock between stream and ICE --- pjmedia/src/pjmedia/stream.c | 23 +++++++++++++++++------ pjmedia/src/pjmedia/vid_stream.c | 18 +++++++++++++++--- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/pjmedia/src/pjmedia/stream.c b/pjmedia/src/pjmedia/stream.c index ce4df6b292..d6175e32a7 100644 --- a/pjmedia/src/pjmedia/stream.c +++ b/pjmedia/src/pjmedia/stream.c @@ -161,6 +161,7 @@ struct pjmedia_stream pj_uint32_t tx_duration; /**< TX duration in timestamp. */ pj_mutex_t *jb_mutex; + pj_mutex_t *rtcp_mutex; pjmedia_jbuf *jb; /**< Jitter buffer. */ char jb_last_frm; /**< Last frame type from jb */ unsigned jb_last_frm_cnt;/**< Last JB frame type counter*/ @@ -1087,10 +1088,9 @@ static pj_status_t send_rtcp(pjmedia_stream *stream, pj_status_t status; /* We need to prevent data race since there is only a single instance - * of rtcp packet buffer. Let's just use the JB mutex for this instead - * of creating a separate lock. + * of rtcp packet buffer. */ - pj_mutex_lock(stream->jb_mutex); + pj_mutex_lock(stream->rtcp_mutex); /* Build RTCP RR/SR packet */ pjmedia_rtcp_build_rtcp(&stream->rtcp, &sr_rr_pkt, &len); @@ -1211,7 +1211,7 @@ static pj_status_t send_rtcp(pjmedia_stream *stream, } } - pj_mutex_unlock(stream->jb_mutex); + pj_mutex_unlock(stream->rtcp_mutex); return status; } @@ -2533,11 +2533,15 @@ PJ_DEF(pj_status_t) pjmedia_stream_create( pjmedia_endpt *endpt, } /* Create mutex to protect jitter buffer: */ - status = pj_mutex_create_simple(pool, NULL, &stream->jb_mutex); if (status != PJ_SUCCESS) goto err_cleanup; + /* Create mutex for RTCP purposes */ + status = pj_mutex_create_simple(pool, NULL, &stream->rtcp_mutex); + if (status != PJ_SUCCESS) + goto err_cleanup; + /* Create and initialize codec: */ @@ -3068,7 +3072,9 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream ) PJ_ASSERT_RETURN(stream != NULL, PJ_EINVAL); /* Send RTCP BYE (also SDES & XR) */ - if (stream->transport && stream->jb_mutex && !stream->rtcp_sdes_bye_disabled) { + if (stream->transport && stream->rtcp_mutex && + !stream->rtcp_sdes_bye_disabled) + { #if defined(PJMEDIA_HAS_RTCP_XR) && (PJMEDIA_HAS_RTCP_XR != 0) send_rtcp(stream, PJ_TRUE, PJ_TRUE, stream->rtcp.xr_enabled, PJ_FALSE); #else @@ -3154,6 +3160,11 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream ) stream->jb_mutex = NULL; } + if (stream->rtcp_mutex) { + pj_mutex_destroy(stream->rtcp_mutex); + stream->rtcp_mutex = NULL; + } + /* Destroy jitter buffer */ if (stream->jb) pjmedia_jbuf_destroy(stream->jb); diff --git a/pjmedia/src/pjmedia/vid_stream.c b/pjmedia/src/pjmedia/vid_stream.c index 59a65f0cc0..db55aea884 100644 --- a/pjmedia/src/pjmedia/vid_stream.c +++ b/pjmedia/src/pjmedia/vid_stream.c @@ -107,6 +107,7 @@ struct pjmedia_vid_stream pjmedia_vid_codec_mgr *codec_mgr; /**< Codec manager. */ pjmedia_vid_stream_info info; /**< Stream info. */ pj_grp_lock_t *grp_lock; /**< Stream lock. */ + pj_mutex_t *rtcp_mutex; /**< RTCP mutex. */ pjmedia_vid_channel *enc; /**< Encoding channel. */ pjmedia_vid_channel *dec; /**< Decoding channel. */ @@ -584,7 +585,7 @@ static pj_status_t send_rtcp(pjmedia_vid_stream *stream, int len, max_len; pj_status_t status; - pj_grp_lock_acquire( stream->grp_lock ); + pj_mutex_lock(stream->rtcp_mutex); /* Build RTCP RR/SR packet */ pjmedia_rtcp_build_rtcp(&stream->rtcp, &sr_rr_pkt, &len); @@ -667,7 +668,7 @@ static pj_status_t send_rtcp(pjmedia_vid_stream *stream, } } - pj_grp_lock_release( stream->grp_lock ); + pj_mutex_unlock(stream->rtcp_mutex); return status; } @@ -1843,6 +1844,11 @@ PJ_DEF(pj_status_t) pjmedia_vid_stream_create( stream->cname.slen = p - stream->cname.ptr; } + /* Create mutex for RTCP purposes */ + status = pj_mutex_create_simple(pool, NULL, &stream->rtcp_mutex); + if (status != PJ_SUCCESS) + goto err_cleanup; + /* Create group lock */ status = pj_grp_lock_create_w_handler(pool, NULL, stream, &on_destroy, @@ -2191,7 +2197,9 @@ PJ_DEF(pj_status_t) pjmedia_vid_stream_destroy( pjmedia_vid_stream *stream ) pjmedia_event_unsubscribe(NULL, &stream_event_cb, stream, &stream->rtcp); /* Send RTCP BYE (also SDES) */ - if (stream->transport && stream->grp_lock && !stream->rtcp_sdes_bye_disabled) { + if (stream->transport && stream->rtcp_mutex && + !stream->rtcp_sdes_bye_disabled) + { send_rtcp(stream, PJ_TRUE, PJ_TRUE, PJ_FALSE, PJ_FALSE); } @@ -2235,6 +2243,10 @@ static void on_destroy( void *arg ) } /* Free mutex */ + if (stream->rtcp_mutex) { + pj_mutex_destroy(stream->rtcp_mutex); + stream->rtcp_mutex = NULL; + } stream->grp_lock = NULL; /* Destroy jitter buffer */ From 2695437ae2a3b5a7ff2a3ec109d5fa30b0f5b143 Mon Sep 17 00:00:00 2001 From: sauwming Date: Wed, 13 Dec 2023 17:29:46 +0800 Subject: [PATCH 2/3] Use media transport's group lock --- pjmedia/src/pjmedia/stream.c | 23 ++++++----------------- pjmedia/src/pjmedia/vid_stream.c | 22 +++++++--------------- 2 files changed, 13 insertions(+), 32 deletions(-) diff --git a/pjmedia/src/pjmedia/stream.c b/pjmedia/src/pjmedia/stream.c index d6175e32a7..77fe9bceed 100644 --- a/pjmedia/src/pjmedia/stream.c +++ b/pjmedia/src/pjmedia/stream.c @@ -161,7 +161,6 @@ struct pjmedia_stream pj_uint32_t tx_duration; /**< TX duration in timestamp. */ pj_mutex_t *jb_mutex; - pj_mutex_t *rtcp_mutex; pjmedia_jbuf *jb; /**< Jitter buffer. */ char jb_last_frm; /**< Last frame type from jb */ unsigned jb_last_frm_cnt;/**< Last JB frame type counter*/ @@ -1088,9 +1087,10 @@ static pj_status_t send_rtcp(pjmedia_stream *stream, pj_status_t status; /* We need to prevent data race since there is only a single instance - * of rtcp packet buffer. + * of rtcp packet buffer. And to avoid deadlock with media transport, + * we use the transport's group lock. */ - pj_mutex_lock(stream->rtcp_mutex); + pj_grp_lock_acquire(stream->transport->grp_lock); /* Build RTCP RR/SR packet */ pjmedia_rtcp_build_rtcp(&stream->rtcp, &sr_rr_pkt, &len); @@ -1211,7 +1211,7 @@ static pj_status_t send_rtcp(pjmedia_stream *stream, } } - pj_mutex_unlock(stream->rtcp_mutex); + pj_grp_lock_release(stream->transport->grp_lock); return status; } @@ -2533,12 +2533,8 @@ PJ_DEF(pj_status_t) pjmedia_stream_create( pjmedia_endpt *endpt, } /* Create mutex to protect jitter buffer: */ - status = pj_mutex_create_simple(pool, NULL, &stream->jb_mutex); - if (status != PJ_SUCCESS) - goto err_cleanup; - /* Create mutex for RTCP purposes */ - status = pj_mutex_create_simple(pool, NULL, &stream->rtcp_mutex); + status = pj_mutex_create_simple(pool, NULL, &stream->jb_mutex); if (status != PJ_SUCCESS) goto err_cleanup; @@ -3072,9 +3068,7 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream ) PJ_ASSERT_RETURN(stream != NULL, PJ_EINVAL); /* Send RTCP BYE (also SDES & XR) */ - if (stream->transport && stream->rtcp_mutex && - !stream->rtcp_sdes_bye_disabled) - { + if (stream->transport && !stream->rtcp_sdes_bye_disabled) { #if defined(PJMEDIA_HAS_RTCP_XR) && (PJMEDIA_HAS_RTCP_XR != 0) send_rtcp(stream, PJ_TRUE, PJ_TRUE, stream->rtcp.xr_enabled, PJ_FALSE); #else @@ -3160,11 +3154,6 @@ PJ_DEF(pj_status_t) pjmedia_stream_destroy( pjmedia_stream *stream ) stream->jb_mutex = NULL; } - if (stream->rtcp_mutex) { - pj_mutex_destroy(stream->rtcp_mutex); - stream->rtcp_mutex = NULL; - } - /* Destroy jitter buffer */ if (stream->jb) pjmedia_jbuf_destroy(stream->jb); diff --git a/pjmedia/src/pjmedia/vid_stream.c b/pjmedia/src/pjmedia/vid_stream.c index db55aea884..59d0ba1203 100644 --- a/pjmedia/src/pjmedia/vid_stream.c +++ b/pjmedia/src/pjmedia/vid_stream.c @@ -107,7 +107,6 @@ struct pjmedia_vid_stream pjmedia_vid_codec_mgr *codec_mgr; /**< Codec manager. */ pjmedia_vid_stream_info info; /**< Stream info. */ pj_grp_lock_t *grp_lock; /**< Stream lock. */ - pj_mutex_t *rtcp_mutex; /**< RTCP mutex. */ pjmedia_vid_channel *enc; /**< Encoding channel. */ pjmedia_vid_channel *dec; /**< Decoding channel. */ @@ -585,7 +584,11 @@ static pj_status_t send_rtcp(pjmedia_vid_stream *stream, int len, max_len; pj_status_t status; - pj_mutex_lock(stream->rtcp_mutex); + + /* To avoid deadlock with media transport, we use the transport's + * group lock. + */ + pj_grp_lock_acquire( stream->transport->grp_lock ); /* Build RTCP RR/SR packet */ pjmedia_rtcp_build_rtcp(&stream->rtcp, &sr_rr_pkt, &len); @@ -668,7 +671,7 @@ static pj_status_t send_rtcp(pjmedia_vid_stream *stream, } } - pj_mutex_unlock(stream->rtcp_mutex); + pj_grp_lock_release( stream->transport->grp_lock ); return status; } @@ -1844,11 +1847,6 @@ PJ_DEF(pj_status_t) pjmedia_vid_stream_create( stream->cname.slen = p - stream->cname.ptr; } - /* Create mutex for RTCP purposes */ - status = pj_mutex_create_simple(pool, NULL, &stream->rtcp_mutex); - if (status != PJ_SUCCESS) - goto err_cleanup; - /* Create group lock */ status = pj_grp_lock_create_w_handler(pool, NULL, stream, &on_destroy, @@ -2197,9 +2195,7 @@ PJ_DEF(pj_status_t) pjmedia_vid_stream_destroy( pjmedia_vid_stream *stream ) pjmedia_event_unsubscribe(NULL, &stream_event_cb, stream, &stream->rtcp); /* Send RTCP BYE (also SDES) */ - if (stream->transport && stream->rtcp_mutex && - !stream->rtcp_sdes_bye_disabled) - { + if (stream->transport && !stream->rtcp_sdes_bye_disabled) { send_rtcp(stream, PJ_TRUE, PJ_TRUE, PJ_FALSE, PJ_FALSE); } @@ -2243,10 +2239,6 @@ static void on_destroy( void *arg ) } /* Free mutex */ - if (stream->rtcp_mutex) { - pj_mutex_destroy(stream->rtcp_mutex); - stream->rtcp_mutex = NULL; - } stream->grp_lock = NULL; /* Destroy jitter buffer */ From 359210fcadc74aafd485dbc7b15c37924953f57b Mon Sep 17 00:00:00 2001 From: sauwming Date: Wed, 13 Dec 2023 19:12:15 +0800 Subject: [PATCH 3/3] Add group lock's existence check --- pjmedia/src/pjmedia/stream.c | 6 ++++-- pjmedia/src/pjmedia/vid_stream.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pjmedia/src/pjmedia/stream.c b/pjmedia/src/pjmedia/stream.c index 77fe9bceed..2e3200d6d2 100644 --- a/pjmedia/src/pjmedia/stream.c +++ b/pjmedia/src/pjmedia/stream.c @@ -1090,7 +1090,8 @@ static pj_status_t send_rtcp(pjmedia_stream *stream, * of rtcp packet buffer. And to avoid deadlock with media transport, * we use the transport's group lock. */ - pj_grp_lock_acquire(stream->transport->grp_lock); + if (stream->transport->grp_lock) + pj_grp_lock_acquire(stream->transport->grp_lock); /* Build RTCP RR/SR packet */ pjmedia_rtcp_build_rtcp(&stream->rtcp, &sr_rr_pkt, &len); @@ -1211,7 +1212,8 @@ static pj_status_t send_rtcp(pjmedia_stream *stream, } } - pj_grp_lock_release(stream->transport->grp_lock); + if (stream->transport->grp_lock) + pj_grp_lock_release(stream->transport->grp_lock); return status; } diff --git a/pjmedia/src/pjmedia/vid_stream.c b/pjmedia/src/pjmedia/vid_stream.c index 59d0ba1203..8e0d9c8272 100644 --- a/pjmedia/src/pjmedia/vid_stream.c +++ b/pjmedia/src/pjmedia/vid_stream.c @@ -588,7 +588,8 @@ static pj_status_t send_rtcp(pjmedia_vid_stream *stream, /* To avoid deadlock with media transport, we use the transport's * group lock. */ - pj_grp_lock_acquire( stream->transport->grp_lock ); + if (stream->transport->grp_lock) + pj_grp_lock_acquire( stream->transport->grp_lock ); /* Build RTCP RR/SR packet */ pjmedia_rtcp_build_rtcp(&stream->rtcp, &sr_rr_pkt, &len); @@ -671,7 +672,8 @@ static pj_status_t send_rtcp(pjmedia_vid_stream *stream, } } - pj_grp_lock_release( stream->transport->grp_lock ); + if (stream->transport->grp_lock) + pj_grp_lock_release( stream->transport->grp_lock ); return status; }