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

[Mark Michelson]

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


Definitely a good change. I, like you, wonder why ast_check_hangup() does not return true for the ;1 leg of a local channel when it clearly has been hung up. It looks as though chan_local does not do any sort of marking of softhangup on the two channels. I think that a change needs to go into chan_local to mark the softhangup flags on the owner and chan during a local_hangup(). Given, however, that messing with chan_local is like juggling bottles of nitroglycerin, I'd recommend saving such a patch for a later time.


/branches/1.8/apps/app_chanspy.c
<https://reviewboard.asterisk.org/r/1819/#comment10670>

    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).


- Mark


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/b135426b/attachment.htm>