Skip to content

Commit

Permalink
dialog: Fix crash due to a "tmp SDP" race condition
Browse files Browse the repository at this point in the history 
This fixes a race condition on the following code which runs, e.g., on a
200 OK to a Re-INVITE (added in d447626):

    if (dlg->legs[leg].tmp_out_sdp.s) {
            shm_free(dlg->legs[leg].tmp_out_sdp.s);
            dlg->legs[leg].tmp_out_sdp.s = 0;       <--- we are here
            dlg->legs[leg].tmp_out_sdp.len = 0;
    }

At this point, if the Re-INVITE is retransmitted and, e.g.,
dlg_callee_reinv_onreq_out() is run, the code may read a corrupt str
value from "tmp_out_sdp" (e.g. {NULL, 172}), which will crash in
shm_str_sync().

Many thanks to Ken Rice for the report!

(cherry picked from commit 6ebbd9a)
  • Loading branch information
@liviuchircu
liviuchircu committed Jan 12, 2021
1 parent e455c5e commit 7c92fff
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 28 deletions.
48 changes: 22 additions & 26 deletions modules/dialog/dlg_handlers.c
View file
Expand Up @@ -92,8 +92,7 @@ int ctx_timeout_idx = -1;
static inline int dlg_update_sdp(struct dlg_cell *dlg, struct sip_msg *msg,
unsigned int leg, int tmp);

static inline void dlg_sync_tmp_sdp(struct dlg_cell *dlg, unsigned int leg);
static inline void dlg_clear_tmp_sdp(struct dlg_cell *dlg, unsigned int leg);
static inline void dlg_merge_tmp_sdp(struct dlg_cell *dlg, unsigned int leg);


void init_dlg_handlers(int default_timeout_p)
Expand Down Expand Up @@ -777,7 +776,7 @@ static void dlg_update_callee_sdp(struct cell* t, int type,

LM_DBG("Status Code received = [%d]\n", statuscode);
if (statuscode == 200) {
dlg_sync_tmp_sdp(dlg, DLG_CALLER_LEG);
dlg_merge_tmp_sdp(dlg, DLG_CALLER_LEG);
dlg_update_sdp(dlg, rpl, callee_idx(dlg), 0);

buffer.s = ((str*)ps->extra1)->s;
Expand All @@ -786,7 +785,7 @@ static void dlg_update_callee_sdp(struct cell* t, int type,
msg=pkg_malloc(sizeof(struct sip_msg));
if (msg==0) {
LM_ERR("no pkg mem left for sip_msg\n");
goto clear;
return;
}

memset(msg,0, sizeof(struct sip_msg));
Expand All @@ -795,16 +794,14 @@ static void dlg_update_callee_sdp(struct cell* t, int type,

if (parse_msg(buffer.s,buffer.len, msg)!=0) {
pkg_free(msg);
goto clear;
return;
}

dlg_update_out_sdp(dlg, callee_idx(dlg), DLG_CALLER_LEG, msg, 0);

free_sip_msg(msg);
pkg_free(msg);
}
clear:
dlg_clear_tmp_sdp(dlg, DLG_CALLER_LEG);
}

static void dlg_update_caller_sdp(struct cell* t, int type,
Expand Down Expand Up @@ -836,7 +833,7 @@ static void dlg_update_caller_sdp(struct cell* t, int type,
LM_DBG("Status Code received = [%d]\n", statuscode);

if (statuscode == 200) {
dlg_sync_tmp_sdp(dlg, callee_idx(dlg));
dlg_merge_tmp_sdp(dlg, callee_idx(dlg));
dlg_update_sdp(dlg, rpl, DLG_CALLER_LEG, 0);

buffer.s = ((str*)ps->extra1)->s;
Expand All @@ -845,7 +842,7 @@ static void dlg_update_caller_sdp(struct cell* t, int type,
msg=pkg_malloc(sizeof(struct sip_msg));
if (msg==0) {
LM_ERR("no pkg mem left for sip_msg\n");
goto clear;
return;
}

memset(msg,0, sizeof(struct sip_msg));
Expand All @@ -854,16 +851,14 @@ static void dlg_update_caller_sdp(struct cell* t, int type,

if (parse_msg(buffer.s,buffer.len, msg)!=0) {
pkg_free(msg);
goto clear;
return;
}

dlg_update_out_sdp(dlg, DLG_CALLER_LEG, callee_idx(dlg),msg, 0);

free_sip_msg(msg);
pkg_free(msg);
}
clear:
dlg_clear_tmp_sdp(dlg, callee_idx(dlg));
}

static void dlg_seq_up_onreply_mod_cseq(struct cell* t, int type,
Expand Down Expand Up @@ -1323,30 +1318,31 @@ static inline int dlg_update_contact(struct dlg_cell *dlg, struct sip_msg *msg,
return ret;
}

static inline void dlg_clear_tmp_sdp(struct dlg_cell *dlg, unsigned int leg)

static inline void dlg_merge_tmp_sdp(struct dlg_cell *dlg, unsigned int leg)
{
dlg_lock_dlg(dlg);

if (dlg->legs[leg].tmp_in_sdp.s) {
if (shm_str_sync(&dlg->legs[leg].in_sdp, &dlg->legs[leg].tmp_in_sdp))
LM_ERR("could not update inbound SDP from temporary SDP!\n");

shm_free(dlg->legs[leg].tmp_in_sdp.s);
dlg->legs[leg].tmp_in_sdp.s = 0;
dlg->legs[leg].tmp_in_sdp.len = 0;
memset(&dlg->legs[leg].tmp_in_sdp, 0, sizeof(str));
}

if (dlg->legs[leg].tmp_out_sdp.s) {
if (shm_str_sync(&dlg->legs[leg].out_sdp, &dlg->legs[leg].tmp_out_sdp))
LM_ERR("could not update outbound SDP from temporary SDP!\n");

shm_free(dlg->legs[leg].tmp_out_sdp.s);
dlg->legs[leg].tmp_out_sdp.s = 0;
dlg->legs[leg].tmp_out_sdp.len = 0;
memset(&dlg->legs[leg].tmp_out_sdp, 0, sizeof(str));
}
}

static inline void dlg_sync_tmp_sdp(struct dlg_cell *dlg, unsigned int leg)
{
if (dlg->legs[leg].tmp_in_sdp.s &&
shm_str_sync(&dlg->legs[leg].in_sdp, &dlg->legs[leg].tmp_in_sdp) < 0)
LM_ERR("could not update inbound SDP from temporary SDP!\n");
if (dlg->legs[leg].tmp_out_sdp.s &&
shm_str_sync(&dlg->legs[leg].out_sdp, &dlg->legs[leg].tmp_out_sdp) < 0)
LM_ERR("could not update outbound SDP from temporary SDP!\n");
dlg_unlock_dlg(dlg);
}


static inline int dlg_update_sdp(struct dlg_cell *dlg, struct sip_msg *msg,
unsigned int leg, int tmp)
{
Expand Down
4 changes: 2 additions & 2 deletions modules/dialog/dlg_hash.h
View file
Expand Up @@ -107,8 +107,8 @@ struct dlg_leg {
str adv_contact; /* topology hiding advertised contact towards this leg - full header */
str in_sdp; /* latest SDP advertised by the uac ( full body ), after all OpenSIPS changes */
str out_sdp; /* latest SDP advertised towards this leg ( full body ), after all OpenSIPS changes */
str tmp_in_sdp; /* temporary stored in_sdp until confirmation (200 OK) arrives */
str tmp_out_sdp; /* temporary stored out until confirmation (200 OK) arrives */
str tmp_in_sdp; /* temporarily stored in_sdp until confirmation (200 OK) arrives */
str tmp_out_sdp; /* temporarily stored out_sdp until confirmation (200 OK) arrives */
str route_uris[64];
int nr_uris;
unsigned int last_gen_cseq; /* FIXME - think this can be atomic_t to avoid locking */
Expand Down

0 comments on commit 7c92fff

Please sign in to comment.