Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Random.self_init problem under Windows #6032

Closed
vicuna opened this issue Jun 5, 2013 · 7 comments
Closed

Random.self_init problem under Windows #6032

vicuna opened this issue Jun 5, 2013 · 7 comments
Assignees

Comments

@vicuna
Copy link

@vicuna vicuna commented Jun 5, 2013

Original bug ID: 6032
Reporter: @alainfrisch
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2015-12-11T18:19:30Z)
Resolution: fixed
Priority: normal
Severity: minor
Category: standard library
Monitored by: @alainfrisch

Bug description

Under Windows, the random seed is computed from the process id and the current time. As a result, the following will (with high probability) display twice the same number:

let () =
Random.self_init ();
Printf.printf "%i\n%!" (Random.int 10000);
Random.self_init ();
Printf.printf "%i\n%!" (Random.int 10000)

The code for caml_win32_random_seed (in win32.c) suggests an improvement (using RtlGenRandom). A simpler fix for the case above could be obtained simply by using a static counter (so that the same process calling self_init twice in a row will get different seeds). I want to check that nobody is against this fix.

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 5, 2013

Comment author: @xavierleroy

The static counter isn't going to add any entropy, even though it would make the problem less apparent. I'd prefer to look for other sources of randomness: if not the Win32 crypto API, at least something like the TSC register of x86 processors.

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 5, 2013

Comment author: @xavierleroy

I'd be tempted to use QueryPerformanceCounter, which looks reasonably portable under Win32:

int caml_win32_random_seed (intnat data[16])
{
FILETIME t;
LARGE_INTEGER pc;
GetSystemTimeAsFileTime(&t);
QueryPerformanceCounter(&pc);
data[0] = t.dwLowDateTime;
data[1] = t.dwHighDateTime;
data[2] = GetCurrentProcessId();
data[3] = pc.LowPart;
data[4] = pc.HighPart;
return 5;
}

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 5, 2013

Comment author: @alainfrisch

What about using:

http://blogs.msdn.com/b/michael_howard/archive/2005/01/14/353379.aspx

?

maybe with a fallback (for the unlikely case where the symbol is not found in the dll or the dll cannot be loaded) to your solution.

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 5, 2013

Comment author: @xavierleroy

Well, using RtlGenRandom is what the comment in win32.c suggests. However, the MSDN doc is scary: "[this function] may be altered or unavailable in subsequent versions [of Windows]". CryptGenRandom is certainly more durable, but risks pulling in all of the Win32 Crypto API.

Another suspicious point: truly cryptography-quality PRNGs block until enough entropy has been gathered. (That's the difference between /dev/random and /dev/urandom under Linux.) We don't want to risk blocking while initializing OCaml's Random library module. The MSDN docs don't say whether RtlGenRandom and CryptGenRandom can block, and how to avoid blocking.

Could you be so nice as to try the QueryPerformanceCounter-based solution I suggested? (I no longer have a working Win32 development environment.) Thanks in advance.

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 6, 2013

Comment author: @alainfrisch

Could you be so nice as to try the QueryPerformanceCounter-based solution I suggested? (I no longer have a working Win32 development environment.) Thanks in advance.

Yes, it seems to work (tried with the 32-bit msvc port).

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 6, 2013

Comment author: @alainfrisch

Commit 13750 to trunk.

@vicuna
Copy link
Author

@vicuna vicuna commented Jun 6, 2013

Comment author: @alainfrisch

Commit 13751 adds a non-regression test (which might fail with very low probability).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants