Changeset 6069


Ignore:
Timestamp:
Sep 12, 2019 8:46:05 AM (13 months ago)
Author:
nanang
Message:

Fix #2230: Fixed crash in STUN session due to race condition which leads to premature tdata destroy.

Location:
pjproject/trunk/pjnath
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • pjproject/trunk/pjnath/include/pjnath/stun_session.h

    r4606 r6069  
    349349    pj_uint32_t          msg_magic;     /**< Message magic.                 */ 
    350350    pj_uint8_t           msg_key[12];   /**< Message/transaction key.       */ 
     351     
     352    pj_grp_lock_t       *grp_lock;      /**< Group lock (for resp cache).   */ 
    351353 
    352354    pj_stun_req_cred_info auth_info;    /**< Credential info                */ 
  • pjproject/trunk/pjnath/src/pjnath/stun_session.c

    r5983 r6069  
    7777static void stun_tsx_on_destroy(pj_stun_client_tsx *tsx); 
    7878static void stun_sess_on_destroy(void *comp); 
     79static void destroy_tdata(pj_stun_tx_data *tdata, pj_bool_t force); 
    7980 
    8081static pj_stun_tsx_cb tsx_cb =  
     
    154155        pj_grp_lock_acquire(sess->grp_lock); 
    155156        tsx_erase(sess, tdata); 
    156         pj_pool_release(tdata->pool); 
     157        destroy_tdata(tdata, PJ_TRUE); 
    157158        pj_grp_lock_release(sess->grp_lock); 
    158159    } 
     
    161162 
    162163    TRACE_((THIS_FILE, "STUN transaction %p destroyed", tsx)); 
     164} 
     165 
     166static void tdata_on_destroy(void *arg) 
     167{ 
     168    pj_stun_tx_data *tdata = (pj_stun_tx_data*)arg; 
     169 
     170    pj_pool_safe_release(&tdata->pool); 
    163171} 
    164172 
     
    168176            force, tdata->client_tsx)); 
    169177 
     178    /* STUN session may have been destroyed, except when tdata is cached. */ 
     179 
    170180    if (tdata->res_timer.id != PJ_FALSE) { 
    171181        pj_timer_heap_cancel_if_active(tdata->sess->cfg->timer_heap, 
    172                                        &tdata->res_timer, PJ_FALSE); 
    173         pj_list_erase(tdata); 
     182                                       &tdata->res_timer, PJ_FALSE); 
    174183    } 
    175184 
     
    180189            pj_stun_client_tsx_set_data(tdata->client_tsx, NULL); 
    181190        } 
    182         pj_pool_release(tdata->pool); 
     191        if (tdata->grp_lock) { 
     192            pj_grp_lock_dec_ref(tdata->sess->grp_lock); 
     193            pj_grp_lock_dec_ref(tdata->grp_lock); 
     194        } else { 
     195            tdata_on_destroy(tdata); 
     196        } 
    183197 
    184198    } else { 
     
    189203 
    190204        } else { 
    191             pj_pool_release(tdata->pool); 
     205            pj_list_erase(tdata); 
     206            if (tdata->grp_lock) { 
     207                pj_grp_lock_dec_ref(tdata->sess->grp_lock); 
     208                pj_grp_lock_dec_ref(tdata->grp_lock); 
     209            } else { 
     210                tdata_on_destroy(tdata); 
     211            } 
    192212        } 
    193213    } 
     
    226246    PJ_LOG(5,(SNAME(tdata->sess), "Response cache deleted")); 
    227247 
    228     pj_list_erase(tdata); 
     248    destroy_tdata(tdata, PJ_FALSE); 
    229249    pj_grp_lock_release(sess->grp_lock); 
    230250 
    231     destroy_tdata(tdata, PJ_FALSE); 
    232251} 
    233252 
     
    565584    } 
    566585 
    567     while (!pj_list_empty(&sess->cached_response_list)) { 
    568         pj_stun_tx_data *tdata = sess->cached_response_list.next; 
    569         destroy_tdata(tdata, PJ_TRUE); 
    570     } 
    571  
    572     if (sess->rx_pool) { 
    573         pj_pool_release(sess->rx_pool); 
    574         sess->rx_pool = NULL; 
    575     } 
    576  
    577     pj_pool_release(sess->pool); 
     586    pj_pool_safe_release(&sess->rx_pool); 
     587    pj_pool_safe_release(&sess->pool); 
    578588 
    579589    TRACE_((THIS_FILE, "STUN session %p destroyed", sess)); 
     
    599609    sess->is_destroying = PJ_TRUE; 
    600610 
    601     /* We need to stop transactions and cached response because they are 
     611    /* We need to stop transactions because they are 
    602612     * holding the group lock's reference counter while retransmitting. 
    603613     */ 
     
    609619    } 
    610620 
    611     tdata = sess->cached_response_list.next; 
    612     while (tdata != &sess->cached_response_list) { 
    613         pj_timer_heap_cancel_if_active(tdata->sess->cfg->timer_heap, 
    614                                        &tdata->res_timer, PJ_FALSE); 
    615         tdata = tdata->next; 
     621    /* Destroy cached response within session lock protection to avoid 
     622     * race scenario with on_cache_timeout(). 
     623     */ 
     624    while (!pj_list_empty(&sess->cached_response_list)) { 
     625        pj_stun_tx_data *tdata = sess->cached_response_list.next; 
     626        destroy_tdata(tdata, PJ_TRUE); 
    616627    } 
    617628 
     
    805816on_error: 
    806817    if (tdata) 
    807         pj_pool_release(tdata->pool); 
     818        pj_pool_safe_release(&tdata->pool); 
    808819    pj_grp_lock_release(sess->grp_lock); 
    809820    return status; 
     
    836847                                NULL, &tdata->msg); 
    837848    if (status != PJ_SUCCESS) { 
    838         pj_pool_release(tdata->pool); 
     849        pj_pool_safe_release(&tdata->pool); 
    839850        pj_grp_lock_release(sess->grp_lock); 
    840851        return status; 
     
    875886                                         err_code, err_msg, &tdata->msg); 
    876887    if (status != PJ_SUCCESS) { 
    877         pj_pool_release(tdata->pool); 
     888        pj_pool_safe_release(&tdata->pool); 
    878889        pj_grp_lock_release(sess->grp_lock); 
    879890        return status; 
     
    10081019 
    10091020    } else { 
     1021        /* Otherwise for non-request message, send directly to transport. */ 
    10101022        if (cache_res &&  
    10111023            (PJ_STUN_IS_SUCCESS_RESPONSE(tdata->msg->hdr.type) || 
     
    10141026            /* Requested to keep the response in the cache */ 
    10151027            pj_time_val timeout; 
     1028 
     1029            status = pj_grp_lock_create(tdata->pool, NULL, &tdata->grp_lock); 
     1030            if (status != PJ_SUCCESS) { 
     1031                pj_stun_msg_destroy_tdata(sess, tdata); 
     1032                LOG_ERR_(sess, "Error creating group lock", status); 
     1033                goto on_return; 
     1034            } 
     1035            pj_grp_lock_add_ref(tdata->grp_lock); 
     1036            pj_grp_lock_add_handler(tdata->grp_lock, tdata->pool, tdata, 
     1037                                    &tdata_on_destroy); 
     1038 
     1039            /* Also add ref session group lock to make sure that the session 
     1040             * is still valid when cache timeout callback is called. 
     1041             */ 
     1042            pj_grp_lock_add_ref(sess->grp_lock); 
    10161043             
    10171044            pj_memset(&tdata->res_timer, 0, sizeof(tdata->res_timer)); 
     
    10251052                                                       &tdata->res_timer, 
    10261053                                                       &timeout, PJ_TRUE, 
    1027                                                        sess->grp_lock); 
     1054                                                       tdata->grp_lock); 
    10281055            if (status != PJ_SUCCESS) { 
    10291056                pj_stun_msg_destroy_tdata(sess, tdata); 
     
    10341061            pj_list_push_back(&sess->cached_response_list, tdata); 
    10351062        } 
    1036      
    1037         /* Otherwise for non-request message, send directly to transport. */ 
     1063 
     1064        /* Send to transport directly. */ 
    10381065        status = sess->cb.on_send_msg(sess, token, tdata->pkt,  
    10391066                                      tdata->pkt_size, server, addr_len); 
Note: See TracChangeset for help on using the changeset viewer.