[Home]

Summary:ASTERISK-12191: Unparked caller has ability to transfer
Reporter:David Woolley (davidw)Labels:
Date Opened:2008-06-13 13:12:34Date Closed:2008-10-23 10:54:07
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Resources/res_features
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) no-re-invite-on-unpark.log
( 1) parkingfix3.diff.txt
( 2) parkingfix4.diff.txt
Description:Please re-open ASTERISK-9286.  It was closed in favour of ASTERISK-1182898, but the fix for that did not actually address the issue in ASTERISK-9286.
Comments:By: David Woolley (davidw) 2008-09-24 05:18:41

Any progress on this?

The problem it causes us is that it inhibits re-invites and therefore the removal of Asterisk from the speech path, which is an important requirement for us.
By: Terry Wilson (twilson) 2008-10-06 15:42:38

I will try to take a look at this again.  It seems to be relatively difficult to fix all incarnations of this bug at the same time.
By: Terry Wilson (twilson) 2008-10-07 14:10:33

davidw: can you get me a dialplan example and description of exactly what you are doing?  I've tried several scenarios and it looks like things are working to me.  (I think there are still some other transfer related bugs in there that I'm looking at, but parking seems to work for my tests)
By: David Woolley (davidw) 2008-10-08 07:43:26

The short answer, is that I believe these are the lines that cause the problem (current SVN HEAD for 1.4, res/res_features.c SVN 146129):

  2190                 ast_set_flag(&(config.features_callee), AST_FEATURE_REDIRECT);
  2191                 ast_set_flag(&(config.features_caller), AST_FEATURE_REDIRECT);

What I don't know is why these lines exist, so I don't know if there will be collateral damage from the obvious fix.

I'm going to double check that we still have the problem, but I believe we do. I'll probably also try the obvious fix again, just to make sure these were the lines we tried changing last time. That will be with 1.4.21.2.

The dialplan interacts with AMI controlled by a VB.NET application.  We are still thinking how best to simplify the model.

However the basic sequence is:

Call arrives and eventually reaches an agent.  The agent, via AMI parks the call and sets up an outbound call to someone else.  Once that call is setup up, they use AMI to redirect the outbound call to the parked call.  Both the incoming call and outbound call are SIP, and not NATted, and we want Asterisk to drop out of the RTP path, but it fails to drop out because the REDIRECT allowed flags requires Asterisk see the DTMF.
By: David Woolley (davidw) 2008-10-08 08:18:00

Reconfirmed.  This is the unpark with out of the box 1.4.21.2:

   -- Executing [701@parkedcalls:1] ParkedCall("SIP/6106-090a0840", "701") in new stack
   -- Stopped music on hold on SIP/192.168.10.10-09081de0
   -- Channel SIP/6106-090a0840 connected to parked call 701

and if one looks at sip show channel for the second party, one sees that they have a "local" bridge.

 NAT Support:            RFC3581
 Audio IP:               192.168.130.116 (local)
 Our Tag:                as75502210

This is the same sequence, but with the above mentioned lines disabled:

   -- Executing [701@parkedcalls:1] ParkedCall("SIP/6106-09237870", "701") in new stack
   -- Stopped music on hold on SIP/192.168.10.10-0921e9c0
   -- Channel SIP/6106-09237870 connected to parked call 701
   -- Native bridging SIP/6106-09237870 and SIP/192.168.10.10-0921e9c0

Note the native bridging.  Also note that sip show channel on the second party shows an Outside bridge

 NAT Support:            RFC3581
 Audio IP:               192.168.10.10 (Outside bridge)
 Our Tag:                as25c3aad3
By: David Woolley (davidw) 2008-10-08 08:43:41

Turns out you can actually demonstrate it with the simplest of parking scenarios, and this dialplan (where the include default is probably redundant):

[internal]

include => default
include => parkedcalls

By simple, I mean dial 700 on one phone and 701 on another.

I'll attach the console output as it essentially the same as before, but I've included everything.


By: Terry Wilson (twilson) 2008-10-09 14:31:28

Ok, would it be possible for you to try the attached patch and see if it fixes the issue for you (and doesn't break anything else)?
By: David Woolley (davidw) 2008-10-10 09:37:31

It won't build against 1.4.21.2, so I tried 1.4.22 (which is probably going to be our fallback, if we fail to port to 1.6) and got:

   -- Channel SIP/djw-messenger-092c98b8 connected to parked call 701
[Oct 10 15:26:42] NOTICE[32256]: res_features.c:2077 park_exec: Couldn't find di
alfeatures!

when doing the simple case with 700 and 701.

There was no re-invite.

With the actual application use, there was no warning, but still no re-invite.

This whole dialfeatures thing seems to be new, but, speculating, could we have a situation where, because we use Originate, to an application, Asterisk has dial features left over from a Dial command earlier in the call.  We did experiment with enabling transfer at one point in the call to support a secondary requirement.  I.E. should dial features maybe be cleared on entering the dial plan?

I assume the failure in the simple case is because no Dial command has ever been issued.
By: Terry Wilson (twilson) 2008-10-10 09:47:40

Yeah, it looks like the datastore is only added in app_dial.   Bah.  I'll look at adding the diialfeature to the other places if it is possible...This issue just won't die!
By: David Woolley (davidw) 2008-10-10 09:48:32

I removed the ,t from the original Dial application, and it now re-invites, so I would say that dial features information is getting left over from the last Dial in the call's history.  That call is, effectively, the call to the agent.

Given that parking operations don't involve a dial command, I'm not sure that you can sensibly infer dial features for them, unless you supply them with the Park or ParkedCall application.
By: Terry Wilson (twilson) 2008-10-10 10:31:38

Ok, in this patch I've taken out the "backward compatibility" part that I put in that was causing your issue. Can I get you to test this one in your setup?
By: David Woolley (davidw) 2008-10-10 11:02:07

Tried the simple case:

Program terminated with signal 11, Segmentation fault.
#0  0x00d276cc in park_exec (chan=0xa0c1e50, data=0xb76c9f58)
   at res_features.c:2078
2078       if (!dialfeatures->is_caller) {

I presume I don't need more!
By: Terry Wilson (twilson) 2008-10-10 11:04:10

bah, that is what I get for making a "quick change" that "couldn't possibly break things" w/o testing.  My sincere apologies--won't happen again.
By: Terry Wilson (twilson) 2008-10-10 11:12:08

Despite my above message that I wouldn't do it again, I've posted another patch w/o testing the no dialfeatures case because it will take me a bit to get the manager originate set up and I have to leave for a lunch meeting real quick...so I thought I'd at least post the patch that I think fixes it--but, with the warning that it is completely untested and may steal you firstborn, etc. in case you wanted to test it while I was gone for about an hour.  Otherwise, I'll test it when I get back.
By: David Woolley (davidw) 2008-10-10 11:44:49

Note that the crash happens for the simple case, which doesn't involve AMI.
By: David Woolley (davidw) 2008-10-10 12:56:42

I was about to try the patch, but we had VMWare crisis and the test machine wasn't receiving any CPU time!  I'll be out of the office on Monday, so I won't be able to run any tests till Tuesday, now.
By: Terry Wilson (twilson) 2008-10-10 14:14:48

Tested w/ both regular dialplan and manager originate, and it worked for me.
By: Terry Wilson (twilson) 2008-10-14 10:31:39

It's Tuesday!  (Ok, so it is just Tuesday morning)  Just checking if you had a chance to test to make sure I haven't broken things again, and that what works for me works for you as well.  :-)
By: David Woolley (davidw) 2008-10-14 12:16:17

It's OK for direct 700/701 operation.  It's OK for AMI provided that the connection to the parker didn't have transfer permission.  If the initial connection to the parker had transfer permission, the unparker gets that, which doensn't feel right to me.

This isn't a problem for our primary requirement, although it eliminates one possible strategy for dealing with the case of a CTI failure.  However, it seems to give away a privilege.
By: Terry Wilson (twilson) 2008-10-15 00:36:05

Hmm, I'll see if I can't come up with another patch tomorrow.  :-/
By: Terry Wilson (twilson) 2008-10-15 10:54:45

Ok, here is the test I am doing with AMI:

Two phones, 6001 and 6002

AMI Originate channel=6001 context=default extension=6002

[default]
exten=> 6002,1,Dial(SIP/$6002,,T)

This gives 6001 the ability to do a native transfer.  If 6002 parks the call, then unparks it, 6001 still has the ability to transfer, and 6002 still does not.  So the channel connected to the parker doesn't seem to be transferring that ability to the parker.  Am I doing a different test than you?

Now, one thing that this patch does do is it treats whoever unparks the call like the channel that parked it as far as permissions go.  I'm not sure that is entirely the right thing to do, but as you mentioned before--the only way to do that would be to modify Park commands to take dial features.  I'm pretty sure that that wouldn't be doable in 1.4 at least, though.  I'll ask around.
By: Terry Wilson (twilson) 2008-10-15 11:08:49

Also, since the parkedcalls context is created automatically, there is no way to correctly transfer the options that way.
By: David Woolley (davidw) 2008-10-16 07:24:17

I believe you can explicitly add ParkedCall to the dialplan, or, for that matter, Park.

Note, our Originate has, say, 701 as the target.  The source is actually a local channel, which gets optimised out when the call completes, although I believe the behaviour would be the same, in this respect, if it were the SIP channel, directly.

I notice that 1.6.0 seems to have explicit control in this area, and the default option seems to be not to give permissions to the unparked call.


By: Terry Wilson (twilson) 2008-10-20 12:05:30

Huh, I never noticed that that was there in 1.6.0+.  Asked around about this issue and didn't hear anything about it then either.  I'll check to see whether, since the current behavior is demonstrably incorrect, or not it is ok to backport this change to 1.4, but have the default be 'both' to match current behavior so we don't break anyone who is relying on it behaving that way--but still allow people who need to fix the issue to fix it.
By: Terry Wilson (twilson) 2008-10-20 14:10:29

Does parkingfix4.diff.txt meet your approval?  I have backported the parkedcalltransfers feature from 1.6.0 with default to "both".  Setting parkedcalltransfers=no in features.conf should fix everything for you (I hope).
By: Terry Wilson (twilson) 2008-10-22 15:29:24

davidw: Is this solution ok with you?  I'd like to get this committed if so.
By: David Woolley (davidw) 2008-10-23 05:43:44

That looks OK.

Sorry for the delay, but I was chasing some other Asterisk issues which had no solution and were more obvious to end users.
By: Digium Subversion (svnbot) 2008-10-23 10:54:04

Repository: asterisk
Revision: 151763

U   branches/1.4/CHANGES
U   branches/1.4/configs/features.conf.sample
U   branches/1.4/res/res_features.c

------------------------------------------------------------------------
r151763 | twilson | 2008-10-23 10:54:03 -0500 (Thu, 23 Oct 2008) | 9 lines

Backport fix from 1.6.0 that allows you to set parkedcalltransfers=no|caller|callee|both, but default to both which would be the equivalent of the existing behaviour.

The problem was that if someone parked a call, the callee and caller would both get assigned the builtin transfer feature, which would not only be potentially giving someone the ability to transfer themselves when they shouldn't have it, but would also dissallow reinviting the media off of the call.
(closes issue ASTERISK-12191)
Reported by: davidw
Patches:
     parkingfix4.diff.txt uploaded by otherwiseguy
 Tested by: davidw, otherwiseguy

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=151763