[Home]

Summary:ASTERISK-07058: [patch] SIP calls going through the same device can have duplicate channel names
Reporter:Paulo Mendes da Silva (kanelbullar)Labels:
Date Opened:2006-05-30 12:24:47Date Closed:2006-06-26 12:33:32
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) patch_sip_channel_with_unique_id.txt
Description:We have found a problem in the AMI channel identifier generation, using SIP. When making multiple simultaneous calls through the same device, in this case, a SIP extension, we can have two totally unrelated calls with the same AMI channel identifier, at the same time. This is a serious problem since we won't be able to tell both calls appart and therefore won't be able to control them properly using AMI. Channel identifiers should be unique for the duration of a call, since they are used as unique identifiers for controlling each call through AMI.

In the following code snippet, we can see where the problem is. When a 'title' is present (a user identifier), the channel is built as the concatenation of the SIP protocol name, the user name and a random number. When making multiple calls through the same user, the only difference will be the tail, which is, in this case, a four digit random number. As such, it may happen that the same random number is generated in a small interval, which, as we observed, can lead to having two simultaneous calls with the same channel identifier.

static struct ast_channel *sip_new(struct sip_pvt *i, int state, char *title)
{
(...)
      if (title)
               snprintf(tmp->name, sizeof(tmp->name), "SIP/%s-%04x", title, thread_safe_rand() & 0xffff);
(...)
}

We are providing a patch for this problem. Instead of using random numbers, we propose using a counter that will be incremented each time the 'sip_new' function is called. That will prevent us from having duplicate identifiers for unrelated calls.



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

Just for completion's sake, here is an example of duplicate channels in a short interval (both calls are going through SIP user 'cid'):

Event: Newchannel
Privilege: call,all
Channel: SIP/cid-f725
State: Down
CallerID: <unknown>
CallerIDName: <unknown>
Uniqueid: 959162692.4733
(...)
Event: Newchannel
Privilege: call,all
Channel: SIP/cid-f725
State: Down
CallerID: <unknown>
CallerIDName: <unknown>
Uniqueid: 959162700.4795

These calls existed simultaneously.
Comments:By: Andrey S Pankov (casper) 2006-05-30 15:11:45

Why not to have that part long not int?

And why not to modify tread_safe_rand to use (long)random, not (int)rand?
By: Serge Vecher (serge-v) 2006-05-30 16:09:20

or, how about a built-in ast_random()?
By: Tilghman Lesher (tilghman) 2006-05-30 16:41:44

ast_random() is only in trunk.
By: Tilghman Lesher (tilghman) 2006-05-30 17:02:25

A problem with your patch is that it's not threadsafe.  If two channels are allocated at the same instant, then it's still possible for them to show up with exactly the same identifier.

A possible alternative would be to use the second half of the chan->uniqueid field, as this is already an incremented integer which is threadsafe.
By: Paulo Mendes da Silva (kanelbullar) 2006-05-31 04:30:29

We assumed the counter was incremented in a thread safe critical region, since the increment instruction was executed after the following lock:

ast_mutex_lock(&i->lock);

And there was no visible unlock instruction. Is there a chance this lock might be unlocked somewhere else?
By: Tilghman Lesher (tilghman) 2006-05-31 11:01:21

That's a lock to the private channel structure, which is allocated per channel.  It does NOT protect against two different channels using a global variable.
By: Paulo Mendes da Silva (kanelbullar) 2006-05-31 12:07:43

Ok, thanks for the clarification. We will follow your suggestion and use the second half of the chan->uniqueid field instead. We will post a new patch as soon as we have it.
By: Paulo Mendes da Silva (kanelbullar) 2006-06-01 04:11:34

We have provided a new patch. This time we are using the second half of the uniqueid field, as suggested.
By: Paulo Mendes da Silva (kanelbullar) 2006-06-01 04:14:11

Please delete the old patch, as I don't have permissions to do so. Thanks.
By: Kevin P. Fleming (kpfleming) 2006-06-26 12:32:14

After discussion with the poster in person at Astricon, I decided to fix this a different way... since there are two other ways for the channel name to be generated with an 8 character suffix instead of 4, changing to always generate 8 characters and using the private structure address will solve the problem.

Fixed in branch-1.2 revision 36078 and trunk revision 36079.