[asterisk-dev] [Code Review] Remove potential deadlock in call pickup race.

[Russell Bryant]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1234/#review3640
-----------------------------------------------------------


I think there may be a race condition here.  Can two threads both determine that a channel can be picked up using ast_can_pickup() and then proceed to attempt to attempt a pickup?  I'm not sure if there's a good way to fix it.  Maybe the pickup function just also needs to check for this datastore and bail if it is already there.

I'm also wondering if there could be bad side effects of leaving the datastore on the channel.  It's going to hang around and keep getting moved if transfers and such happen.  Will there _ever_ be a case when it's valid to do a pickup on the same ast_channel twice?  I can't think of one but leaving that data around still doesn't feel right.


/branches/1.8/include/asterisk/features.h
<https://reviewboard.asterisk.org/r/1234/#comment7308>

    I know this is pedantic, but can you be more explicit about the return value and say non-zero instead of true here since the return type is int?


- Russell


On 2011-05-26 18:31:19, rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1234/
> -----------------------------------------------------------
> 
> (Updated 2011-05-26 18:31:19)
> 
> 
> Review request for Asterisk Developers, Alec Davis and irroot.
> 
> 
> Summary
> -------
> 
> Fix potential deadlock in ast_do_pickup() with channels chan and target lock and channel container lock.
> 
> This should eliminate the deadlock avoidance code in https://reviewboard.asterisk.org/r/1118/ for issue 18830.
> 
> * Moved can_pickup() to ast_can_pickup() in features.c.
> 
> 
> This addresses bugs 18654 and 18830.
>     https://issues.asterisk.org/view.php?id=18654
>     https://issues.asterisk.org/view.php?id=18830
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/include/asterisk/features.h 321209 
>   /branches/1.8/main/features.c 321209 
>   /branches/1.8/apps/app_directed_pickup.c 321209 
> 
> Diff: https://reviewboard.asterisk.org/r/1234/diff
> 
> 
> Testing
> -------
> 
> Tested a simple call pickup to make sure that it at least still works.
> Needs competing call pickup testing.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110527/f0f009a2/attachment-0001.htm>