[Asterisk-Dev] Questions Regarding the ast_copy_string() Janitor
Project
Matt Roth
mroth at imminc.com
Tue Jan 10 10:28:02 MST 2006
The ast_copy_string() janitor project is defined as follows:
- Check all ast_copy_string() usage to ensure that buffers are not being
unnecessarily zeroed before or after calling it.
In apps/app_queue.c I found some instances of memset() being used to
zero a buffer that is later being passed to ast_copy_string(). It
appears to be unnecessary in the existing code, but the call to memset()
is not immediately prior to the call to ast_copy_string(). My concern
is that memset() is being called as a safeguard in case the code is
changed in the future and not do to a lack of understanding about how
ast_copy_string() works. I need clarification as to when it's
appropriate to remove the calls to memset(). Should it only be done
when they immediately precede the call to ast_copy_string() or any time
a buffer is initialized to zero and then isn't referenced again until it
is passed to ast_copy_string()?
I'm also assuming that using memset() to zero an entire struct
containing strings that are then passed to ast_copy_string() is outside
the case of this janitor project. Is that correct?
Here are two questionable examples from apps/app_queue.c:
/* The zeroed buffer is tmpbuf */
static struct ast_call_queue *find_queue_by_name_rt(const char
*queuename, struct ast_variable *queue_vars, struct ast_config
*member_config)
{
struct ast_variable *v;
struct ast_call_queue *q, *prev_q;
struct member *m, *prev_m, *next_m;
char *interface;
char *tmp, *tmp_name;
char tmpbuf[64]; /* Must be longer than the longest queue param
name. */
/* Find the queue in the in-core list (we will create a new one if
not found). */
q = queues;
prev_q = NULL;
while (q) {
if (!strcasecmp(q->name, queuename)) {
break;
}
q = q->next;
prev_q = q;
}
/* Static queues override realtime. */
if (q) {
ast_mutex_lock(&q->lock);
if (!q->realtime) {
if (q->dead) {
ast_mutex_unlock(&q->lock);
return NULL;
} else {
return q;
}
}
} else if (!member_config)
/* Not found in the list, and it's not realtime ... */
return NULL;
/* Check if queue is defined in realtime. */
if (!queue_vars) {
/* Delete queue from in-core list if it has been deleted in
realtime. */
if (q) {
/*! \note Hmm, can't seem to distinguish a DB failure from a not
found condition... So we might delete an in-core queue
in case of DB failure. */
ast_log(LOG_DEBUG, "Queue %s not found in realtime.\n",
queuename);
q->dead = 1;
/* Delete if unused (else will be deleted when last caller
leaves). */
if (!q->count) {
/* Delete. */
if (!prev_q) {
queues = q->next;
} else {
prev_q->next = q->next;
}
ast_mutex_unlock(&q->lock);
free(q);
} else
ast_mutex_unlock(&q->lock);
}
return NULL;
}
/* Create a new queue if an in-core entry does not exist yet. */
if (!q) {
q = alloc_queue(queuename);
if (!q)
return NULL;
ast_mutex_lock(&q->lock);
clear_queue(q);
q->realtime = 1;
q->next = queues;
queues = q;
}
init_queue(q); /* Ensure defaults for all parameters not set
explicitly. */
v = queue_vars;
memset(tmpbuf, 0, sizeof(tmpbuf));
while(v) {
/* Convert to dashes `-' from underscores `_' as the latter are
more SQL friendly. */
if((tmp = strchr(v->name, '_')) != NULL) {
ast_copy_string(tmpbuf, v->name, sizeof(tmpbuf));
tmp_name = tmpbuf;
tmp = tmp_name;
while((tmp = strchr(tmp, '_')) != NULL)
*tmp++ = '-';
} else
tmp_name = v->name;
queue_set_param(q, tmp_name, v->value, -1, 0);
v = v->next;
}
/* Temporarily set non-dynamic members dead so we can detect deleted
ones. */
m = q->members;
while (m) {
if (!m->dynamic)
m->dead = 1;
m = m->next;
}
interface = ast_category_browse(member_config, NULL);
while (interface) {
rt_handle_member_record(q, interface,
ast_variable_retrieve(member_config, interface, "penalty"));
interface = ast_category_browse(member_config, interface);
}
/* Delete all realtime members that have been deleted in DB. */
m = q->members;
prev_m = NULL;
while (m) {
next_m = m->next;
if (m->dead) {
if (prev_m) {
prev_m->next = next_m;
} else {
q->members = next_m;
}
free(m);
} else {
prev_m = m;
}
m = next_m;
}
return q;
}
/* The zeroed buffer is interface */
static void reload_queues(void)
{
struct ast_call_queue *q, *ql, *qn;
struct ast_config *cfg;
char *cat, *tmp;
struct ast_variable *var;
struct member *prev, *cur;
int new;
char *general_val = NULL;
char interface[80];
int penalty;
cfg = ast_config_load("queues.conf");
if (!cfg) {
ast_log(LOG_NOTICE, "No call queueing config file (queues.conf),
so no call queues\n");
return;
}
memset(interface, 0, sizeof(interface));
ast_mutex_lock(&qlock);
use_weight=0;
/* Mark all queues as dead for the moment */
q = queues;
while(q) {
q->dead = 1;
q = q->next;
}
/* Chug through config file */
cat = ast_category_browse(cfg, NULL);
while(cat) {
if (!strcasecmp(cat, "general")) {
/* Initialize global settings */
queue_persistent_members = 0;
if ((general_val = ast_variable_retrieve(cfg, "general",
"persistentmembers")))
queue_persistent_members = ast_true(general_val);
} else { /* Define queue */
/* Look for an existing one */
q = queues;
while(q) {
if (!strcmp(q->name, cat))
break;
q = q->next;
}
if (!q) {
/* Make one then */
q = alloc_queue(cat);
new = 1;
} else
new = 0;
if (q) {
if (!new)
ast_mutex_lock(&q->lock);
/* Re-initialize the queue, and clear statistics */
init_queue(q);
clear_queue(q);
free_members(q, 0);
prev = q->members;
if (prev) {
/* find the end of any dynamic members */
while(prev->next)
prev = prev->next;
}
var = ast_variable_browse(cfg, cat);
while(var) {
if (!strcasecmp(var->name, "member")) {
/* Add a new member */
ast_copy_string(interface, var->value,
sizeof(interface));
if ((tmp = strchr(interface, ','))) {
*tmp = '\0';
tmp++;
penalty = atoi(tmp);
if (penalty < 0) {
penalty = 0;
}
} else
penalty = 0;
cur = create_queue_member(interface, penalty, 0);
if (cur) {
if (prev)
prev->next = cur;
else
q->members = cur;
prev = cur;
}
} else {
queue_set_param(q, var->name, var->value,
var->lineno, 1);
}
var = var->next;
}
if (!new)
ast_mutex_unlock(&q->lock);
if (new) {
q->next = queues;
queues = q;
}
}
}
cat = ast_category_browse(cfg, cat);
}
ast_config_destroy(cfg);
q = queues;
ql = NULL;
while(q) {
qn = q->next;
if (q->dead) {
if (ql)
ql->next = q->next;
else
queues = q->next;
if (!q->count) {
free(q);
} else
ast_log(LOG_WARNING, "XXX Leaking a little memory :(
XXX\n");
} else {
for (cur = q->members; cur; cur = cur->next)
cur->status = ast_device_state(cur->interface);
ql = q;
}
q = qn;
}
ast_mutex_unlock(&qlock);
}
Thank you,
Matthew Roth
InterMedia Marketing Solutions
Software Engineer and Systems Developer
More information about the asterisk-dev
mailing list