Skip to content

Commit

Permalink
Fix memory corruption on $stat() vars
Browse files Browse the repository at this point in the history
The $stat() var specs are kept in pkg, while the name (if stat not found) is kept in memeory. This means all the copies (form all procs) of the specs will point to a single shm block. It is not safet to free this block as you have no idea how many copies of the spec (from other processes) still refer the name.
Closes #902
  • Loading branch information
bogdan-iancu committed Jun 16, 2016
1 parent f923d2f commit 4baead7
Showing 1 changed file with 17 additions and 2 deletions.
19 changes: 17 additions & 2 deletions modules/statistics/statistics.c
Expand Up @@ -323,6 +323,7 @@ int pv_parse_name(pv_spec_p sp, str *in)
if(in==NULL || in->s==NULL || sp==NULL)
return -1;

LM_NOTICE("xXx name %p with name <%.*s>\n", &sp->pvp.pvn, in->len, in->s);
if (pv_parse_format( in, &format)!=0) {
LM_ERR("failed to parse statistic name format <%.*s> \n",
in->len,in->s);
Expand All @@ -343,10 +344,13 @@ int pv_parse_name(pv_spec_p sp, str *in)
LM_ERR("failed to clone name of statistic \n");
return -1;
}
LM_NOTICE("xXx name %p, name cloned (in=%p, out=%p)\n",
&sp->pvp.pvn, in->s, sp->pvp.pvn.u.isname.name.s.s);
} else {
/* link the stat pointer directly as dynamic name */
sp->pvp.pvn.type = PV_NAME_PVAR;
sp->pvp.pvn.u.dname = (void*)stat;
LM_NOTICE("xXx name %p, stat found\n", &sp->pvp.pvn);
}

} else {
Expand All @@ -355,6 +359,7 @@ int pv_parse_name(pv_spec_p sp, str *in)
sp->pvp.pvn.u.isname.type = 0; /* not string */
sp->pvp.pvn.u.isname.name.s.s = (char*)(void*)format;
sp->pvp.pvn.u.isname.name.s.len = 0;
LM_NOTICE("xXx name %p, stat name is FMT\n", &sp->pvp.pvn);

}

Expand All @@ -369,6 +374,7 @@ static inline int get_stat_name(struct sip_msg* msg, pv_name_t *name,

/* is the statistic found ? */
if (name->type==PV_NAME_INTSTR) {
LM_NOTICE("xXx stat with name %p still not found\n", name);
/* not yet :( */
/* do we have at least the name ?? */
if (name->u.isname.type==0) {
Expand All @@ -384,6 +390,8 @@ static inline int get_stat_name(struct sip_msg* msg, pv_name_t *name,
}
/* lookup for the statistic */
*stat = get_stat( &pv_val.rs );
LM_NOTICE("xXx stat name %p (%.*s) after lookup is %p\n",
name, pv_val.rs.len, pv_val.rs.s, *stat);
if (*stat==NULL) {
if (!create)
return 0;
Expand All @@ -396,9 +404,15 @@ static inline int get_stat_name(struct sip_msg* msg, pv_name_t *name,
return -1;
}
}
/* if name is static string, better link the stat directly and discard name */
/* if name is static string, better link the stat directly
* and discard name */
if (name->u.isname.type==AVP_NAME_STR) {
shm_free(name->u.isname.name.s.s);
LM_NOTICE("xXx name %p freeing %p\n",name,name->u.isname.name.s.s);
/* it is totally unsafe to free this shm block here, as it is
* referred by the spec from all the processess. Even if we create
* here a small leak (one time only), we do not have a better fix
* until a final review of the specs in pkg and shm mem - bogdan */
//shm_free(name->u.isname.name.s.s);
name->u.isname.name.s.s = NULL;
name->u.isname.name.s.len = 0;
name->type = PV_NAME_PVAR;
Expand All @@ -407,6 +421,7 @@ static inline int get_stat_name(struct sip_msg* msg, pv_name_t *name,
} else {
/* stat already found ! */
*stat = (stat_var*)name->u.dname;
LM_NOTICE("xXx stat name %p is founded\n",name);
}

return 0;
Expand Down

0 comments on commit 4baead7

Please sign in to comment.