[asterisk-dev] [Code Review]: Keep chanspy from sinking its fangs into zombies. (biting zombies is bad for your health)

[jrose]

> On March 16, 2012, 12:08 p.m., Mark Michelson wrote:
> > /branches/1.8/apps/app_chanspy.c, lines 486-490
> > <https://reviewboard.asterisk.org/r/1819/diff/1/?file=26179#file26179line486>
> >
> >     I think it's definitely correct to be checking the zombie state of a channel within app_chanspy, but I think this should be done sooner. In channel_spy() there is an if block that checks ast_check_hangup() for the spy and spyee's channels. This seems like a good place to do the zombie chec, as well. This way, we don't do any extra work we don't have to (e.g. initializing an audio hook, sending a manager event saying we're spying on a channel).

That was actually my first inclination as well.  I changed it just because it seemed to me like attaching the audio hook seemed like the most important choking point (and it would cover all the other audiohook attaching in there as well), but since you are saying to just put it at as early as possible, that's fine as well.  I'll post the new diff. I already got it ready and tested it.


- jrose


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


On March 16, 2012, 11:33 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1819/
> -----------------------------------------------------------
> 
> (Updated March 16, 2012, 11:33 a.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and Matt Jordan.
> 
> 
> Summary
> -------
> 
> The call setup for this bug was sort of elaborate, but roughly 50% of the time, it would create a situation where chanspy can latch onto a zombie channel which would keep the zombie alive forever and keep itself in a perpetual loop. The approach of this patch is to keep that from happening by preventing chanspy from attaching audiohooks to zombies.
> 
> 
> This addresses bug ASTERISK-19493.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19493
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_chanspy.c 358857 
> 
> Diff: https://reviewboard.asterisk.org/r/1819/diff
> 
> 
> Testing
> -------
> 
> The test for this was against a configuration similar to the one described in the bug report.
> 
> I started by spying on a local channel...
> <extensions.conf>
> exten => 038,1,ChanSpy(Local/s at phones,qs)
> ...
> 
> That local channel was a member of a queue and when it picked up, it would dial a SIP peer.
> <queues.conf>
> [markq]
> member => Local/s at phones/n
> 
> <extensions.conf>
> [phones]
> exten => s,1,Dial(SIP/gold)
> 
> and I had a sip peer dial into that queue.
> <extensions.conf>
> exten => 037,1,Queue(markq)
> 
> 
> Between all of these, five channels were established:
> 
> 1 - SIP channel spying on the "local channel"
> 1 - SIP channel dialing into the queue
> 1 - SIP channel being dialed by a local channel
> 
> and 2 local channels bot with the name s at phones,qs with the difference between them being ;1 and ;2 at the end.
> 
> At this point, the chanspy could bind to either channel, and it would screw up as described in the bug report if the spy went to ;2.  What would happen is, channel 1 and 2 would hang up, but because they were both part of the same autochan group, if the chanspy was on 2, it would try to go and grab 1 before it could be destroyed. From there it would go into the spying loop and never come out.
> 
> 
> This patch fixed that behavior, but displays the new warning that can be seen in the diff.
> 
> 
> I'm going to go ahead and note that this is probably a silly way to use chanspy in the first place since that seems like something better suited for extenspy... but there is some potential for this patch to fix other bugs, so... eh, might be worth doing.
> 
> 
> Thanks,
> 
> jrose
> 
>

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