[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:52 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.
====================================================================== 

---------------------------------------------------------------------- 
 jpgrayson - 07-07-08 16:52  
---------------------------------------------------------------------- 
I've uploaded a second version of the patch where we unlock/relock around
the ast_channel_free() call. Russel's explanation makes sense.

As for (1), it makes sense that the (i != iaxs[callno]) part of the test
can no longer be true. We originally fixed this problem against 1.4.10. I
believe that version did not have the 60 second guarantee.

To be pedantic, I left the test unchanged from the original patch anyway.
I wouldn't object to someone changing that piece of the patch though. 

Issue History 
Date Modified   Username       Field                    Change               
====================================================================== 
07-07-08 16:52  jpgrayson      Note Added: 0089864                          
======================================================================