[asterisk-dev] murf: branch 1.4 r150637 - /branches/1.4/res/res_features.c

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: murf
> Date: Fri Oct 17 12:18:31 2008
> New Revision: 150637
> 
> URL: http://svn.digium.com/view/asterisk?view=rev&rev=150637
> Log:
> 
> Interesting crash. In this case, you exit the
> bridge with peer completely GONE. 
> 
> I moved the channel find call up to cover the
> whole peer CDR reset code segment. This appears
> to solve the crash without changing the logic
> at all.
> 
> Modified:
>     branches/1.4/res/res_features.c
> 
> Modified: branches/1.4/res/res_features.c
> URL: http://svn.digium.com/view/asterisk/branches/1.4/res/res_features.c?view=diff&rev=150637&r1=150636&r2=150637
> ==============================================================================
> --- branches/1.4/res/res_features.c (original)
> +++ branches/1.4/res/res_features.c Fri Oct 17 12:18:31 2008
> @@ -1812,9 +1812,9 @@
>  		} else {
>  			ast_cdr_specialized_reset(chan_cdr,0); /* nothing changed, reset the chan_cdr  */
>  		}
> -		if (strcasecmp(orig_peername, peer->name) != 0) { 
> +		chan_ptr = ast_get_channel_by_name_locked(orig_peername);
> +		if (chan_ptr && strcasecmp(orig_peername, peer->name) != 0) { 
>  			/* old channel */
> -			chan_ptr = ast_get_channel_by_name_locked(orig_peername);
>  			if (chan_ptr) {
>  				if (!ast_bridged_channel(chan_ptr)) {
>  					struct ast_cdr *cur;
> @@ -1826,13 +1826,15 @@
>  					if (cur)
>  						ast_cdr_specialized_reset(peer_cdr,0);
>  				}
> -				ast_channel_unlock(chan_ptr);
>  			}
>  			/* new channel */
>  			ast_cdr_specialized_reset(new_peer_cdr,0);
>  		} else {
> -			ast_cdr_specialized_reset(peer_cdr,0); /* nothing changed, reset the peer_cdr  */
> -		}
> +			if (chan_ptr)
> +				ast_cdr_specialized_reset(peer_cdr,0); /* nothing changed, reset the peer_cdr  */
> +		}
> +		if (chan_ptr)
> +			ast_channel_unlock(chan_ptr);
>  	}
>  	return res;
>  }

I'm concerned about the code you're changing here.

There should be absolutely no case in which either of the channel 
pointers passed to this function become invalid.  If this is happening, 
there is a bug elsewhere.  Also, given this condition, there should be 
no reason that you need to do this get_channel_by_name stuff here.  In 
fact, this is an _extremely_ expensive operation to be doing on every 
bridge.  It will hurt performance on loaded systems.

So, we're going to need some rework here ...

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