Bug 29956 - Math.random() gives too low values on Win32 when _CRT_RAND_S is not defined
Summary: Math.random() gives too low values on Win32 when _CRT_RAND_S is not defined
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-01 02:40 PDT by Jonni Rainisto
Modified: 2009-10-02 16:35 PDT (History)
1 user (show)

See Also:


Attachments
Patch which fixes the bug. (1.49 KB, patch)
2009-10-01 02:49 PDT, Jonni Rainisto
darin: review-
Details | Formatted Diff | Diff
Rand() patch which uses 53bits (1.69 KB, patch)
2009-10-01 12:52 PDT, Jonni Rainisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonni Rainisto 2009-10-01 02:40:26 PDT
Windows rand() gives 15bit random number (with MSVC and MinGW), if you build webkit with MinGW or with MSVC (without _CRT_RAND_S define) then code execution goes to #else where they assume that rand() returns 31bits. 

In that case calling javascript Math.random() always gives values in range of 0.0000xxx and not [0,1]. I will attach patch where this is fixed.
Comment 1 Jonni Rainisto 2009-10-01 02:49:29 PDT
Created attachment 40430 [details]
Patch which fixes the bug.
Comment 2 Darin Adler 2009-10-01 11:06:47 PDT
Comment on attachment 40430 [details]
Patch which fixes the bug.

I think we need more bits of randomness. I'm not sure how many. It seems we are using 32 bits of randomness in the first two cases, and 53 bits in the next two. Having a Windows code path that uses only 30 bits seems wrong, but perhaps the 32-bit ones are also wrong.

review- for now based on that issue, but I could be convinced I am wrong.
Comment 3 Jonni Rainisto 2009-10-01 12:22:13 PDT
Well in weakRandomNumber its using 15bit rand() 2 times, so imho normal case (when _CRT_RAND_S is defined) in MSVC randomless is even weaker (or same) than 30bits. Im a bit worried that calling rand() 4 times to get 53bit randomness could have some performance impacts. I can make the patch to use rand 4 times, or copypaste the weak random, if 30bit random is not enough?
Comment 4 Jonni Rainisto 2009-10-01 12:52:16 PDT
Created attachment 40473 [details]
Rand() patch which uses 53bits
Comment 5 WebKit Commit Bot 2009-10-02 16:35:38 PDT
Comment on attachment 40473 [details]
Rand() patch which uses 53bits

Clearing flags on attachment: 40473

Committed r49056: <http://trac.webkit.org/changeset/49056>
Comment 6 WebKit Commit Bot 2009-10-02 16:35:42 PDT
All reviewed patches have been landed.  Closing bug.