Skip to content

Commit

Permalink
dialog: Change dlg->vals locking to be independent
Browse files Browse the repository at this point in the history 
By making the dlg->vals locking independent from the dialog hash lock,
working with the dialog API will become less prone to AB/BA deadlocks.
Sample possible AB/BA deadlock which is now naturally avoided:

  * module "X" timer grabs lock-A, then gets stuck trying to grab
	lock-B(dialog bucket) through some innocent .store_dlg_val()
	dialog API call

  * dialog DB timer had grabbed lock-B(dialog bucket), and is now stuck
	grabbing lock-A while running a DLGCB_DESTROYED callback
	installed by module "X"
  • Loading branch information
@liviuchircu
liviuchircu committed Jun 26, 2023
1 parent 3fb2c35 commit ad6aef0
Show file tree
Hide file tree
Showing 8 changed files with 185 additions and 92 deletions.
81 changes: 40 additions & 41 deletions modules/dialog/dialog.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1388,7 +1388,6 @@ static int w_get_dlg_info(struct sip_msg *msg, str *attr, pv_spec_t *attr_val,
pv_value_t val;
int n;
unsigned int h;
unsigned short aux;
int type;
int_str isval;

Expand All @@ -1407,39 +1406,38 @@ static int w_get_dlg_info(struct sip_msg *msg, str *attr, pv_spec_t *attr_val,
if ( dlg->state>DLG_STATE_CONFIRMED )
continue;

if (check_dlg_value_unsafe(msg, dlg, key, key_val)==0) {
LM_DBG("dialog found, fetching variable\n");

/* XXX - in lack of an unsafe version of fetch_dlg_value */
aux = dlg->locked_by;
dlg->locked_by = process_no;
lock_start_read(dlg->vals_lock);
/* first, the (@key, @key_val) MUST match */
if (check_dlg_value(msg, dlg, key, key_val, 0) != 0)
goto next_dialog;

/* success! now fetch @attr into @attr_val, within this dialog */
if (fetch_dlg_value_unsafe(dlg, attr, &type, &isval, 0) != 0) {
lock_stop_read(dlg->vals_lock);
dlg_unlock( d_table, d_entry);
LM_ERR("failed to fetch dialog value <%.*s>\n",
(attr)->len, (attr)->s);
return -1;
}

if (fetch_dlg_value( dlg, attr, &type, &isval, 0) ) {
dlg->locked_by = aux;
dlg_unlock( d_table, d_entry);
LM_ERR("failed to fetch dialog value <%.*s>\n",
(attr)->len, (attr)->s);
return -1;
} else {
if (type == DLG_VAL_TYPE_STR) {
val.rs = isval.s;
val.flags = PV_VAL_STR;
} else {
val.ri = isval.n;
val.flags = PV_VAL_INT|PV_TYPE_INT;
}
if (type == DLG_VAL_TYPE_STR) {
val.rs = isval.s;
val.flags = PV_VAL_STR;
} else {
val.ri = isval.n;
val.flags = PV_VAL_INT|PV_TYPE_INT;
}

if (attr_val->setf( msg, &attr_val->pvp, 0, &val )!=0) {
LM_ERR("Failed to set out pvar \n");
dlg->locked_by = aux;
dlg_unlock( d_table, d_entry);
return -1;
} else
n++;
}

dlg->locked_by = aux;
if (attr_val->setf(msg, &attr_val->pvp, 0, &val) != 0) {
lock_stop_read(dlg->vals_lock);
dlg_unlock( d_table, d_entry);
LM_ERR("Failed to set out pvar\n");
return -1;
}

n++;
next_dialog:
lock_stop_read(dlg->vals_lock);
}

dlg_unlock( d_table, d_entry);
Expand Down Expand Up @@ -1481,7 +1479,7 @@ static int w_get_dlg_vals(struct sip_msg *msg, pv_spec_t *v_name,
/* dlg found - NOTE you have a ref! */
LM_DBG("dialog found, fetching all variable\n");

dlg_lock_dlg( dlg );
lock_start_read(dlg->vals_lock);

/* iterate the list with all the dlg variables */
for( dv=dlg->vals ; dv ; dv=dv->next) {
Expand All @@ -1503,15 +1501,15 @@ static int w_get_dlg_vals(struct sip_msg *msg, pv_spec_t *v_name,
if ( pv_set_value( msg, v_val, 0, &val)<0 ) {
LM_ERR("failed to add new value in dlg val list, ignoring\n");
/* better exit here, as we will desync the lists */
dlg_unlock_dlg( dlg );
lock_stop_read(dlg->vals_lock);
unref_dlg(dlg, 1);
return -1;
}
}

}

dlg_unlock_dlg( dlg );
lock_stop_read(dlg->vals_lock);
unref_dlg(dlg, 1);

return 1;
Expand Down Expand Up @@ -1985,6 +1983,8 @@ static char *dlg_get_json_out(struct dlg_cell *dlg,int ctx,int *out_len)
}
*p++=']';

lock_start_read(dlg->vals_lock);

if (ctx && dlg->vals) {
char *it, *end;

Expand All @@ -1995,11 +1995,9 @@ static char *dlg_get_json_out(struct dlg_cell *dlg,int ctx,int *out_len)
k=0;
for( dv=dlg->vals ; dv ; dv=dv->next) {
if (dv->type == DLG_VAL_TYPE_STR)
for (i = 0, j = 0; i < dv->val.s.len; i++) {
if (dv->val.s.s[i] < 0x20 || dv->val.s.s[i] >= 0x7F) {
goto next_val;
}
}
for (i = 0, j = 0; i < dv->val.s.len; i++)
if (dv->val.s.s[i] < 0x20 || dv->val.s.s[i] >= 0x7F)
continue;

if (k!=0) {
*p++ = ',';
Expand Down Expand Up @@ -2055,12 +2053,13 @@ static char *dlg_get_json_out(struct dlg_cell *dlg,int ctx,int *out_len)
memcpy(p,intbuf,intlen);
p += intlen;
}
next_val:;
}

*p++='}';
}

lock_stop_read(dlg->vals_lock);

if (ctx && dlg->profile_links) {
memcpy(p,",\"profiles\":{",13);
p+=13;
Expand Down Expand Up @@ -2215,7 +2214,7 @@ static int w_get_dlg_jsons_by_val(struct sip_msg *msg, str *attr, pv_spec_t *att
if ( dlg->state>DLG_STATE_CONFIRMED )
continue;

if (check_dlg_value_unsafe(msg, dlg, attr, attr_val)==0) {
if (check_dlg_value(msg, dlg, attr, attr_val, 1) == 0) {
LM_DBG("dialog found, fetching variable\n");

if ((out_json = dlg_get_json_out(dlg,1,&out_len)) == NULL) {
Expand Down
37 changes: 26 additions & 11 deletions modules/dialog/dlg_db_handler.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,8 @@ void read_dialog_vars(char *b, int l, struct dlg_cell *dlg)

end = b + l;
p = b;

lock_start_write(dlg->vals_lock);
do {
/* read a new pair from input string */
p = read_pair( p, end, &name, &isval, &type);
Expand All @@ -434,6 +436,7 @@ void read_dialog_vars(char *b, int l, struct dlg_cell *dlg)
LM_ERR("failed to add val, skipping...\n");
} while(p!=end);

lock_stop_write(dlg->vals_lock);
}


Expand Down Expand Up @@ -1279,7 +1282,7 @@ static inline unsigned int write_pair( char *b, str *name, str *name_suffix,
}


str* write_dialog_vars( struct dlg_val *vars)
str* write_dialog_vars( struct dlg_cell *dlg)
{
static str o = {NULL,0};
static int o_l=0;
Expand All @@ -1288,8 +1291,10 @@ str* write_dialog_vars( struct dlg_val *vars)
char *p;
int intlen;

lock_start_read(dlg->vals_lock);

/* compute the required len */
for ( v=vars,l=0 ; v ; v=v->next) {
for ( v=dlg->vals,l=0 ; v ; v=v->next) {
l += v->name.len + 1/*'#'*/ + 1/*type char*/ + 1;/*'#'*/
if (v->type == DLG_VAL_TYPE_STR) {
l += v->val.s.len;
Expand All @@ -1313,16 +1318,19 @@ str* write_dialog_vars( struct dlg_val *vars)
if (o.s) pkg_free(o.s);
o.s = (char*)pkg_malloc(l);
if (o.s==NULL) {
lock_stop_read(dlg->vals_lock);
LM_ERR("not enough pkg mem (req=%d)\n",l);
return NULL;
}
o_l = l;
}

lock_stop_read(dlg->vals_lock);

/* write the stuff into it */
o.len = l;
p = o.s;
for ( v=vars ; v ; v=v->next) {
for ( v=dlg->vals ; v ; v=v->next) {
p += write_pair( p, &v->name,NULL, &v->val, v->type);
}
if (o.len!=p-o.s) {
Expand Down Expand Up @@ -1409,53 +1417,60 @@ int persist_reinvite_pinging(struct dlg_cell *dlg)
return 0;
}

lock_start_write(dlg->vals_lock);

val.s = dlg->legs[DLG_CALLER_LEG].in_sdp;
if (dlg->legs[DLG_CALLER_LEG].in_sdp.len &&
store_dlg_value_unsafe(dlg, &caller_in_sdp,
&val, DLG_VAL_TYPE_STR) != 0) {
LM_ERR("failed to persist caller UAC SDP\n");
return -1;
goto error;
}

val.s = dlg->legs[DLG_CALLER_LEG].out_sdp;
if (dlg->legs[DLG_CALLER_LEG].out_sdp.len &&
store_dlg_value_unsafe(dlg, &caller_out_sdp,
&val, DLG_VAL_TYPE_STR) != 0) {
LM_ERR("failed to persist caller advertised SDP\n");
return -1;
goto error;
}

val.s = dlg->legs[DLG_CALLER_LEG].adv_contact;
if (store_dlg_value_unsafe(dlg, &caller_adv_ct,
&val, DLG_VAL_TYPE_STR) != 0) {
LM_ERR("failed to persist caller advertised Contact\n");
return -1;
goto error;
}

val.s = dlg->legs[dlg->legs_no[DLG_LEG_200OK]].in_sdp;
if (dlg->legs[dlg->legs_no[DLG_LEG_200OK]].in_sdp.len &&
store_dlg_value_unsafe(dlg, &callee_in_sdp,
&val, DLG_VAL_TYPE_STR) != 0) {
LM_ERR("failed to persist callee UAC SDP\n");
return -1;
goto error;
}

val.s = dlg->legs[dlg->legs_no[DLG_LEG_200OK]].out_sdp;
if (dlg->legs[dlg->legs_no[DLG_LEG_200OK]].out_sdp.len &&
store_dlg_value_unsafe(dlg, &callee_out_sdp,
&val, DLG_VAL_TYPE_STR) != 0) {
LM_ERR("failed to persist callee advertised SDP\n");
return -1;
goto error;
}

val.s = dlg->legs[dlg->legs_no[DLG_LEG_200OK]].adv_contact;
if (store_dlg_value_unsafe(dlg, &callee_adv_ct,
&val, DLG_VAL_TYPE_STR) != 0) {
LM_ERR("failed to persist callee advertised Contact\n");
return -1;
goto error;
}

lock_stop_write(dlg->vals_lock);
return 0;

error:
lock_stop_write(dlg->vals_lock);
return -1;
}

/* re-populate the SDPs/Contacts of caller/callee(s) from dlg val storage */
Expand Down Expand Up @@ -1576,7 +1591,7 @@ static inline void set_final_update_cols(db_val_t *vals, struct dlg_cell *cell,

/* save sharing tag name as dlg val */
val.s = cell->shtag;
if (cell->shtag.s && store_dlg_value_unsafe(cell, &shtag_dlg_val, &val,
if (cell->shtag.s && store_dlg_value(cell, &shtag_dlg_val, &val,
DLG_VAL_TYPE_STR) < 0)
LM_ERR("Failed to store sharing tag %.*s(%p) as dlg val\n",
cell->shtag.len, cell->shtag.s, cell->shtag.s);
Expand All @@ -1585,7 +1600,7 @@ static inline void set_final_update_cols(db_val_t *vals, struct dlg_cell *cell,
if (cell->vals==NULL) {
VAL_NULL(vals) = 1;
} else {
s = write_dialog_vars( cell->vals );
s = write_dialog_vars( cell );
if (s==NULL) {
VAL_NULL(vals) = 1;
} else {
Expand Down
2 changes: 1 addition & 1 deletion modules/dialog/dlg_db_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ void dialog_update_db(unsigned int ticks, void * param);
void read_dialog_vars(char *b, int l, struct dlg_cell *dlg);
void read_dialog_profiles(char *b, int l, struct dlg_cell *dlg,
int double_check, char is_replicated);
str* write_dialog_vars(struct dlg_val *vars);
str* write_dialog_vars(struct dlg_cell *dlg);
str* write_dialog_profiles(struct dlg_profile_link *links);

mi_response_t *mi_sync_db_dlg(const mi_params_t *params,
Expand Down
37 changes: 29 additions & 8 deletions modules/dialog/dlg_hash.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,13 @@ static inline void free_dlg_dlg(struct dlg_cell *dlg)

#ifdef DBG_DIALOG
sh_log(dlg->hist, DLG_DESTROY, "ref %d", dlg->ref);
sh_unref(dlg->hist);
dlg->hist = NULL;
if (dlg->hist) {
sh_unref(dlg->hist);
dlg->hist = NULL;
}
#endif

lock_destroy_rw(dlg->vals_lock);
shm_free(dlg);
}

Expand Down Expand Up @@ -321,17 +324,23 @@ struct dlg_cell* build_new_dlg( str *callid, str *from_uri, str *to_uri,

memset(dlg, 0, len);

dlg->vals_lock = lock_init_rw();
if (!dlg->vals_lock) {
LM_ERR("oom\n");
shm_free(dlg);
return NULL;
}

#if defined(DBG_DIALOG)
dlg->hist = sh_push(dlg, dlg_hist);
if (!dlg->hist) {
LM_ERR("oom\n");
shm_free(dlg);
free_dlg_dlg(dlg);
return NULL;
}
#endif

dlg->state = DLG_STATE_UNCONFIRMED;

dlg->h_entry = dlg_hash( callid);

LM_DBG("new dialog %p (c=%.*s,f=%.*s,t=%.*s,ft=%.*s) on hash %u\n",
Expand Down Expand Up @@ -874,7 +883,7 @@ struct dlg_cell* get_dlg_by_val(struct sip_msg *msg, str *attr, pv_spec_t *val)
LM_DBG("dlg in state %d to check\n",dlg->state);
if ( dlg->state>DLG_STATE_CONFIRMED )
continue;
if (check_dlg_value_unsafe(msg, dlg, attr, val)==0) {
if (check_dlg_value(msg, dlg, attr, val, 1)==0) {
ref_dlg_unsafe( dlg, 1);
dlg_unlock( d_table, d_entry);
return dlg;
Expand Down Expand Up @@ -1481,10 +1490,14 @@ static inline int internal_mi_print_dlg(mi_item_t *dialog_obj,
if (!context_obj)
goto error;

lock_start_read(dlg->vals_lock);

if (dlg->vals) {
values_arr = add_mi_array(context_obj, MI_SSTR("values"));
if (!values_arr)
if (!values_arr) {
lock_stop_read(dlg->vals_lock);
goto error;
}

/* print dlg values -> iterate the list */
for( dv=dlg->vals ; dv ; dv=dv->next) {
Expand Down Expand Up @@ -1516,10 +1529,16 @@ static inline int internal_mi_print_dlg(mi_item_t *dialog_obj,
}

values_item = add_mi_object(values_arr, NULL, 0);
if (!values_item)
if (!values_item) {
lock_stop_read(dlg->vals_lock);
goto error;
if (add_mi_string(values_item,dv->name.s,dv->name.len,p,j) < 0)
}

if (add_mi_string(values_item,dv->name.s,dv->name.len,p,j) < 0) {
lock_stop_read(dlg->vals_lock);
goto error;
}

} else {
values_item = add_mi_object(values_arr, NULL, 0);
if (!values_item)
Expand All @@ -1531,6 +1550,8 @@ static inline int internal_mi_print_dlg(mi_item_t *dialog_obj,
}
}

lock_stop_read(dlg->vals_lock);

/* print dlg profiles */
if (dlg->profile_links) {
profiles_arr = add_mi_array(context_obj, MI_SSTR("profiles"));
Expand Down

0 comments on commit ad6aef0

Please sign in to comment.