From 16bf8a31cb05a596b6181a95b9cc42f344ec24e8 Mon Sep 17 00:00:00 2001 From: Olivier Dion Date: Tue, 2 Aug 2022 10:03:56 -0300 Subject: [PATCH] pjnath/turn_sock: Fix destruction of TURN sockets When destroying a TURN socket on failure -- e.g. failed allocation of socket -- the underlying session does not change its last status. Change this by passing a `pj_status_t' value to `pj_turn_sock_shutdown2' and calling `pj_turn_session_shutdown2' instead of `pj_turn_session_shutdown'. The old `pj_turn_sock_shutdown' remains for ABI compatibility and call the new version `pj_turn_sock_shutdown2' with status `PJ_SUCCESS'. https://git.jami.net/savoirfairelinux/jami-daemon/-/issues/682 Upstream ticket: https://github.com/pjsip/pjproject/issues/3154 Change-Id: I7f902b0a5bb44fd403bd147676d655dc775f2156 --- pjnath/include/pjnath/turn_session.h | 20 ++++++++ pjnath/src/pjnath/ice_session.c | 74 ++++++++++++++-------------- pjnath/src/pjnath/turn_session.c | 9 ++++ pjnath/src/pjnath/turn_sock.c | 15 ++++-- 4 files changed, 76 insertions(+), 42 deletions(-) diff --git a/pjnath/include/pjnath/turn_session.h b/pjnath/include/pjnath/turn_session.h index 837dcf40c..1573fe4a0 100644 --- a/pjnath/include/pjnath/turn_session.h +++ b/pjnath/include/pjnath/turn_session.h @@ -611,6 +611,26 @@ PJ_DECL(pj_status_t) pj_turn_session_create(const pj_stun_config *cfg, PJ_DECL(pj_status_t) pj_turn_session_shutdown(pj_turn_session *sess); +/** + * Shutdown TURN client session. This will gracefully deallocate and + * destroy the client session. + * + * @param sess The TURN client session. + * @param last_err Optional error code to be set to the session, + * which would be returned back in the \a info + * parameter of #pj_turn_session_get_info(). If + * this argument value is PJ_SUCCESS, the error + * code will not be set. If the session already + * has an error code set, this function will not + * overwrite that error code either. + * + * @return PJ_SUCCESS if the operation has been successful, + * or the appropriate error code on failure. + */ +PJ_DECL(pj_status_t) pj_turn_session_shutdown2(pj_turn_session *sess, + pj_status_t last_err); + + /** * Forcefully destroy the TURN session. This will destroy the session * immediately. If there is an active allocation, the server will not diff --git a/pjnath/src/pjnath/ice_session.c b/pjnath/src/pjnath/ice_session.c index bc1e3f715..24f837210 100644 --- a/pjnath/src/pjnath/ice_session.c +++ b/pjnath/src/pjnath/ice_session.c @@ -3180,49 +3180,49 @@ void ice_sess_on_peer_connection(pj_ice_sess *ice, status == PJ_ERRNO_START_SYS + 104 || status == 130054 /* CONNECTION RESET BY PEER */ || status == PJ_ERRNO_START_SYS + 111 /* Connection refused */ )) { - /** - * This part of the code is triggered when using ICE over TCP via TURN - * In fact, the other peer has to authorize this peer to connect to - * the relayed candidate. This is done by set_perm from the other case. - * But from this side, we can't know if the peer has authorized us. If it's - * not the case, the connection will got a CONNECTION RESET BY PEER status. - * In this case, we try to reconnect few times with a delay between two - * attempts. - */ - if (check->reconnect_count < PJ_ICE_TCP_MAX_RECONNECTION_COUNT) { - check->state = PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY; - check_set_state(ice, check,PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY, - status); - check->reconnect_count++; - } else { - // Max attempts reached. Fail this check. - LOG4((ice->obj_name, "Check %s: connection failed after %d attempts", - dump_check(ice->tmp.txt, sizeof(ice->tmp.txt), &ice->clist, check), - PJ_ICE_TCP_MAX_RECONNECTION_COUNT)); - check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, status); - on_check_complete(ice, check); - } - pj_grp_lock_release(ice->grp_lock); - return; + /** + * This part of the code is triggered when using ICE over TCP via TURN + * In fact, the other peer has to authorize this peer to connect to + * the relayed candidate. This is done by set_perm from the other case. + * But from this side, we can't know if the peer has authorized us. If it's + * not the case, the connection will got a CONNECTION RESET BY PEER status. + * In this case, we try to reconnect few times with a delay between two + * attempts. + */ + if (check->reconnect_count < PJ_ICE_TCP_MAX_RECONNECTION_COUNT) { + check->state = PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY; + check_set_state(ice, check,PJ_ICE_SESS_CHECK_STATE_NEEDS_RETRY, + status); + check->reconnect_count++; + } else { + // Max attempts reached. Fail this check. + LOG4((ice->obj_name, "Check %s: connection failed after %d attempts", + dump_check(ice->tmp.txt, sizeof(ice->tmp.txt), &ice->clist, check), + PJ_ICE_TCP_MAX_RECONNECTION_COUNT)); + check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, status); + on_check_complete(ice, check); + } + pj_grp_lock_release(ice->grp_lock); + return; } else if (status != PJ_SUCCESS) { - if (rcand->type == PJ_ICE_CAND_TYPE_RELAYED) { - char raddr[PJ_INET6_ADDRSTRLEN + 10]; - PJ_LOG(4, (ice->obj_name, - "Connection to TURN (%s) failed with status %u", - pj_sockaddr_print(&rcand->addr, raddr, sizeof(raddr), 3), status)); - } - check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, status); - on_check_complete(ice, check); - pj_grp_lock_release(ice->grp_lock); - return; + if (rcand->type == PJ_ICE_CAND_TYPE_RELAYED) { + char raddr[PJ_INET6_ADDRSTRLEN + 10]; + PJ_LOG(4, (ice->obj_name, + "Connection to TURN (%s) failed with status %u", + pj_sockaddr_print(&rcand->addr, raddr, sizeof(raddr), 3), status)); + } + check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, status); + on_check_complete(ice, check); + pj_grp_lock_release(ice->grp_lock); + return; } // TCP is correctly connected. Craft the message to send const pj_ice_sess_cand *lcand = check->lcand; if (check->tdata == NULL) { - LOG5((ice->obj_name, "Error sending STUN request, empty data")); - pj_grp_lock_release(ice->grp_lock); - return; + LOG5((ice->obj_name, "Error sending STUN request, empty data")); + pj_grp_lock_release(ice->grp_lock); + return; } pj_ice_msg_data *msg_data = PJ_POOL_ZALLOC_T(check->tdata->pool, pj_ice_msg_data); diff --git a/pjnath/src/pjnath/turn_session.c b/pjnath/src/pjnath/turn_session.c index 568022bbf..0589f382f 100644 --- a/pjnath/src/pjnath/turn_session.c +++ b/pjnath/src/pjnath/turn_session.c @@ -448,11 +448,20 @@ static void sess_shutdown(pj_turn_session *sess, * Public API to destroy TURN client session. */ PJ_DEF(pj_status_t) pj_turn_session_shutdown(pj_turn_session *sess) +{ + return pj_turn_session_shutdown2(sess, PJ_SUCCESS); +} + +PJ_DEF(pj_status_t) pj_turn_session_shutdown2(pj_turn_session *sess, + pj_status_t last_err) { PJ_ASSERT_RETURN(sess, PJ_EINVAL); pj_grp_lock_acquire(sess->grp_lock); + if (last_err != PJ_SUCCESS && sess->last_status == PJ_SUCCESS) + sess->last_status = last_err; + sess_shutdown(sess, PJ_SUCCESS); pj_grp_lock_release(sess->grp_lock); diff --git a/pjnath/src/pjnath/turn_sock.c b/pjnath/src/pjnath/turn_sock.c index 99e4807bb..6884d8386 100644 --- a/pjnath/src/pjnath/turn_sock.c +++ b/pjnath/src/pjnath/turn_sock.c @@ -428,7 +428,8 @@ static void destroy(pj_turn_sock *turn_sock) pj_grp_lock_release(turn_sock->grp_lock); } -PJ_DEF(void) pj_turn_sock_destroy(pj_turn_sock *turn_sock) +static void turn_sock_destroy(pj_turn_sock *turn_sock, + pj_status_t last_err) { pj_grp_lock_acquire(turn_sock->grp_lock); if (turn_sock->is_destroying) { @@ -437,11 +438,11 @@ PJ_DEF(void) pj_turn_sock_destroy(pj_turn_sock *turn_sock) } if (turn_sock->sess) { - pj_turn_session_shutdown(turn_sock->sess); + pj_turn_session_shutdown2(turn_sock->sess, last_err); /* This will ultimately call our state callback, and when - * session state is DESTROYING we will schedule a timer to - * destroy ourselves. - */ + * session state is DESTROYING we will schedule a timer to + * destroy ourselves. + */ } else { destroy(turn_sock); } @@ -449,6 +450,10 @@ PJ_DEF(void) pj_turn_sock_destroy(pj_turn_sock *turn_sock) pj_grp_lock_release(turn_sock->grp_lock); } +PJ_DEF(void) pj_turn_sock_destroy(pj_turn_sock *turn_sock) +{ + turn_sock_destroy(turn_sock, PJ_SUCCESS); +} /* Timer callback */ static void timer_cb(pj_timer_heap_t *th, pj_timer_entry *e)