Skip to content

Commit

Permalink
pjnath/turn_sock: Fix destruction of TURN sockets
Browse files Browse the repository at this point in the history 
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: pjsip/pjproject#3154

Change-Id: I7f902b0a5bb44fd403bd147676d655dc775f2156
  • Loading branch information
Olivier Dion authored and Sébastien Blin committed Sep 7, 2023
1 parent 6eb7a17 commit 16bf8a3
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 42 deletions.
20 changes: 20 additions & 0 deletions pjnath/include/pjnath/turn_session.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
74 changes: 37 additions & 37 deletions pjnath/src/pjnath/ice_session.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 9 additions & 0 deletions pjnath/src/pjnath/turn_session.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
15 changes: 10 additions & 5 deletions pjnath/src/pjnath/turn_sock.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -437,18 +438,22 @@ 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);
}

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)
Expand Down

0 comments on commit 16bf8a3

Please sign in to comment.