[asterisk-dev] mmichelson: branch 1.4 r131299 - /branches/1.4/apps/app_queue.c

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: mmichelson
> Date: Wed Jul 16 13:57:34 2008
> New Revision: 131299
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=131299
> Log:
> Make absolutely certain that the transfer datastore
> is removed from the calling channel once the caller
> is finished in the queue. This could have weird con-
> sequences when dialing local queue members when multiple
> transfers occur on a single call.
> 
> Also fixed a memory leak that would occur when an
> attended transfer occurred from a queue member.
> 
> (closes issue #13047)
> Reported by: festr

<snip>

> @@ -3109,6 +3110,7 @@
>  		bridge = ast_bridge_call(qe->chan,peer, &bridge_config);
>  
>  		if (!attended_transfer_occurred(qe->chan)) {
> +			struct ast_datastore *transfer_ds = ast_channel_datastore_find(qe->chan, &queue_transfer_info, NULL);
>  			if (strcasecmp(oldcontext, qe->chan->context) || strcasecmp(oldexten, qe->chan->exten)) {
>  				ast_queue_log(queuename, qe->chan->uniqueid, member->membername, "TRANSFER", "%s|%s|%ld|%ld",
>  					qe->chan->exten, qe->chan->context, (long) (callstart - qe->start),

Is the channel locked around this call to ast_channel_datastore_find()? 
  It has to be, or else it is not a thread-safe operation.

> @@ -3146,6 +3148,12 @@
>  							queuename, qe->chan->uniqueid, peer->name, member->membername, (long)(callstart - qe->start),
>  							(long)(time(NULL) - callstart),
>  							qe->parent->eventwhencalled == QUEUE_EVENT_VARIABLES ? vars2manager(qe->chan, vars, sizeof(vars)) : "");
> +			}
> +			if (transfer_ds) {
> +				ast_channel_lock(qe->chan);
> +				ast_channel_datastore_remove(qe->chan, transfer_ds);
> +				ast_channel_datastore_free(transfer_ds);
> +				ast_channel_unlock(qe->chan);
>  			}
>  		}

This is a bit tricky.  Is there any possibility that the datastore may 
not be on the channel anymore at this point?  It is theoretically 
possible since the channel is not locked, but knowing whether it is 
actually possible would take more analysis.  (Of course, if it is 
possible that it's not on the channel anymore, that would also mean that 
it's possible for the pointer you hold to the datastore to become 
invalid out from under you ...)

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