From: Richard Mudgett Date: Thu, 31 May 2012 18:20:15 +0000 (+0000) Subject: Coverity Report: Fix issues for error type REVERSE_INULL (core modules) X-Git-Tag: 10.6.0-rc1~18 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a9c6f566abe968be114852b3c28389abe768d792;p=thirdparty%2Fasterisk.git Coverity Report: Fix issues for error type REVERSE_INULL (core modules) * Fixes findings: 0-2,5,7-15,24-26,28-31 (issue ASTERISK-19648) Reported by: Matt Jordan ........ Merged revisions 368039 from http://svn.asterisk.org/svn/asterisk/branches/1.8 git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/10@368042 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- diff --git a/apps/app_queue.c b/apps/app_queue.c index 21dd4a7f42..910c20a6f8 100644 --- a/apps/app_queue.c +++ b/apps/app_queue.c @@ -2539,11 +2539,11 @@ static int join_queue(char *queuename, struct queue_ent *qe, enum queue_result * */ if (!inserted && (qe->prio >= cur->prio) && position && (position <= pos + 1)) { insert_entry(q, prev, qe, &pos); + inserted = 1; /*pos is incremented inside insert_entry, so don't need to add 1 here*/ if (position < pos) { ast_log(LOG_NOTICE, "Asked to be inserted at position %d but forced into position %d due to higher priority callers\n", position, pos); } - inserted = 1; } cur->pos = ++pos; prev = cur; @@ -6132,6 +6132,8 @@ static int queue_exec(struct ast_channel *chan, const char *data) set_queue_result(chan, reason); return 0; } + ast_assert(qe.parent != NULL); + ast_queue_log(args.queuename, chan->uniqueid, "NONE", "ENTERQUEUE", "%s|%s|%d", S_OR(args.url, ""), S_COR(chan->caller.id.number.valid, chan->caller.id.number.str, ""), @@ -6286,12 +6288,13 @@ stop: if (reason != QUEUE_UNKNOWN) set_queue_result(chan, reason); - if (qe.parent) { - /* every queue_ent is given a reference to it's parent call_queue when it joins the queue. - * This ref must be taken away right before the queue_ent is destroyed. In this case - * the queue_ent is about to be returned on the stack */ - qe.parent = queue_unref(qe.parent); - } + /* + * every queue_ent is given a reference to it's parent + * call_queue when it joins the queue. This ref must be taken + * away right before the queue_ent is destroyed. In this case + * the queue_ent is about to be returned on the stack + */ + qe.parent = queue_unref(qe.parent); return res; } diff --git a/channels/chan_agent.c b/channels/chan_agent.c index 1cd1f46bad..49ed7f9818 100644 --- a/channels/chan_agent.c +++ b/channels/chan_agent.c @@ -516,21 +516,27 @@ static struct agent_pvt *add_agent(const char *agent, int pending) /*! * Deletes an agent after doing some clean up. * Further documentation: How safe is this function ? What state should the agent be to be cleaned. + * + * \warning XXX This function seems to be very unsafe. + * Potential for double free and use after free among other + * problems. + * * \param p Agent to be deleted. * \returns Always 0. */ static int agent_cleanup(struct agent_pvt *p) { - struct ast_channel *chan = NULL; + struct ast_channel *chan; + ast_mutex_lock(&p->lock); chan = p->owner; p->owner = NULL; - chan->tech_pvt = NULL; /* Release ownership of the agent to other threads (presumably running the login app). */ p->app_sleep_cond = 1; p->app_lock_flag = 0; ast_cond_signal(&p->app_complete_cond); if (chan) { + chan->tech_pvt = NULL; chan = ast_channel_release(chan); } if (p->dead) { @@ -539,7 +545,9 @@ static int agent_cleanup(struct agent_pvt *p) ast_cond_destroy(&p->app_complete_cond); ast_cond_destroy(&p->login_wait_cond); ast_free(p); - } + } else { + ast_mutex_unlock(&p->lock); + } return 0; } diff --git a/channels/chan_iax2.c b/channels/chan_iax2.c index 05d7a010fe..36369e58c2 100644 --- a/channels/chan_iax2.c +++ b/channels/chan_iax2.c @@ -2484,19 +2484,20 @@ static void peercnt_remove(struct peercnt *peercnt) .sin_addr.s_addr = peercnt->addr, }; - if (peercnt) { - /* Container locked here since peercnt may be unlinked from list. If left unlocked, - * peercnt_add could try and grab this entry from the table and modify it at the - * "same time" this thread attemps to unlink it.*/ - ao2_lock(peercnts); - peercnt->cur--; - ast_debug(1, "ip callno count decremented to %d for %s\n", peercnt->cur, ast_inet_ntoa(sin.sin_addr)); - /* if this was the last connection from the peer remove it from table */ - if (peercnt->cur == 0) { - ao2_unlink(peercnts, peercnt);/* decrements ref from table, last ref is left to scheduler */ - } - ao2_unlock(peercnts); + /* + * Container locked here since peercnt may be unlinked from + * list. If left unlocked, peercnt_add could try and grab this + * entry from the table and modify it at the "same time" this + * thread attemps to unlink it. + */ + ao2_lock(peercnts); + peercnt->cur--; + ast_debug(1, "ip callno count decremented to %d for %s\n", peercnt->cur, ast_inet_ntoa(sin.sin_addr)); + /* if this was the last connection from the peer remove it from table */ + if (peercnt->cur == 0) { + ao2_unlink(peercnts, peercnt);/* decrements ref from table, last ref is left to scheduler */ } + ao2_unlock(peercnts); } /*! @@ -6016,16 +6017,15 @@ static unsigned int calc_timestamp(struct chan_iax2_pvt *p, unsigned int ts, str The "genuine" distinction is needed because genuine frames must get a clock-based timestamp, the others need a timestamp slaved to the voice frames so that they go in sequence */ - if (f) { - if (f->frametype == AST_FRAME_VOICE) { - voice = 1; - delivery = &f->delivery; - } else if (f->frametype == AST_FRAME_IAX) { - genuine = 1; - } else if (f->frametype == AST_FRAME_CNG) { - p->notsilenttx = 0; - } + if (f->frametype == AST_FRAME_VOICE) { + voice = 1; + delivery = &f->delivery; + } else if (f->frametype == AST_FRAME_IAX) { + genuine = 1; + } else if (f->frametype == AST_FRAME_CNG) { + p->notsilenttx = 0; } + if (ast_tvzero(p->offset)) { p->offset = ast_tvnow(); /* Round to nearest 20ms for nice looking traces */ diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 16c7da0af2..f54bb35449 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -6426,31 +6426,27 @@ static int sip_hangup(struct ast_channel *ast) ast_channel_unlock(bridge); } - if (p->do_history || oldowner) { - if (p->rtp && (quality = ast_rtp_instance_get_quality(p->rtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) { - if (p->do_history) { - append_history(p, "RTCPaudio", "Quality:%s", quality); - } - if (oldowner) { - pbx_builtin_setvar_helper(oldowner, "RTPAUDIOQOS", quality); - } + /* + * The channel variables are set below just to get the AMI + * VarSet event because the channel is being hungup. + */ + if (p->rtp && (quality = ast_rtp_instance_get_quality(p->rtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) { + if (p->do_history) { + append_history(p, "RTCPaudio", "Quality:%s", quality); } - if (p->vrtp && (quality = ast_rtp_instance_get_quality(p->vrtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) { - if (p->do_history) { - append_history(p, "RTCPvideo", "Quality:%s", quality); - } - if (oldowner) { - pbx_builtin_setvar_helper(oldowner, "RTPVIDEOQOS", quality); - } + pbx_builtin_setvar_helper(oldowner, "RTPAUDIOQOS", quality); + } + if (p->vrtp && (quality = ast_rtp_instance_get_quality(p->vrtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) { + if (p->do_history) { + append_history(p, "RTCPvideo", "Quality:%s", quality); } - if (p->trtp && (quality = ast_rtp_instance_get_quality(p->trtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) { - if (p->do_history) { - append_history(p, "RTCPtext", "Quality:%s", quality); - } - if (oldowner) { - pbx_builtin_setvar_helper(oldowner, "RTPTEXTQOS", quality); - } + pbx_builtin_setvar_helper(oldowner, "RTPVIDEOQOS", quality); + } + if (p->trtp && (quality = ast_rtp_instance_get_quality(p->trtp, AST_RTP_INSTANCE_STAT_FIELD_QUALITY, quality_buf, sizeof(quality_buf)))) { + if (p->do_history) { + append_history(p, "RTCPtext", "Quality:%s", quality); } + pbx_builtin_setvar_helper(oldowner, "RTPTEXTQOS", quality); } /* Send a hangup */ @@ -25714,9 +25710,10 @@ static int handle_request_subscribe(struct sip_pvt *p, struct sip_request *req, } } - if (!req->ignore && p) + if (!req->ignore) { p->lastinvite = seqno; - if (p && !p->needdestroy) { + } + if (!p->needdestroy) { p->expiry = atoi(sip_get_header(req, "Expires")); /* check if the requested expiry-time is within the approved limits from sip.conf */ diff --git a/funcs/func_math.c b/funcs/func_math.c index e7217c7b3d..e745c4733f 100644 --- a/funcs/func_math.c +++ b/funcs/func_math.c @@ -57,7 +57,7 @@ ASTERISK_FILE_VERSION(__FILE__, "$Revision$") number1opnumber2 where the possible values for op are: - +,-,/,*,%,<<,>>,^,AND,OR,XOR,<,%gt;,>=,<=,== (and behave as their C equivalents) + +,-,/,*,%,<<,>>,^,AND,OR,XOR,<,>,<=,>=,== (and behave as their C equivalents) Wanted type of result: @@ -254,7 +254,7 @@ static int math(struct ast_channel *chan, const char *cmd, char *parse, } } - if (!mvalue1 || !mvalue2) { + if (!mvalue2) { ast_log(LOG_WARNING, "Supply all the parameters - just this once, please\n"); return -1; diff --git a/main/features.c b/main/features.c index aa62b5b3af..f0e4b1a416 100644 --- a/main/features.c +++ b/main/features.c @@ -2076,10 +2076,7 @@ static int builtin_automonitor(struct ast_channel *chan, struct ast_channel *pee } set_peers(&caller_chan, &callee_chan, peer, chan, sense); - if (!caller_chan || !callee_chan) { - ast_log(LOG_NOTICE,"Cannot record the call. One or both channels have gone away.\n"); - return -1; - } + /* Find extra messages */ automon_message_start = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MONITOR_MESSAGE_START"); automon_message_stop = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MONITOR_MESSAGE_STOP"); @@ -2155,6 +2152,8 @@ static int builtin_automixmonitor(struct ast_channel *chan, struct ast_channel * size_t len; struct ast_channel *caller_chan, *callee_chan; const char *mixmonitor_spy_type = "MixMonitor"; + const char *touch_format; + const char *touch_monitor; int count = 0; if (!mixmonitor_ok) { @@ -2189,7 +2188,6 @@ static int builtin_automixmonitor(struct ast_channel *chan, struct ast_channel * /* This means a mixmonitor is attached to the channel, running or not is unknown. */ if (count > 0) { - ast_verb(3, "User hit '%s' to stop recording call.\n", code); /* Make sure they are running */ @@ -2214,51 +2212,44 @@ static int builtin_automixmonitor(struct ast_channel *chan, struct ast_channel * ast_log(LOG_WARNING,"Stopped MixMonitors are attached to the channel.\n"); } - if (caller_chan && callee_chan) { - const char *touch_format = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MIXMONITOR_FORMAT"); - const char *touch_monitor = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MIXMONITOR"); - - if (!touch_format) - touch_format = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MIXMONITOR_FORMAT"); + touch_format = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MIXMONITOR_FORMAT"); + touch_monitor = pbx_builtin_getvar_helper(caller_chan, "TOUCH_MIXMONITOR"); - if (!touch_monitor) - touch_monitor = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MIXMONITOR"); - - if (touch_monitor) { - len = strlen(touch_monitor) + 50; - args = alloca(len); - touch_filename = alloca(len); - snprintf(touch_filename, len, "auto-%ld-%s", (long)time(NULL), touch_monitor); - snprintf(args, len, "%s.%s,b", touch_filename, (touch_format) ? touch_format : "wav"); - } else { - caller_chan_id = ast_strdupa(S_COR(caller_chan->caller.id.number.valid, - caller_chan->caller.id.number.str, caller_chan->name)); - callee_chan_id = ast_strdupa(S_COR(callee_chan->caller.id.number.valid, - callee_chan->caller.id.number.str, callee_chan->name)); - len = strlen(caller_chan_id) + strlen(callee_chan_id) + 50; - args = alloca(len); - touch_filename = alloca(len); - snprintf(touch_filename, len, "auto-%ld-%s-%s", (long)time(NULL), caller_chan_id, callee_chan_id); - snprintf(args, len, "%s.%s,b", touch_filename, S_OR(touch_format, "wav")); - } + if (!touch_format) + touch_format = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MIXMONITOR_FORMAT"); - for( x = 0; x < strlen(args); x++) { - if (args[x] == '/') - args[x] = '-'; - } + if (!touch_monitor) + touch_monitor = pbx_builtin_getvar_helper(callee_chan, "TOUCH_MIXMONITOR"); - ast_verb(3, "User hit '%s' to record call. filename: %s\n", code, touch_filename); + if (touch_monitor) { + len = strlen(touch_monitor) + 50; + args = alloca(len); + touch_filename = alloca(len); + snprintf(touch_filename, len, "auto-%ld-%s", (long)time(NULL), touch_monitor); + snprintf(args, len, "%s.%s,b", touch_filename, (touch_format) ? touch_format : "wav"); + } else { + caller_chan_id = ast_strdupa(S_COR(caller_chan->caller.id.number.valid, + caller_chan->caller.id.number.str, caller_chan->name)); + callee_chan_id = ast_strdupa(S_COR(callee_chan->caller.id.number.valid, + callee_chan->caller.id.number.str, callee_chan->name)); + len = strlen(caller_chan_id) + strlen(callee_chan_id) + 50; + args = alloca(len); + touch_filename = alloca(len); + snprintf(touch_filename, len, "auto-%ld-%s-%s", (long)time(NULL), caller_chan_id, callee_chan_id); + snprintf(args, len, "%s.%s,b", touch_filename, S_OR(touch_format, "wav")); + } - pbx_exec(callee_chan, mixmonitor_app, args); - pbx_builtin_setvar_helper(callee_chan, "TOUCH_MIXMONITOR_OUTPUT", touch_filename); - pbx_builtin_setvar_helper(caller_chan, "TOUCH_MIXMONITOR_OUTPUT", touch_filename); - return AST_FEATURE_RETURN_SUCCESS; - + for( x = 0; x < strlen(args); x++) { + if (args[x] == '/') + args[x] = '-'; } - ast_log(LOG_NOTICE,"Cannot record the call. One or both channels have gone away.\n"); - return -1; + ast_verb(3, "User hit '%s' to record call. filename: %s\n", code, touch_filename); + pbx_exec(callee_chan, mixmonitor_app, args); + pbx_builtin_setvar_helper(callee_chan, "TOUCH_MIXMONITOR_OUTPUT", touch_filename); + pbx_builtin_setvar_helper(caller_chan, "TOUCH_MIXMONITOR_OUTPUT", touch_filename); + return AST_FEATURE_RETURN_SUCCESS; } static int builtin_disconnect(struct ast_channel *chan, struct ast_channel *peer, struct ast_bridge_config *config, const char *code, int sense, void *data) @@ -2480,6 +2471,8 @@ static int builtin_atxfer(struct ast_channel *chan, struct ast_channel *peer, st struct ast_channel *transferer;/* Party B */ struct ast_channel *transferee;/* Party A */ struct ast_exten *park_exten; + const char *chan1_attended_sound; + const char *chan2_attended_sound; const char *transferer_real_context; char xferto[256] = ""; int res; @@ -2552,16 +2545,13 @@ static int builtin_atxfer(struct ast_channel *chan, struct ast_channel *peer, st /* If we are performing an attended transfer and we have two channels involved then copy sound file information to play upon attended transfer completion */ - if (transferee) { - const char *chan1_attended_sound = pbx_builtin_getvar_helper(transferer, "ATTENDED_TRANSFER_COMPLETE_SOUND"); - const char *chan2_attended_sound = pbx_builtin_getvar_helper(transferee, "ATTENDED_TRANSFER_COMPLETE_SOUND"); - - if (!ast_strlen_zero(chan1_attended_sound)) { - pbx_builtin_setvar_helper(transferer, "BRIDGE_PLAY_SOUND", chan1_attended_sound); - } - if (!ast_strlen_zero(chan2_attended_sound)) { - pbx_builtin_setvar_helper(transferee, "BRIDGE_PLAY_SOUND", chan2_attended_sound); - } + chan1_attended_sound = pbx_builtin_getvar_helper(transferer, "ATTENDED_TRANSFER_COMPLETE_SOUND"); + chan2_attended_sound = pbx_builtin_getvar_helper(transferee, "ATTENDED_TRANSFER_COMPLETE_SOUND"); + if (!ast_strlen_zero(chan1_attended_sound)) { + pbx_builtin_setvar_helper(transferer, "BRIDGE_PLAY_SOUND", chan1_attended_sound); + } + if (!ast_strlen_zero(chan2_attended_sound)) { + pbx_builtin_setvar_helper(transferee, "BRIDGE_PLAY_SOUND", chan2_attended_sound); } /* Extract redial transferer information from the channel name. */ diff --git a/main/manager.c b/main/manager.c index f4540f528c..8556275ae8 100644 --- a/main/manager.c +++ b/main/manager.c @@ -6263,7 +6263,7 @@ static int auth_http_callback(struct ast_tcptls_session_instance *ser, ast_md5_hash(resp_hash, resp); } - if (!d.nonce || strncasecmp(d.response, resp_hash, strlen(resp_hash))) { + if (strncasecmp(d.response, resp_hash, strlen(resp_hash))) { /* Something was wrong, so give the client to try with a new challenge */ AST_RWLIST_UNLOCK(&users); nonce = 0; diff --git a/main/tcptls.c b/main/tcptls.c index 3601373aa2..6d725fe0fd 100644 --- a/main/tcptls.c +++ b/main/tcptls.c @@ -245,10 +245,11 @@ static void *handle_tcptls_connection(void *data) return NULL; } - if (tcptls_session && tcptls_session->parent->worker_fn) + if (tcptls_session->parent->worker_fn) { return tcptls_session->parent->worker_fn(tcptls_session); - else + } else { return tcptls_session; + } } void *ast_tcptls_server_root(void *data) @@ -443,9 +444,7 @@ client_start_error: close(desc->accept_fd); desc->accept_fd = -1; } - if (tcptls_session) { - ao2_ref(tcptls_session, -1); - } + ao2_ref(tcptls_session, -1); return NULL; } diff --git a/pbx/pbx_config.c b/pbx/pbx_config.c index d427447da7..a54d586781 100644 --- a/pbx/pbx_config.c +++ b/pbx/pbx_config.c @@ -1370,8 +1370,13 @@ static int unload_module(void) static char *pbx_strsep(char **destructible, const char *delim) { int square = 0; - char *res = *destructible; - for (; destructible && *destructible && **destructible; (*destructible)++) { + char *res; + + if (!destructible || !*destructible) { + return NULL; + } + res = *destructible; + for (; **destructible; (*destructible)++) { if (**destructible == '[' && !strchr(delim, '[')) { square++; } else if (**destructible == ']' && !strchr(delim, ']')) { @@ -1386,7 +1391,7 @@ static char *pbx_strsep(char **destructible, const char *delim) break; } } - if (destructible && *destructible && **destructible == '\0') { + if (**destructible == '\0') { *destructible = NULL; } return res; @@ -1596,7 +1601,7 @@ process_extension: v->lineno, vfile); } } - free(tc); + ast_free(tc); } else if (!strcasecmp(v->name, "include")) { pbx_substitute_variables_helper(NULL, v->value, realvalue, sizeof(realvalue) - 1); if (ast_context_add_include2(con, realvalue, registrar)) { diff --git a/res/ael/pval.c b/res/ael/pval.c index 9c557df2bf..327dbffc7c 100644 --- a/res/ael/pval.c +++ b/res/ael/pval.c @@ -1223,21 +1223,24 @@ static pval *get_goto_target(pval *item) return x; } } - return 0; + return NULL; } static void check_goto(pval *item) { + if (!item->u1.list) { + return; + } + /* check for the target of the goto-- does it exist? */ if ( !(item->u1.list)->next && !(item->u1.list)->u1.str ) { ast_log(LOG_ERROR,"Error: file %s, line %d-%d: goto: empty label reference found!\n", item->filename, item->startline, item->endline); errs++; } - + /* just one item-- the label should be in the current extension */ - - if (item->u1.list && !item->u1.list->next && !strstr((item->u1.list)->u1.str,"${")) { + if (!item->u1.list->next && !strstr(item->u1.list->u1.str,"${")) { struct pval *z = get_extension_or_contxt(item); struct pval *x = 0; if (z) diff --git a/res/res_config_odbc.c b/res/res_config_odbc.c index 30dfc5cdff..46922835fd 100644 --- a/res/res_config_odbc.c +++ b/res/res_config_odbc.c @@ -317,7 +317,7 @@ static struct ast_config *realtime_multi_odbc(const char *database, const char * char sql[1024]; char coltitle[256]; char rowdata[2048]; - const char *initfield=NULL; + const char *initfield; char *op; const char *newparam; char *stringp; @@ -375,9 +375,7 @@ static struct ast_config *realtime_multi_odbc(const char *database, const char * } va_end(aq); - if (initfield) { - snprintf(sql + strlen(sql), sizeof(sql) - strlen(sql), " ORDER BY %s", initfield); - } + snprintf(sql + strlen(sql), sizeof(sql) - strlen(sql), " ORDER BY %s", initfield); va_copy(cps.ap, ap); stmt = ast_odbc_prepare_and_execute(obj, custom_prepare, &cps); @@ -447,7 +445,7 @@ static struct ast_config *realtime_multi_odbc(const char *database, const char * if (strchr(chunk, '^')) { decode_chunk(chunk); } - if (initfield && !strcmp(initfield, coltitle)) { + if (!strcmp(initfield, coltitle)) { ast_category_rename(cat, chunk); } var = ast_variable_new(coltitle, chunk, "");