[asterisk-dev] [svn-commits] tilghman: branch 1.4 r120001 - /branches/1.4/channels/chan_iax2.c

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: tilghman
> Date: Tue Jun  3 11:10:53 2008
> New Revision: 120001
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=120001
> Log:
> Save the callno when we're poking, because our peer structure could change
> during deadlock avoidance (and thus we unlock the wrong callno, causing a
> cascade failure).
> (closes issue #12717)
>  Reported by: gewfie
>  Patches: 
>        20080525__bug12717.diff.txt uploaded by Corydon76 (license 14)
>  Tested by: gewfie
> 
> Modified:
>     branches/1.4/channels/chan_iax2.c
> 
> Modified: branches/1.4/channels/chan_iax2.c
> URL: http://svn.digium.com/view/asterisk/branches/1.4/channels/chan_iax2.c?view=diff&rev=120001&r1=120000&r2=120001
> ==============================================================================
> --- branches/1.4/channels/chan_iax2.c (original)
> +++ branches/1.4/channels/chan_iax2.c Tue Jun  3 11:10:53 2008
> @@ -8842,6 +8842,7 @@
>  
>  static int iax2_poke_peer(struct iax2_peer *peer, int heldcall)
>  {
> +	int callno;
>  	if (!peer->maxms || (!peer->addr.sin_addr.s_addr && !peer->dnsmgr)) {
>  		/* IF we have no IP without dnsmgr, or this isn't to be monitored, return
>  		  immediately after clearing things out */
> @@ -8851,11 +8852,13 @@
>  		peer->callno = 0;
>  		return 0;
>  	}
> -	if (peer->callno > 0) {
> +
> +	/* The peer could change the callno inside iax2_destroy, since we do deadlock avoidance */
> +	if ((callno = peer->callno) > 0) {
>  		ast_log(LOG_NOTICE, "Still have a callno...\n");
> -		ast_mutex_lock(&iaxsl[peer->callno]);
> -		iax2_destroy(peer->callno);
> -		ast_mutex_unlock(&iaxsl[peer->callno]);
> +		ast_mutex_lock(&iaxsl[callno]);
> +		iax2_destroy(callno);
> +		ast_mutex_unlock(&iaxsl[callno]);
>  	}
>  	if (heldcall)
>  		ast_mutex_unlock(&iaxsl[heldcall]);

What deadlock avoidance are you referring to here?  It looks to me like 
the real problem is that the peer->callno could change at any time, 
because the peer is not locked at this point.  However, with the change 
you have added, it's not needed anymore.  I just wanted to point out 
where the problem came from.

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