[Home]

Summary:ASTERISK-06690: [patch] Poor use of rand() scaling
Reporter:Alan Ferrency (ferrency)Labels:
Date Opened:2006-04-03 15:18:22Date Closed:2006-04-05 12:45:23
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20060403__bug6873.diff.txt
Description:As seen in app_queue.c and elsewhere throughout Asterisk:

1995 :     tmp->metric = rand() % 1000;

This is not a good way to scale random numbers to a specific range of integers, because it throws away a lot of precision in the pseudorandom stream.

My `man rand` suggests a better way to scale rand(), which could easily be turned into a macro or subroutine that accepts the range as an integer parameter:


      In Numerical Recipes in C: The Art of Scientific Computing (William  H.
      Press, Brian P. Flannery, Saul A. Teukolsky, William T. Vetterling; New
      York: Cambridge University Press, 1992 (2nd ed., p. 277)), the  follow-
      ing comments are made:
             "If  you want to generate a random integer between 1 and 10, you
             should always do it by using high-order bits, as in

                    j=1+(int) (10.0*rand()/(RAND_MAX+1.0));

             and never by anything resembling

                    j=1+(rand() % 10);

             (which uses lower-order bits)."


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

This technique is also used in other, possibly more important places; circa Asterisk 1.2.3:

./asterisk-addons-1.2.1/asterisk-ooh323c/ooh323c/src/ooq931.c:      lastCallRef = (ASN1USINT)(rand()%100);
./asterisk-addons-1.2.1/asterisk-ooh323c/ooh323c/src/ooh245.c:   statusDeterminationNumber = rand()%16777215;
./libpri-1.2.2/q921.c:   if (!(random() % 3)) {
./zaptel-1.2.2/hdlcgen.c:               cnt = (rand() % 256) + 4;       /* Read a pseudo-random amount of stuff */
./zaptel-1.2.2/hdlcgen.c:               flags = (rand() % 4);
./asterisk/rtp.c:       x = (rand() % (rtpend-rtpstart)) + rtpstart;
./asterisk/apps/app_random.c:   if ((random() % 100) + probint > 100) {
./asterisk/apps/app_queue.c:            tmp->metric = rand() % 1000;
./asterisk/channels/chan_sip.c:         peer->pokeexpire = ast_sched_add(sched, thread_safe_rand() % 5000 + 1, sip_poke_peer_s, peer);
./asterisk/channels/chan_agent.c:                       snprintf(tmp->name, sizeof(tmp->name), "Agent/P%s-%d", p->agent, rand() & 0xffff);


This may not be a "real" bug, but it may explain other reported "rand isn't random" behavior, or it may be a bug lurking and waiting to be exercised.

Alan
Comments:By: Tilghman Lesher (tilghman) 2006-04-03 17:45:47

There's already work in Asterisk to convert to using ast_random(), which takes care of two problems:  first, that rand() lower order bits aren't all that random, and second, that there's a race condition with a multithreaded apps calling rand(), because rand() is not threadsafe (or not threadsafe on all platforms).

If you'd like to contribute a patch to convert places that use rand() to use ast_random() (with a disclaimer on file), that would be graciously accepted.
By: Tilghman Lesher (tilghman) 2006-04-03 18:29:35

And there we go... a patch...
By: Jeffrey C. Ollie (jcollie) 2006-04-04 07:51:03

Corydon76, your patch removes thread_safe_rand from chan_sip, but does not add ast_random anywhere...


By: Tilghman Lesher (tilghman) 2006-04-04 10:42:59

ast_random() is already defined in include/asterisk/utils.h and utils.c.
By: Tilghman Lesher (tilghman) 2006-04-04 15:20:03

I'll additionally note that ASTERISK-5563 was where that routine was originally introduced, with the note that it wasn't a permanent solution and needed a review.  ast_random() is the function that we introduced for this purpose.
By: Tilghman Lesher (tilghman) 2006-04-05 12:45:23

Committed to trunk.