[asterisk-dev] [Code Review] 3067: channels: Return channel locked when allocating.

[rmudgett]

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



/branches/12/channels/chan_pjsip.c
<https://reviewboard.asterisk.org/r/3067/#comment19893>

    Please look at what was done in chan_pjsip_new() for the snapshot locking patch.  There is a deadlock potential with these ao2_finds while holding the channel lock.
    
    Also there is another change at the end of this function that was done to not hold the lock too long or it was another deadlock potential when calling ast_endpoint_add_channel().



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3067/#comment19895>

    I think you should combine this to ensure that the since is associated with the note:
    \note Since 12.0.0 ....



/branches/12/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/3067/#comment19896>

    idem



/branches/12/main/core_unreal.c
<https://reviewboard.asterisk.org/r/3067/#comment19897>

    Deadlock potential here.  The owner channel is locked when creating the chan channel.



/branches/12/main/message.c
<https://reviewboard.asterisk.org/r/3067/#comment19898>

    Deadlock potential.  chan is locked while unlinking.
    Swap the order of these statements.



/branches/12/res/res_calendar.c
<https://reviewboard.asterisk.org/r/3067/#comment19899>

    Holding the chan lock when calling ast_dial_run() is not a good thing since the function will not return until the channel answers.



/branches/12/res/res_calendar.c
<https://reviewboard.asterisk.org/r/3067/#comment19900>

    answered is chan here and chan is locked.



/branches/12/res/res_stasis_snoop.c
<https://reviewboard.asterisk.org/r/3067/#comment19901>

    Move this unlock before the SCOPED_CHANNELLOCK() scope.  There are a couple of returns inside that scope calling ast_hangup(snoop->chan) when snoop->chan and SCOPED_CHANNELLOCK() are locked.  Locking two channel locks requires deadlock avoidance.
    
    BUG detected: Speaking of those calls to ast_hangup().  You cannot call ast_hangup() inside the SCOPED_CHANNELLOCK() because of deadlock.  ast_hangup() cannot be called with any channel locks held.



/branches/12/tests/test_app.c
<https://reviewboard.asterisk.org/r/3067/#comment19902>

    Deadlock potential.
    
    Unlock the channels immediately rather than leaving them locked.
    
    There is also whitespace used instead of tabs in the if test line continuation.



/branches/12/tests/test_stasis_channels.c
<https://reviewboard.asterisk.org/r/3067/#comment19903>

    All ast_channel_alloc() calls in this file need to unlock the channel immediately.  ast_test_validate() is a macro that has an embedded return in it.  The test could fail with the channel still locked.
    
    There is also one test creating multiple channels with the usual deadlock potential of multiple channels locked.


- rmudgett


On Dec. 17, 2013, 12:33 p.m., Joshua Colp wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3067/
> -----------------------------------------------------------
> 
> (Updated Dec. 17, 2013, 12:33 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Currently when allocating a channel it is possible for external entities to become aware of the channel and try to interact with it before it is completely set up. This occurs because the channel is returned unlocked, and the caller allocating it is then expected to lock, populate it, and unlock. This leaves a small window where something else can get the channel.
> 
> The attached change makes it so ast_channel_alloc returns the allocated channel locked. It is expected that the caller then populates and unlocks. This leaves no window for something external to get and use it.
> 
> 
> Diffs
> -----
> 
>   /branches/12/tests/test_voicemail_api.c 403992 
>   /branches/12/tests/test_substitution.c 403992 
>   /branches/12/tests/test_stasis_channels.c 403992 
>   /branches/12/tests/test_cel.c 403992 
>   /branches/12/tests/test_cdr.c 403992 
>   /branches/12/tests/test_app.c 403992 
>   /branches/12/res/res_stasis_snoop.c 403992 
>   /branches/12/res/res_calendar.c 403992 
>   /branches/12/res/parking/parking_tests.c 403992 
>   /branches/12/main/pbx.c 403992 
>   /branches/12/main/message.c 403992 
>   /branches/12/main/core_unreal.c 403992 
>   /branches/12/main/channel.c 403992 
>   /branches/12/include/asterisk/channel.h 403992 
>   /branches/12/channels/chan_vpb.cc 403992 
>   /branches/12/channels/chan_unistim.c 403992 
>   /branches/12/channels/chan_skinny.c 403992 
>   /branches/12/channels/chan_sip.c 403992 
>   /branches/12/channels/chan_pjsip.c 403992 
>   /branches/12/channels/chan_phone.c 403992 
>   /branches/12/channels/chan_oss.c 403992 
>   /branches/12/channels/chan_nbs.c 403992 
>   /branches/12/channels/chan_multicast_rtp.c 403992 
>   /branches/12/channels/chan_motif.c 403992 
>   /branches/12/channels/chan_misdn.c 403992 
>   /branches/12/channels/chan_mgcp.c 403992 
>   /branches/12/channels/chan_jingle.c 403992 
>   /branches/12/channels/chan_iax2.c 403992 
>   /branches/12/channels/chan_h323.c 403992 
>   /branches/12/channels/chan_gtalk.c 403992 
>   /branches/12/channels/chan_dahdi.c 403992 
>   /branches/12/channels/chan_console.c 403992 
>   /branches/12/channels/chan_alsa.c 403992 
>   /branches/12/apps/confbridge/conf_chan_record.c 403992 
>   /branches/12/apps/app_voicemail.c 403992 
>   /branches/12/apps/app_meetme.c 403992 
>   /branches/12/addons/chan_ooh323.c 403992 
>   /branches/12/addons/chan_mobile.c 403992 
> 
> Diff: https://reviewboard.asterisk.org/r/3067/diff/
> 
> 
> Testing
> -------
> 
> Placed calls, ran tests, all work as expected.
> 
> 
> Thanks,
> 
> Joshua Colp
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131217/bc0d8c67/attachment-0001.html>