[asterisk-bugs] [Asterisk 0013014]: [patch] ast_iax2_new() fails to account for a bad situation, deals with another incorrectly

[noreply at bugs.digium.com]

A NOTE has been added to this issue. 
====================================================================== 
http://bugs.digium.com/view.php?id=13014 
====================================================================== 
Reported By:                jpgrayson
Assigned To:                russell
====================================================================== 
Project:                    Asterisk
Issue ID:                   13014
Category:                   Channels/chan_iax2
Reproducibility:            sometimes
Severity:                   major
Priority:                   normal
Status:                     assigned
Asterisk Version:           SVN 
SVN Branch (only for SVN checkouts, not tarball releases):  1.4  
SVN Revision (number only!):  
Disclaimer on File?:        N/A 
Request Review:              
====================================================================== 
Date Submitted:             07-07-2008 15:05 CDT
Last Modified:              07-07-2008 16:14 CDT
====================================================================== 
Summary:                    [patch] ast_iax2_new() fails to account for a bad
situation, deals with another incorrectly
Description: 
ast_iax2_new() does a tricky dance when allocating an ast_channel. There
are two related problems in ast_iax2_new() regarding this channel
allocation.

For reference, here is the offending code:

	ast_mutex_unlock(&iaxsl[callno]);
	tmp = ast_channel_alloc(1, state, ...);
	ast_mutex_lock(&iaxsl[callno]);
	if (!iaxs[callno]) {
		if (tmp) {
			ast_channel_free(tmp);
		}
		ast_mutex_unlock(&iaxsl[callno]);
		return NULL;
	}

The first and most obvious problem in the current implementation is that
there is a case we will unlock callno's mutex prior to returning NULL. This
breaks the protocol for ast_iax2_new() -- users of this function expect
iaxsl[callno] to _always_ be returned locked. When this failure case is
hit, callers of ast_iax2_new() will try to unlock an already unlocked
mutex. Whoops.

The second problem is that this test is incomplete. This code only tests
to see if the chan_iax2_pvt object for callno has become null. A better
test is to check if iaxs[callno] has changed (e.g. i != iaxs[callno]).
There are real-world cases where asterisk is being bombarded with new calls
that this callno may not only be destroyed, but destroyed and reused while
ast_channel_alloc() is being called.
====================================================================== 

---------------------------------------------------------------------- 
 russell - 07-07-08 16:14  
---------------------------------------------------------------------- 
Nice catch.  A couple of comments before I commit the patch ...

1) With regards to the check for non-NULL, I don't think it's really
possible for the pvt != iaxs[callno] case to occur.  There is code in the
channel driver to not reuse a call number for 60 seconds.  I do agree that
it's an assumption that probably shouldn't be made all over the place, but
that is why it doesn't actually cause a problem.

2) The reason that the pvt is unlocked before calling ast_channel_free()
has to do with locking order.  You must lock the ast_channel before the
pvt.  That code path would have the channel locked after the pvt, and can
cause a deadlock.  So, that change is necessary. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
07-07-08 16:14  russell        Note Added: 0089863                          
======================================================================