[asterisk-dev] mmichelson: trunk r125649 - /trunk/apps/app_queue.c

[Russell Bryant]

On Jun 26, 2008, at 8:15 PM, SVN commits to the Digium repositories  
wrote:

> Author: mmichelson
> Date: Thu Jun 26 19:15:54 2008
> New Revision: 125649
>
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=125649
> Log:
> The monitor-join option for queues was deprecated in favor of using
> MixMonitor to mix audio. However, it was pointed out to me that  
> because
> of this, the command set for the MONITOR_EXEC variable is ignored as  
> well.
> This means that people can't do their own custom mixing commands at  
> the end
> of recordings in order to make, for instance, stereo recordings of  
> calls.
>
> With this patch, app_queue will set the "joinfiles" variable for the  
> channel's
> monitor if MONITOR_EXEC is not zero-length. This means that for  
> normal audio
> mixing, MixMonitor is still the preferred choice, but we allow custom
> mixing to be done with the two Monitor streams if desired.
>
> (closes issue #12923)
> Reported by: snyfer
>
>
> Modified:
>    trunk/apps/app_queue.c
>
> Modified: trunk/apps/app_queue.c
> URL: http://svn.digium.com/view/asterisk/trunk/apps/app_queue.c?view=diff&rev=125649&r1=125648&r2=125649
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- trunk/apps/app_queue.c (original)
> +++ trunk/apps/app_queue.c Thu Jun 26 19:15:54 2008
> @@ -3539,8 +3539,9 @@
> 		/* Begin Monitoring */
> 		if (qe->parent->monfmt && *qe->parent->monfmt) {
> 			if (!qe->parent->montype) {
> +				char *monexec, *monargs;
> 				ast_debug(1, "Starting Monitor as requested.\n");
> -				if (pbx_builtin_getvar_helper(qe->chan, "MONITOR_EXEC") ||  
> pbx_builtin_getvar_helper(qe->chan, "MONITOR_EXEC_ARGS"))
> +				if ((monexec = pbx_builtin_getvar_helper(qe->chan,  
> "MONITOR_EXEC")) || (monargs = pbx_builtin_getvar_helper(qe->chan,  
> "MONITOR_EXEC_ARGS")))
> 					which = qe->chan;
> 				else
> 					which = peer;
> @@ -3552,6 +3553,9 @@
> 					/* Last ditch effort -- no CDR, make up something */
> 					snprintf(tmpid, sizeof(tmpid), "chan-%lx", ast_random());
> 					ast_monitor_start(which, qe->parent->monfmt, tmpid, 1, X_REC_IN  
> | X_REC_OUT);
> +				}
> +				if (!ast_strlen_zero(monexec)) {
> +					ast_monitor_setjoinfiles(which, 1);
> 				}
> 			} else {
> 				mixmonapp = pbx_findapp("MixMonitor");


Is the channel locked around this whole section of code?  If not, then  
the usage of getvar_helper() here is not safe.  You have to copy the  
result off to another buffer (usually just ast_strdupa) with the  
channel locked.  Without the channel locked, a result from  
getvar_helper() can become invalid at any point.

A lot of this has been fixed in existing code as a janitor project,  
but I'm sure it still exists in a number of places.  However, we  
should be really careful to avoiding introducing more code that has  
the problem.

--
Russell Bryant
Senior Software Engineer
Open Source Team Lead
Digium, Inc.