[asterisk-dev] race condition on bridge/masquerading?

[Russell Bryant]

On Mar 7, 2009, at 6:11 PM, Guillermo Winkler wrote:

>                               /* See if the BRIDGEPEER variable  
> needs to be updated */
>                               if (! 
> ast_strlen_zero(pbx_builtin_getvar_helper(c0, "BRIDGEPEER")))
>                                                
> pbx_builtin_setvar_helper(c0, "BRIDGEPEER", c1->name);
>                               if (! 
> ast_strlen_zero(pbx_builtin_getvar_helper(c1, "BRIDGEPEER")))
>                                                
> pbx_builtin_setvar_helper(c1, "BRIDGEPEER", c0->name);
>
> It’s not done inside a lock, as it’s done inside  
> ast_do_masquerade(struct ast_channel *original) when the clone  
> masquerading member gets destroyed.
>
> In all the crashes I’ve seen with gdb it’s always freeing the  
> BRIDGEPEER variable with a value of local,1 <zombie>, so it was  
> caught in the middle of the masquerading process.
>
> I think access to channel->name should be done inside a lock that  
> also checks for zombieness to be sure we’re always touching valid  
> memory.(or better yet, to check the bridge is going through   
> masquerading and stop touching channel members completely)

You are correct.  It is not safe to read ast_channel->name without the  
channel locked.  This is a bug.  Nice work with your analysis.

If you open an issue on bugs.digium.com, we can ensure that this gets  
fixed.

--
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org