[asterisk-dev] [Code Review] Fix a deadlock in agents occuring due to trying to lock a channel while having a lock on the pvt.

[Matt Jordan]

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



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9958>

    You can just use owner = ast_channel_ref(p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9964>

    I'd remove these comments - I don't think it adds much, and it seems to incorrectly apply that the first lock of p->lock isn't needed, when it is - it keeps the pvt locked while you determine if it has a channel owner.  So you really are using the lock when you check if (p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9948>

    You can just do owner = ast_channel_unref(owner) here; it will assign owner to NULL after unref'ing.



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9949>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9957>

    Document here that this method will lock both the pvt and the owning channel



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9959>

    You can just use owner = ast_channel_ref(p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9950>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9960>

    You can just use owner = ast_channel_ref(p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9951>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9961>

    You can just use owner = ast_channel_ref(p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9952>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9962>

    You can just use owner = ast_channel_ref(p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9953>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9963>

    You can just use owner = ast_channel_ref(p->owner)



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9954>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9955>

    owner = ast_channel_unref(owner) here



/branches/1.8/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/1708/#comment9956>

    owner = ast_channel_unref(owner) here


- Matt


On Feb. 2, 2012, 9:01 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1708/
> -----------------------------------------------------------
> 
> (Updated Feb. 2, 2012, 9:01 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson, rmudgett, and Matt Jordan.
> 
> 
> Summary
> -------
> 
> Also adds locking to a number of other functions which are calling ast_bridged_channel (which is documented as requiring a lock for safe running, which was the purpose of irroot's original locking patch in action_agents.
> 
> Unlike the other patch on reviewboard right now, this opts to enforce locking order instead of using deadlock avoidance.
> 
> Celebrity endorsement: "This looks incredibly sane to me." - MMichelson
> 
> 
> This addresses bug ASTERISK-19285.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19285
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/channels/chan_agent.c 353685 
> 
> Diff: https://reviewboard.asterisk.org/r/1708/diff
> 
> 
> Testing
> -------
> 
> Not much I'm afraid to say. I can't reproduce the issue since it involves real world use over a period of time.  I'll ask Alex Villacís Lasso to give it a shot though.
> 
> 
> Thanks,
> 
> jrose
> 
>

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