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

[jrose]

> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, lines 1548-1559
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line1548>
> >
> >     Should use owner instead of p->owner after you grabbed the owner lock.

Check.


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, line 2469
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line2469>
> >
> >     Owner lock and ref not released here.

Check.


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, line 2477
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line2477>
> >
> >     Owner lock and ref not released here.

Check.


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, lines 922-932
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line922>
> >
> >     Should not have private lock held here because of potential deadlock with channel locks.

check


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, lines 587-603
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line587>
> >
> >     Potential use of new locking method.

I'm not sure how much I can do with p->chan on this just yet, but I'll go ahead and use that locking method for p->owner.


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, lines 927-931
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line927>
> >
> >     Holding chan lock while calling ast_indicate_data() *will* cause a deadlock if it is a local channel.

check


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, lines 1607-1627
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line1607>
> >
> >     Ooh look an opportunity to use the new locking method.

Check


> On Feb. 1, 2012, 3:49 p.m., rmudgett wrote:
> > /branches/1.8/channels/chan_agent.c, line 565
> > <https://reviewboard.asterisk.org/r/1708/diff/1/?file=23867#file23867line565>
> >
> >     Potential deadlock trying to grab two channel locks in ast_read().

I'm still a little iffy on this.  I'll post the rest of the changes and hit this tomorrow.


- jrose


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


On Feb. 1, 2012, 2:20 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1708/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2012, 2:20 p.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/20120201/cebaa0df/attachment-0001.htm>