[Home]

Summary:ASTERISK-09385: devicestate.c:__ast_device_state_changed_literal can't cope with iax (and sip?) peer names containing "-"
Reporter:Steve Davies . (stevedavies)Labels:
Date Opened:2007-05-04 14:31:23Date Closed:2008-01-04 17:09:02.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Addons/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20070907__bug9668.diff.txt
Description:__ast_device_state_changed_lieral does this:

       device = buf;
       if ((tmp = strrchr(device, '-')))
               *tmp = '\0';

With this result in my environment:

[May  2 09:19:12] DEBUG[11129] devicestate.c: Notification of state change to be queued on device/channel IAX2/swenitech-2
[May  2 09:19:12] DEBUG[11129] devicestate.c: No provider found, checking channel drivers for IAX2 - swenitech
[May  2 09:19:12] DEBUG[11129] chan_iax2.c: Checking device state for device swenitech
[May  2 09:19:12] DEBUG[11129] devicestate.c: Changing state for IAX2/swenitech - state 4 (Invalid)
[May  2 09:19:14] DEBUG[11129] devicestate.c: Notification of state change to be queued on device/channel IAX2/swenitech-1
[May  2 09:19:14] DEBUG[11129] devicestate.c: No provider found, checking channel drivers for IAX2 - swenitech
[May  2 09:19:14] DEBUG[11129] chan_iax2.c: Checking device state for device swenitech
[May  2 09:19:14] DEBUG[11129] devicestate.c: Changing state for IAX2/swenitech - state 4 (Invalid)
[May  2 09:19:15] DEBUG[11129] devicestate.c: Notification of state change to be queued on device/channel IAX2/vpntech-1
[May  2 09:19:15] DEBUG[11129] devicestate.c: No provider found, checking channel drivers for IAX2 - vpntech
[May  2 09:19:15] DEBUG[11129] chan_iax2.c: Checking device state for device vpntech
[May  2 09:19:15] DEBUG[11129] devicestate.c: Changing state for IAX2/vpntech - state 4 (Invalid)

etc etc.

I guess that truncating at the last "-' is to do with cleaning up things like Zap/1-1 and similar.

But the result breaks devicestate for any iax2 or sip peer with a dash in the name.

Perhaps it would be possible to queue the devicestate for both the complete and the truncated version in order to deal with this issue?

Steve

****** ADDITIONAL INFORMATION ******

SVN trunk rev 62263, 2007-04-27
Comments:By: Joshua C. Colp (jcolp) 2007-05-14 13:41:32

Fixed in 1.2 as of revision 64275, 1.4 as of revision 64276, and trunk as of revision 64277. Thanks!
By: Steve Davies . (stevedavies) 2007-05-14 15:09:19

Thanks, but is this a good fix?

For one, SIP also constructs channel names by appending -something to the device name.
If ast_device_changed_literal has to handle being called with Zap/1-1, then why not SIP/peer-stuff.

For example, from my logs:

> Notification of state change to be queued on device/channel SIP/0878202300-118b4cd0
> Notification of state change to be queued on device/channel SIP/0878202300-0dba0bc0

Steve
By: Olle Johansson (oej) 2007-05-15 11:23:26

I think the fix is bad and vaguely remembers that this has been up for discussion before. I recommend all my students *not* to use "-" in device names... The devicestate system needs the peer name as the first part.
By: Steve Davies . (stevedavies) 2007-05-16 11:05:30

I did review all my devicestate messages on my box and it did seem that only Zaps are seen with a Zap/1-1 type of format.
By: R. Voermans (voermans) 2007-06-06 04:38:33

Olle is indeed right. This fix is bad. SIP channels are of the form SIP/1-<channelid>. When subscribing to a SIP extension, the extension state isn't being updated. Please revert back to the old style of devicestate.c.
By: Joshua C. Colp (jcolp) 2007-06-06 07:21:33

I've reverted this in 1.2 as of revision 67593, 1.4 as of revision 67594, and trunk as of revision 67595 until we can figure out what to do.
By: Leif Madsen (lmadsen) 2007-06-06 10:00:47

Perhaps we need to just make - an invalid character here and give an ERROR or WARNING when someone uses it. Lets not also forget to make a note in the sample file, etc... that - is illegal (assuming this is indeed the way we want to handle this if no other fix can be found).
By: mackey (mackey) 2007-07-12 09:00:48

How about only chopping if the channel starts with "SIP/" ?
By: James Golovich (jamesgolovich) 2007-09-07 02:06:12

Why not just convert all potential bad chars (not sure if there are any besides '-') to the ascii hex equiv and replace them in the string.  So change '-' to '%2D'  I suppose if that was going to take place then '%' would be another char that would want to be replaced.

Just throwing the idea out there, haven't looked into what code would need to be written to do it.

If we don't have a function already for it, a generic function that you can pass the string and the chars to replace with their hex value and then another function to "unescape" the string (if its necessary at all)
By: Tilghman Lesher (tilghman) 2007-09-07 08:24:32

The issue, as I understand it, is that you can pass either form of a channel name to devicestate and it should work.  That means all of the following should work correctly:

SIP/123-ABCD1234
SIP/123
SIP/foo-bar-ABCD1234
SIP/foo-bar
(IAX has the same style formats as SIP)

Essentially what you're proposing is to make '-' an invalid character in a channel name, which is the same as oej's workaround.  Note that this would probably cause a flag day event, whereas we'd be passing the encoded form to remote hosts (and could cause its own set of issues).

What we might could do, instead, is to first do a lookup on the name as passed, and if the state is invalid, only then do we zero the final '-', and do a second lookup on the name.  This would ensure that both forms of names would work correctly, although there still might be a problem with two SIP hosts of the following forms:
SIP/foo
SIP/foo-bar

What do we think of this approach?  In the general case, it should not be much slower than the existing system.

(Edit: it looks like stevedavies proposed this approach in the original report, so giving credit where credit is due.)


By: James Golovich (jamesgolovich) 2007-09-07 11:38:28

I haven't looked deep into the code yet for this, I might have some time this afternoon.

I don't believe that we would have to be sending an encoded version over to a remote host, since we would be using either the IP address or an unencoded hostname.

ast_gethostbyname would be modified to do the decoding, so the query would be for the undecoded name.

I may just be totally off here since I haven't looked at the code.  If I get a chance today I'll see if what I'm proposing is even possible or reasonable, if not today then I'll have a chance over the weekend.
By: Tilghman Lesher (tilghman) 2007-09-07 11:51:15

There's my suggestion, implemented for 1.4.
By: James Golovich (jamesgolovich) 2007-09-07 12:52:35

Looks like a very simple solution.  Should there be a return inside the if { } in case it gets matched we won't go on further processing.

Assuming the following exist:
SIP/foo
SIP/foo-bar
SIP/foo-bar-foo
SIP/foo-bar-foo-bar

If it gets called for SIP/foo-bar-foo-bar then it will recurse all the way down to SIP/foo
By: Tilghman Lesher (tilghman) 2007-09-09 09:44:09

It doesn't actually attempt to do a match, just forces an update, so there's no way to know if it succeeded or not.  It is harmless to call this on a device whose state did not change, which is why I didn't bother to add a parameter to make it stop after one level.
By: Olle Johansson (oej) 2007-12-18 02:31:02.000-0600

The proper fix would be to separate the data, really. Let's add device address to the channel structure and not rely on it being embedded in the channel ID. That will also help blitzrage's efforts of creating a $DEVICE() dialplan function, as the channel would know the device as a "default" device for a channel.
By: Digium Subversion (svnbot) 2008-01-04 16:59:59.000-0600

Repository: asterisk
Revision: 96575

U   branches/1.4/main/devicestate.c

------------------------------------------------------------------------
r96575 | tilghman | 2008-01-04 16:59:58 -0600 (Fri, 04 Jan 2008) | 7 lines

Fix the problem of notification of a device state change to a device with a '-'
in the name.  Could probably do with a better fix in trunk, but this bug has
been open way too long without a better solution.
Reported by: stevedavies
Patch by: tilghman
(Closes issue ASTERISK-9385)

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

http://svn.digium.com/view/asterisk?view=rev&revision=96575
By: Digium Subversion (svnbot) 2008-01-04 17:09:02.000-0600

Repository: asterisk
Revision: 96576

_U  trunk/
U   trunk/main/devicestate.c

------------------------------------------------------------------------
r96576 | tilghman | 2008-01-04 17:09:01 -0600 (Fri, 04 Jan 2008) | 15 lines

Merged revisions 96575 via svnmerge from
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r96575 | tilghman | 2008-01-04 17:03:40 -0600 (Fri, 04 Jan 2008) | 7 lines

Fix the problem of notification of a device state change to a device with a '-'
in the name.  Could probably do with a better fix in trunk, but this bug has
been open way too long without a better solution.
Reported by: stevedavies
Patch by: tilghman
(Closes issue ASTERISK-9385)

........

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

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