Hello!
I was just browsing the code and found a couple of minor problems...
On Windows, when CryptGenRandom has failed for whatever reason, a variable
named stoptime is initialized to 0 (since good == 0 in that case). This
means that the followup tests (which are present in a number of loops)
"GetTickCount() < stoptime" will always return 0 and terminate the loops
prematurely (after one iteration).
Also, there's a problem for the case where CyyptGenRandom has succeeded and
GetTickCount() is just about to wrap. In that case, all the loops will also
terminate too early (again, after one iteration).
Why can people not learn to always use subtraction and signed comparison
against zero when dealing with wrapping timers?
The attached patch tries to fix the above problems so that the loops are
allowed to run for much longer when CryptGenRandom has failed and so that
the loops are allowed to always run for MAXDELAY (1000) milliseconds when
CryptGenRandom has succeded.
LONG and DWORD are always 32 bits on Windows, even on Win64. At least I'm
convinced about that...
I haven't compile-tested this patch, sorry. But you wouldn't just commit it
anyway, right? :-)
Cheers,
Peter
Index: crypto/rand/rand_win.c
===================================================================
RCS file: /v/openssl/cvs/openssl/crypto/rand/rand_win.c,v
retrieving revision 1.45
diff -u -r1.45 rand_win.c
--- crypto/rand/rand_win.c 13 Oct 2005 19:06:43 -0000 1.45
+++ crypto/rand/rand_win.c 22 May 2008 10:51:38 -0000
@@ -122,7 +122,7 @@
#include <tlhelp32.h>
/* Limit the time spent walking through the heap, processes, threads and
modules to
- a maximum of 1000 miliseconds each, unless CryptoGenRandom failed */
+ a maximum of 1000 milliseconds each, unless CryptoGenRandom failed */
#define MAXDELAY 1000
/* Intel hardware RNG CSP -- available from
@@ -463,7 +463,7 @@
PROCESSENTRY32 p;
THREADENTRY32 t;
MODULEENTRY32 m;
- DWORD stoptime = 0;
+ DWORD stoptime;
snap = (CREATETOOLHELP32SNAPSHOT)
GetProcAddress(kernel, "CreateToolhelp32Snapshot");
@@ -495,7 +495,8 @@
* of entropy.
*/
hlist.dwSize = sizeof(HEAPLIST32);
- if (good) stoptime = GetTickCount() + MAXDELAY;
+ stoptime = GetTickCount() +
+ (good ? MAXDELAY : 0x7fffffff);
if (heaplist_first(handle, &hlist))
do
{
@@ -512,8 +513,8 @@
while (heap_next(&hentry)
&& --entrycnt > 0);
}
- } while (heaplist_next(handle,
- &hlist) && GetTickCount() <
stoptime);
+ } while (heaplist_next(handle, &hlist)
&&
+ (LONG)(GetTickCount() -
stoptime) < 0);
/* process walking */
/* PROCESSENTRY32 contains 9 fields that will change
@@ -522,11 +523,14 @@
*/
p.dwSize = sizeof(PROCESSENTRY32);
- if (good) stoptime = GetTickCount() + MAXDELAY;
+ stoptime = GetTickCount() +
+ (good ? MAXDELAY : 0x7fffffff);
if (process_first(handle, &p))
do
RAND_add(&p, p.dwSize, 9);
- while (process_next(handle, &p) &&
GetTickCount() < stoptime);
+ while (process_next(handle, &p) &&
+ (LONG)(GetTickCount() - stoptime) < 0);
+
/* thread walking */
/* THREADENTRY32 contains 6 fields that will change
@@ -534,11 +538,13 @@
* 1 byte of entropy.
*/
t.dwSize = sizeof(THREADENTRY32);
- if (good) stoptime = GetTickCount() + MAXDELAY;
+ stoptime = GetTickCount() +
+ (good ? MAXDELAY : 0x7fffffff);
if (thread_first(handle, &t))
do
RAND_add(&t, t.dwSize, 6);
- while (thread_next(handle, &t) &&
GetTickCount() < stoptime);
+ while (thread_next(handle, &t) &&
+ (LONG)(GetTickCount() -
stoptime) < 0);
/* module walking */
/* MODULEENTRY32 contains 9 fields that will change
@@ -546,12 +552,13 @@
* 1 byte of entropy.
*/
m.dwSize = sizeof(MODULEENTRY32);
- if (good) stoptime = GetTickCount() + MAXDELAY;
+ stoptime = GetTickCount() +
+ (good ? MAXDELAY : 0x7fffffff);
if (module_first(handle, &m))
do
RAND_add(&m, m.dwSize, 9);
- while (module_next(handle, &m)
- && (GetTickCount() < stoptime));
+ while (module_next(handle, &m) &&
+ (LONG)(GetTickCount() -
stoptime) < 0);
if (close_snap)
close_snap(handle);
else
