[Home]

Summary:ASTERISK-10860: RTP session ID is negative half the time on x86_64
Reporter:Simon Perreault (sperreault)Labels:
Date Opened:2007-11-22 08:19:52.000-0600Date Closed:2007-11-27 10:58:47.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) ast_random_int.diff
( 1) ast_random.diff
( 2) ast_random2.diff
Description:I'm surprised this hasn't been reported before (which makes me doubt my findings).

ast_random() returns a positive long. When this gets truncated to an int, it is no longer guaranteed positive. It will be negative exactly half the time. This may make clients very unhappy. Eyebeam, for example, drops the connection when it sees a negative session ID.

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

Index: channels/chan_sip.c
===================================================================
--- channels/chan_sip.c (revision 89526)
+++ channels/chan_sip.c (working copy)
@@ -7124,7 +7124,8 @@

       /* Set RTP Session ID and version */
       if (!p->sessionid) {
-               p->sessionid = (int)ast_random();
+               int r = ast_random();
+               p->sessionid = r < 0 ? ~r : r;
               p->sessionversion = p->sessionid;
       } else
               p->sessionversion++;
Comments:By: Simon Perreault (sperreault) 2007-11-22 08:29:53.000-0600

Other similar changes, fixes r77269 completely:

Index: channels/chan_sip.c
===================================================================
--- channels/chan_sip.c (revision 89526)
+++ channels/chan_sip.c (working copy)
@@ -6950,7 +6950,8 @@
       }

       if (!p->sessionid) {
-               p->sessionid = (int)ast_random();
+               int r = ast_random();
+               p->sessionid = r < 0 ? ~r : r;
               p->sessionversion = p->sessionid;
       } else
               p->sessionversion++;
@@ -8679,8 +8680,9 @@
       peer->addr.sin_port = htons(port);
       if (sipsock < 0) {
               /* SIP isn't up yet, so schedule a poke only, pretty soon */
+               int r = ast_random();
               peer->pokeexpire = ast_sched_replace(peer->pokeexpire, sched,
-                       ast_random() % 5000 + 1, sip_poke_peer_s, peer);
+                       (r < 0 ? ~r : r) % 5000 + 1, sip_poke_peer_s, peer);
       } else
               sip_poke_peer(peer);
       peer->expire = ast_sched_replace(peer->expire, sched,
By: Simon Perreault (sperreault) 2007-11-22 08:32:18.000-0600

Note that calling abs() is also wrong because it is not defined for INT_MIN.
By: Dmitry Andrianov (dimas) 2007-11-22 08:47:25.000-0600

isn't it better to instroduce ast_random_int (or something) to return random signed int which is alwasy posituve?
(there also can be ast_random_uint etc)
By: Simon Perreault (sperreault) 2007-11-22 08:55:55.000-0600

Good idea. I upload the patch right away.
By: Dmitry Andrianov (dimas) 2007-11-22 13:41:36.000-0600

To me, your ast_random_int looks overcomplicated - why you need #ifdef HAVE_DEV_URANDOM there?
Why not to have single function which gets random bits (ast_random in this case) and other functions just using that one?
By: Simon Perreault (sperreault) 2007-11-22 13:56:33.000-0600

Well, ast_random() gets a long from /dev/urandom while all we need is an int. (A full 2x less!) So maybe we don't want to read a whole long, maybe we don't care. If the patch applier doesn't care, feel free to just remove all the #ifdef HAVE_DEV_URANDOM part.
By: Dmitry Andrianov (dimas) 2007-11-22 14:12:17.000-0600

afaik int = long and both a 32 bits.
long long is in contrast 64, short is 16
By: Simon Perreault (sperreault) 2007-11-22 14:17:41.000-0600

That's only true on a 32-bit arch. The cause of this bug is precisely that it isn't true on a 64-bit arch.
By: Dmitry Andrianov (dimas) 2007-11-22 15:11:43.000-0600

Hm... According to the random manpage, it returns "numbers in the range from 0 to RAND_MAX" that is positive. Since ast_random falls back to random in the case /dev/urandom is missing, isn't it a good idea just to fix ast_random so it will be alays returning positive values?
By: Simon Perreault (sperreault) 2007-11-22 15:22:43.000-0600

If by "positive values" you mean "values between 0 and RAND_MAX", then yes, I think it would be a good idea. Boringly simple to do, but better in the end. ;)
By: Olle Johansson (oej) 2007-11-23 02:38:02.000-0600

Is this  a problem with 1.4 as well?
By: P. Christeas (xrg) 2007-11-23 07:10:27.000-0600

I read that code in 1.4(.14): pokeexpire uses the ast_random..
We should comb the code for incorrect uses of (int)ast_random() in other files..
By: Simon Perreault (sperreault) 2007-11-23 07:52:14.000-0600

I just uploaded a better, simpler patch that makes ast_random() always return a number between 0 and RAND_MAX even when using /dev/urandom on a 64-bit arch. Now it's not needed to go back and fix all uses of ast_random() as an int because casts are safe.

Please consider applying and closing this bug.
By: Olle Johansson (oej) 2007-11-23 07:55:12.000-0600

hmm.. My question is still around: Is this a problem in 1.4 as well?
By: Simon Perreault (sperreault) 2007-11-23 08:10:24.000-0600

Hmmm... let's see...

#ifdef linux
#define ast_random random
#else
long int ast_random(void);
#endif

[...]

/*! \brief glibc puts a lock inside random(3), so that the results are thread-safe.
* BSD libc (and others) do not. */
#ifndef linux

AST_MUTEX_DEFINE_STATIC(randomlock);

long int ast_random(void)
{
       long int res;
       ast_mutex_lock(&randomlock);
       res = random();
       ast_mutex_unlock(&randomlock);
       return res;
}
#endif

Calls to ast_random() always result in calls to random(), which always returns a number between 0 and RAND_MAX, which is safely castable to an int. 1.4 is safe.
By: Simon Perreault (sperreault) 2007-11-23 13:00:05.000-0600

Damn casts are sneaky. ast_random2.diff is the one that works without warnings.
By: Digium Subversion (svnbot) 2007-11-26 09:25:47.000-0600

Repository: asterisk
Revision: 89576

U   trunk/main/utils.c

------------------------------------------------------------------------
r89576 | file | 2007-11-26 09:25:47 -0600 (Mon, 26 Nov 2007) | 6 lines

Make the behavior of using /dev/urandom for random numbers the same as random().
(closes issue ASTERISK-10860)
Reported by: sperreault
Patches:
     ast_random2.diff uploaded by sperreault (license 252)

------------------------------------------------------------------------
By: Digium Subversion (svnbot) 2007-11-27 10:58:47.000-0600

Repository: asterisk
Revision: 89637

U   trunk/main/utils.c

------------------------------------------------------------------------
r89637 | file | 2007-11-27 10:58:46 -0600 (Tue, 27 Nov 2007) | 4 lines

Ensure the value returned from ast_random is between 0 and RAND_MAX on 64-bit platforms.
(closes issue ASTERISK-10860)
Reported by: sperreault

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