[asterisk-dev] [Code Review] Locking issue in action_bridge and bridge_exec

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/167/#review465
-----------------------------------------------------------



/trunk/main/features.c
<http://reviewboard.digium.com/r/167/#comment1109>

    You're missing an unlock of chana here



/trunk/main/features.c
<http://reviewboard.digium.com/r/167/#comment1110>

    I would set chana = NULL here to be explicit about the fact that code is not allowed to touch it anymore.



/trunk/main/features.c
<http://reviewboard.digium.com/r/167/#comment1111>

    You're missing an unlock of chanb here



/trunk/main/features.c
<http://reviewboard.digium.com/r/167/#comment1112>

    I think you should use ast_hangup(tmpchana);



/trunk/main/features.c
<http://reviewboard.digium.com/r/167/#comment1113>

    Set chanb = NULL


- Russell


On 2009-02-17 17:43:36, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/167/
> -----------------------------------------------------------
> 
> (Updated 2009-02-17 17:43:36)
> 
> 
> Review request for Asterisk Developers and Russell Bryant.
> 
> 
> Summary
> -------
> 
> action_bridge() and bridge_exec() both search for the channels to bridge to, and then immediately drop the lock.  Instead, they should hold the lock until the masquerade is complete.  This will guarantee the channel remains and prevent any other weirdness from occurring.  In action_bridge() some more weirdness comes into play.  Both channels are needlessly locked at the same time and perform the exact same logic.  It makes sense from a coding organizational standpoint, but could cause a theoretical deadlock so I split the code up.  There is an issue associated with this, but since its a rather complicated thing to reproduce I'm not certain this allone will close it.
> 
> 
> This addresses bug 0014296.
>     http://bugs.digium.com/view.php?id=0014296
> 
> 
> Diffs
> -----
> 
>   /trunk/main/features.c 176358 
> 
> Diff: http://reviewboard.digium.com/r/167/diff
> 
> 
> Testing
> -------
> 
> I've executed both functions with no problems. 
> 
> 
> Thanks,
> 
> David
> 
>