Crash in on_stun_request_complete

[AmarOk1412]

Describe the bug

Hi,

I use pjproject to initiate calls and discovered a crash in on_stun_request_complete while initializing a call.

This bug is located in 7.1.2.2.1., if no lcand is found, we try to add a candidate and then use it to compute the priority.

However, in one case (if the candidate is a deprecated IPv6), no candidate will be added but the ICE negotiation MUST NOT fail (the check however SHOULD).

Steps to reproduce

1. On a router, only have deprecated ipv6 addresses e.g.:

3: wlp4s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether e4:70:b8:42:38:cb brd ff:ff:ff:ff:ff:ff
    inet 192.168.1.52/24 brd 192.168.1.255 scope global dynamic noprefixroute wlp4s0
       valid_lft 79238sec preferred_lft 79238sec
    inet6 fde4:fb5d:99a0:1500:446d:68f6:1f2c:6a17/64 scope global temporary deprecated dynamic 
       valid_lft 953sec preferred_lft 0sec
    inet6 fde4:fb5d:99a0:1500:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 953sec preferred_lft 0sec
    inet6 2a06:4282:12:9498:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 731sec preferred_lft 0sec
    inet6 2a06:4282:12:9497:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 403sec preferred_lft 0sec
    inet6 2a06:4282:12:9496:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 772sec preferred_lft 0sec
    inet6 2a06:4282:12:9489:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 226sec preferred_lft 0sec
    inet6 2a06:4282:12:9471:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 555sec preferred_lft 0sec
    inet6 2a06:4282:12:9424:e670:b8ff:fe42:38cb/64 scope global deprecated dynamic mngtmpaddr noprefixroute 
       valid_lft 600sec preferred_lft 0sec
    inet6 2a06:4282:12:9496:7c6c:2517:fd62:62f2/64 scope global temporary deprecated dynamic 
       valid_lft 772sec preferred_lft 0sec
    inet6 2a06:4282:12:9498:7c6c:2517:fd62:62f2/64 scope global temporary deprecated dynamic 
       valid_lft 731sec preferred_lft 0sec
    inet6 2a06:4282:12:9424:7c6c:2517:fd62:62f2/64 scope global temporary deprecated dynamic 
       valid_lft 601sec preferred_lft 0sec
    inet6 2a06:4282:12:9471:7c6c:2517:fd62:62f2/64 scope global temporary deprecated dynamic 
       valid_lft 555sec preferred_lft 0sec
    inet6 fde4:fb5d:99a0:1500:7c6c:2517:fd62:62f2/64 scope global temporary deprecated dynamic 
       valid_lft 952sec preferred_lft 0sec
    inet6 2a06:4282:12:9489:7c6c:2517:fd62:62f2/64 scope global temporary deprecated dynamic 
       valid_lft 226sec preferred_lft 0sec
    inet6 2a06:4282:12:9489:b0cc:5a38:1ce4:d2bd/64 scope global temporary deprecated dynamic 
       valid_lft 38sec preferred_lft 0sec
    inet6 2a06:4282:12:9497:b0cc:5a38:1ce4:d2bd/64 scope global temporary deprecated dynamic 
       valid_lft 38sec preferred_lft 0sec
    inet6 fde4:fb5d:99a0:1500:b0cc:5a38:1ce4:d2bd/64 scope global temporary deprecated dynamic 

2. Negotiate an ipv6 address Got:

0x00007fffe8b21181 in on_stun_request_complete ()
   from /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
(gdb) bt
#0  0x00007fffe8b21181 in on_stun_request_complete ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#1  0x00007fffe8b2ef40 in stun_tsx_on_complete ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#2  0x00007fffe8b3327d in pj_stun_client_tsx_on_rx_msg ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#3  0x00007fffe8b2f8d1 in pj_stun_session_on_rx_pkt ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#4  0x00007fffe8b22e6c in pj_ice_sess_on_rx_pkt ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#5  0x00007fffe8b23093 in stun_on_rx_data ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#6  0x00007fffe8b2ff47 in parse_rx_packet ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#7  0x00007fffe8b4bfed in ioqueue_on_read_complete ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#8  0x00007fffe8b46fdc in ioqueue_dispatch_read_event ()
    at /home/sblin/Projects/jami/daemon/src/.libs/libring.so.0
#9  0x00007fffe8b48abb in pj_ioqueue_poll ()

PJSIP version

2.10

Context

Probably not windows, but others should be impacted.

Log, call stack, etc

Proposed patch if we want to keep the success with deprecated address (else we need an error code and change other parts):


 pjnath/src/pjnath/ice_session.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/pjnath/src/pjnath/ice_session.c b/pjnath/src/pjnath/ice_session.c
index 80681e5a..797c4428 100644
--- a/pjnath/src/pjnath/ice_session.c
+++ b/pjnath/src/pjnath/ice_session.c
@@ -3622,7 +3622,7 @@ static void on_stun_request_complete(pj_stun_session *stun_sess,
      * address represents a new candidate - a peer reflexive candidate.
      */
     if (lcand == NULL) {
-	unsigned cand_id;
+	unsigned cand_id = ice->lcand_cnt;
 	pj_str_t foundation;
 
 	pj_ice_calc_foundation(ice->pool, &foundation, PJ_ICE_CAND_TYPE_PRFLX,
@@ -3638,7 +3638,7 @@ static void on_stun_request_complete(pj_stun_session *stun_sess,
 	 */
 
 	/* Add new peer reflexive candidate */
-	status = pj_ice_sess_add_cand(ice, check->lcand->comp_id, 
+	status = pj_ice_sess_add_cand(ice, check->lcand->comp_id,
 				      msg_data->transport_id,
 				      PJ_ICE_CAND_TYPE_PRFLX,
 #if PJNATH_ICE_PRIO_STD
@@ -3648,15 +3648,23 @@ static void on_stun_request_complete(pj_stun_session *stun_sess,
 				      ice->lcand_cnt,
 #endif
 				      &foundation,
-				      &xaddr->sockaddr, 
-				      &check->lcand->base_addr, 
+				      &xaddr->sockaddr,
+				      &check->lcand->base_addr,
 				      &check->lcand->base_addr,
 				      pj_sockaddr_get_len(&xaddr->sockaddr),
 				      &cand_id,
 				      check->rcand->transport == PJ_CAND_UDP ?
 				      PJ_CAND_UDP : PJ_CAND_TCP_PASSIVE);
-	if (status != PJ_SUCCESS) {
-	    check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED, 
+	// Note: for IPv6, pj_ice_sess_add_cand can return SUCCESS
+	// without adding any candidates if the candidate is
+	// deprecated (because the ICE MUST NOT fail)
+	// In this case, cand_id == ice->lcand_cnt will be true.
+	if (status != PJ_SUCCESS || cand_id == ice->lcand_cnt) {
+	    if (cand_id == ice->lcand_cnt) {
+                LOG4((ice->obj_name,
+                  "Cannot add any candidate, all IPv6 seems deprecated"));
+	    }
+	    check_set_state(ice, check, PJ_ICE_SESS_CHECK_STATE_FAILED,
 			    status);
 	    on_check_complete(ice, check);
 	    pj_grp_lock_release(ice->grp_lock);

[silentindark]

@AmarOk1412 Would you be able to create a pull request for this, then pjproject team can review it.

[AmarOk1412]

Sure I'll