Summary: | ASTERISK-06690: [patch] Poor use of rand() scaling | ||
Reporter: | Alan Ferrency (ferrency) | Labels: | |
Date Opened: | 2006-04-03 15:18:22 | Date Closed: | 2006-04-05 12:45:23 |
Priority: | Trivial | Regression? | No |
Status: | Closed/Complete | Components: | 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. |