[asterisk-dev] [asterisk-commits] dvossel: trunk r256319 - /trunk/channels/chan_sip.c

[Russell Bryant]

I missed the code review on this one, but I do have some comments ...


----- "SVN commits to the Asterisk project" <asterisk-commits at lists.digium.com> wrote:

> Author: dvossel
> Date: Tue Apr  6 09:42:10 2010
> New Revision: 256319
> 
> URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=256319
> Log:
> fixes deadlock in chan_sip caused by usage of MASTER_CHANNEL dialplan
> function
> 
> (closes issue #16767)
> Reported by: lmsteffan
> Patches:
>       deadlock_16767v3.diff uploaded by dvossel (license 671)
> 
> 
> Modified:
>     trunk/channels/chan_sip.c
> 
> Modified: trunk/channels/chan_sip.c
> URL:
> http://svnview.digium.com/svn/asterisk/trunk/channels/chan_sip.c?view=diff&rev=256319&r1=256318&r2=256319
> ==============================================================================
> --- trunk/channels/chan_sip.c (original)
> +++ trunk/channels/chan_sip.c Tue Apr  6 09:42:10 2010
> @@ -17495,7 +17495,6 @@
>  
>  	owner = p->owner;
>  	if (owner) {
> -		char causevar[256], causeval[256];
>  		const char *rp = NULL, *rh = NULL;
>  
>  		owner->hangupcause = 0;
> @@ -17513,10 +17512,6 @@
>  
>  		if (!owner->hangupcause)
>  			owner->hangupcause = hangup_sip2cause(resp);
> -
> -		snprintf(causevar, sizeof(causevar),
> "MASTER_CHANNEL(HASH(SIP_CAUSE,%s))", owner->name);
> -		snprintf(causeval, sizeof(causeval), "SIP %s",
> REQ_OFFSET_TO_STR(req, rlPart2));
> -		pbx_builtin_setvar_helper(owner, causevar, causeval);
>  	}
>  
>  	if (p->socket.type == SIP_TRANSPORT_UDP) {
> @@ -20922,10 +20917,26 @@
>  				ast_log(LOG_DEBUG, "Ignoring out of order response %d (expecting
> %d)\n", seqno, p->ocseq);
>  			return -1;
>  		} else {
> +			char causevar[256], causeval[256];
> +
>  			if ((respid == 200) || ((respid >= 300) && (respid <= 399))) {
>  				extract_uri(p, req);
>  			}
> +
>  			handle_response(p, respid, e + len, req, seqno);
> +
> +			if (p->owner) {
> +				struct ast_channel *owner = p->owner;
> +
> +				snprintf(causevar, sizeof(causevar),
> "MASTER_CHANNEL(HASH(SIP_CAUSE,%s))", owner->name);
> +				snprintf(causeval, sizeof(causeval), "SIP %s",
> REQ_OFFSET_TO_STR(req, rlPart2));
> +
> +				sip_pvt_unlock(p);
> +				ast_channel_unlock(owner);
> +				*nounlock = 1;
> +				pbx_builtin_setvar_helper(owner, causevar, causeval);
> +				sip_pvt_lock(p);
> +			}
>  		}
>  		return 0;
>  	}

What guarantees that owner is a valid channel in this code?  Based on my reading, there is nothing that guarantees that the channel will not get freed out from under this code.  A reference needs to be taken on p->owner before the unlocks to resolve this.

--
Russell Bryant
Digium, Inc.  |  Engineering Manager, Open Source Software
445 Jan Davis Drive NW   -    Huntsville, AL 35806  -  USA
jabber: rbryant at digium.com    -=-    skype: russell-bryant
www.digium.com -=- www.asterisk.org -=- blogs.asterisk.org