Bumped and fed to RT as this bugger doesn't seem to have a ticket # while the fix is available, and I just got an enquiry about this and whether did it make it into the latest. (Nope, it did not. Yet?)
See also the dev@ mail trail at the subject line & time shown below for how it got to this. This is WinCE + Win32/64 relevant stuff. ---------- Forwarded message ---------- From: Ger Hobbelt <g...@hobbelt.com> Date: Wed, Mar 11, 2009 at 1:40 PM Subject: Re: RAND_poll() and CreateToolhelp32Snapshot() stability To: openssl-dev@openssl.org Would the attached patch be agreeable to both of you? It contains a fix (okay, it's a hack) to ensure OpenSSL doesn't loop infinitely on crashing Heap32Next, so that should at least cover the (theoretical?) issue of arbitrary/unknown fault origin from within Heap32Next. Then there's the fun part about the argument of not using it in a multithread app when it's crashing on WinCE: well, this particular bit of code is part of the entropy harvester, which has some different demands, compared to 'regular application code' (compare the Debian screwup from last year), so my position is this: as long as the crash is controlled (and it is, thanks to the code posted by Tanguy) and it's cause is generally known (it's documented in MSDN for one of the Microsoft platforms), it's good enough as a partial source of entropy: after all, we do NOT depend on the data [structures] returned, all we are looking for here, is _entropy_. (And philosophically, is a corrupt data structure bad from an entropy perspective? Nope. Not at all.) Thus, this whole issue reduces to the question: do we can uncontrolled core dumps from using this? Answer: no, we don't. Can we get infinite loops, assuming worst case scenario of total heap API failure? Maybe. So we fix that by limiting the number of crash failures we tolerate here. (And, yes, I know, _theoretically_, a corrupt heap [API] can deliver seemingly 'good' structure indefinitely. Heck, same fix: limit the number of _total_ rounds instead of the number of failures and we're good to go. (move the ex_cnt_limit decrement to the while condition instead, then.) Meanwhile, this is a definite software improvement for CE, so I'd really like to have it in there.) (Attached diff against latest 0.9.9 CVS HEAD) -- Met vriendelijke groeten / Best regards, Ger Hobbelt -------------------------------------------------- web: http://www.hobbelt.com/ http://www.hebbut.net/ mail: g...@hobbelt.com mobile: +31-6-11 120 978 --------------------------------------------------
--- /home/ger/prj/1original/openssl/openssl/./crypto/rand/rand_win.c 2005-10-13 21:06:43.000000000 +0200 +++ ./crypto/rand/rand_win.c 2009-03-11 12:25:50.000000000 +0100 @@ -494,12 +496,28 @@ * each entry. Consider each field a source of 1 byte * of entropy. */ + ZeroMemory(&hlist, sizeof(HEAPLIST32)); hlist.dwSize = sizeof(HEAPLIST32); if (good) stoptime = GetTickCount() + MAXDELAY; if (heaplist_first(handle, &hlist)) + { + /* + following discussion on dev ML, exception on WinCE (or other Win + platform) is theoretically of unknown origin; prevent infinite + loop here when this theoretical case occurs; otherwise cope with + the expected (MSDN documented) exception-throwing behaviour of + Heap32Next() on WinCE. + + based on patch in original message by Tanguy Fautré (2009/03/02) + Subject: RAND_poll() and CreateToolhelp32Snapshot() stability + */ + int ex_cnt_limit = 42; do { RAND_add(&hlist, hlist.dwSize, 3); + __try + { + ZeroMemory(&hentry, sizeof(HEAPENTRY32)); hentry.dwSize = sizeof(HEAPENTRY32); if (heap_first(&hentry, hlist.th32ProcessID, @@ -512,8 +530,16 @@ while (heap_next(&hentry) && --entrycnt > 0); } - } while (heaplist_next(handle, - &hlist) && GetTickCount() < stoptime); + } + __except (EXCEPTION_EXECUTE_HANDLER) + { + /* ignore access violations when walking the heap list */ + ex_cnt_limit--; + } + } while (heaplist_next(handle, &hlist) + && GetTickCount() < stoptime + && ex_cnt_limit > 0); + } /* process walking */ /* PROCESSENTRY32 contains 9 fields that will change