[openssl-dev] [PATCH] to fix hang in RAND_poll on Windows 7 / Server 2008R2 and other performance problems
Adam Walling
adam.walling at gmail.com
Tue Jul 28 20:02:31 UTC 2015
When Heap32First is called while RtlAllocateHeap is executing on
another thread, the process can deadlock. The underlying bug has a
hotfix from Microsoft, but is not part of a service pack. Only Windows
7 or 2008R2 are affected:
https://support.microsoft.com/en-us/kb/2719306
Deadlock noted in various places, such as:
http://rt.openssl.org/Ticket/Display.html?id=2485&user=guest&pass=guest
http://comments.gmane.org/gmane.comp.encryption.openssl.devel/20440
And the performance was discussed and had workarounds as noted here:
http://rt.openssl.org/Ticket/Display.html?id=2100&user=guest&pass=guest
The discussion here:
http://openssl.6102.n7.nabble.com/Deadlock-in-RAND-poll-s-Heap32First-call-td13043.html
proposed a similar solution to this, but discussion diverged from the
actual problem.
While using the heap as a source of entropy is debatable, we can still
fix this issue while also avoiding the performance issue that was
already patched with the workaround.
In the existing code, the process ID of 0 is passed to
CreateToolhelp32Snapshot, so it only iterates the heaps of the current
process. This patch uses HeapWalk to implement the same behavior in a
safer manner; toolhelp is still used to iterate the modules, threads,
processes for entropy. HeapWalk and other functions used by this patch
are available since NT 3.51, though they are dynamically loaded
anyway.
Most of the time, a windows process will only have or use one heap,
but sometimes private heaps are used. However they cannot really be
traversed safely since some may be created without serialization. The
attached version patch will only use the main heap returned by
GetProcessHeap since this is a safe operation.
If there is only one heap, then this version matches the entropy
provided by the existing implementation without the performance
implications or deadlock / hang. If there is interest, I also have a
version of this patch that iterates all heaps of the process in
addition to the main heap (GetProcessHeap). However I am uncertain if
the extra motes of entropy that those private heaps provide are worth
potentially destabilizing the process.
I did my best to maintain style using the OpenSSL style guide and
'when-in-rome' guidelines; for example there was a pre-existing mix of
tabs and spaces for indentation, so I kept them where it already
existed.
I've been encountering this quite often on some machines that can't
realistically be updated, and the patch works very well for both
0.9.8gz and 1.0.1h.
The only changes are rand_win.c -- I've attached the unified diff for
1.0.1h, though the differences in the implementation of this change
between 1.0.1h and 0.9.8zg are cosmetic only. I also have a diff for
0.9.8zg, as well as the implementation of walking all heaps for both
of these release branches as well, but I'd rather not pollute this
list with mostly-redundant patches.
If preferred, I can create a pull request, but it seems that
submitting simpler patches to the mailing list is suggested initial
approach.
Thanks,
--
- Adam D. Walling
-------------- next part --------------
--- rand_win.c.original 2014-06-05 05:41:30.000000000 -0400
+++ rand_win.c 2015-07-28 12:28:43.393846200 -0400
@@ -168,13 +168,15 @@
typedef HANDLE (WINAPI *CREATETOOLHELP32SNAPSHOT)(DWORD, DWORD);
typedef BOOL (WINAPI *CLOSETOOLHELP32SNAPSHOT)(HANDLE);
-typedef BOOL (WINAPI *HEAP32FIRST)(LPHEAPENTRY32, DWORD, size_t);
-typedef BOOL (WINAPI *HEAP32NEXT)(LPHEAPENTRY32);
-typedef BOOL (WINAPI *HEAP32LIST)(HANDLE, LPHEAPLIST32);
typedef BOOL (WINAPI *PROCESS32)(HANDLE, LPPROCESSENTRY32);
typedef BOOL (WINAPI *THREAD32)(HANDLE, LPTHREADENTRY32);
typedef BOOL (WINAPI *MODULE32)(HANDLE, LPMODULEENTRY32);
+typedef BOOL(WINAPI *HEAPWALK) (HANDLE, LPPROCESS_HEAP_ENTRY);
+typedef BOOL(WINAPI *HEAPLOCK) (HANDLE);
+typedef BOOL(WINAPI *HEAPUNLOCK) (HANDLE);
+typedef HANDLE(WINAPI *GETPROCESSHEAP) (VOID);
+
#include <lmcons.h>
#include <lmstats.h>
#if 1 /* The NET API is Unicode only. It requires the use of the UNICODE
@@ -432,7 +434,67 @@
FreeLibrary(user);
}
- /* Toolhelp32 snapshot: enumerate processes, threads, modules and heap
+ if (kernel) {
+ GETPROCESSHEAP get_process_heap;
+ HEAPWALK heap_walk;
+ HEAPLOCK heap_lock;
+ HEAPUNLOCK heap_unlock;
+
+ HANDLE heap;
+
+ DWORD starttime = 0;
+
+ // HeapWalk et al available as of NT 3.51
+ get_process_heap = (GETPROCESSHEAP)
+ GetProcAddress(kernel, "GetProcessHeap");
+ heap_walk = (HEAPWALK) GetProcAddress(kernel, "HeapWalk");
+ heap_lock = (HEAPLOCK) GetProcAddress(kernel, "HeapLock");
+ heap_unlock = (HEAPUNLOCK) GetProcAddress(kernel, "HeapUnlock");
+
+ if (get_process_heap && heap_walk && heap_lock && heap_unlock) {
+
+ if (good)
+ starttime = GetTickCount();
+
+ HANDLE heap;
+ PROCESS_HEAP_ENTRY hentry;
+
+ heap = get_process_heap();
+
+ ZeroMemory(&hentry, sizeof(PROCESS_HEAP_ENTRY));
+ int entrycnt = 80;
+
+ heap_lock(heap);
+# ifdef _MSC_VER
+ __try {
+# endif
+ while (heap_walk(heap, &hentry)
+ && (!good
+ || (GetTickCount() - starttime) <
+ MAXDELAY)
+ && --entrycnt > 0)
+ {
+ /* PROCESS_HEAP_ENTRY has 5 fields that change with
+ * each entry. Consider each field a source of 1 byte
+ * of entropy.
+ */
+ RAND_add(&hentry, sizeof(PROCESS_HEAP_ENTRY), 5);
+ }
+# ifdef _MSC_VER
+ }
+ __except(EXCEPTION_EXECUTE_HANDLER) {
+ /*
+ * ignore access violations when walking the heap
+ * list
+ */
+ }
+# endif
+ heap_unlock(heap);
+ }
+ }
+
+
+ /* Toolhelp32 snapshot: enumerate processes, threads, modules and heap
* http://msdn.microsoft.com/library/psdk/winbase/toolhelp_5pfd.htm
* (Win 9x and 2000 only, not available on NT)
*
@@ -451,15 +513,10 @@
CLOSETOOLHELP32SNAPSHOT close_snap;
HANDLE handle;
- HEAP32FIRST heap_first;
- HEAP32NEXT heap_next;
- HEAP32LIST heaplist_first, heaplist_next;
PROCESS32 process_first, process_next;
THREAD32 thread_first, thread_next;
MODULE32 module_first, module_next;
- HEAPLIST32 hlist;
- HEAPENTRY32 hentry;
PROCESSENTRY32 p;
THREADENTRY32 t;
MODULEENTRY32 m;
@@ -469,10 +526,6 @@
GetProcAddress(kernel, "CreateToolhelp32Snapshot");
close_snap = (CLOSETOOLHELP32SNAPSHOT)
GetProcAddress(kernel, "CloseToolhelp32Snapshot");
- heap_first = (HEAP32FIRST) GetProcAddress(kernel, "Heap32First");
- heap_next = (HEAP32NEXT) GetProcAddress(kernel, "Heap32Next");
- heaplist_first = (HEAP32LIST) GetProcAddress(kernel, "Heap32ListFirst");
- heaplist_next = (HEAP32LIST) GetProcAddress(kernel, "Heap32ListNext");
process_first = (PROCESS32) GetProcAddress(kernel, "Process32First");
process_next = (PROCESS32) GetProcAddress(kernel, "Process32Next");
thread_first = (THREAD32) GetProcAddress(kernel, "Thread32First");
@@ -480,90 +533,15 @@
module_first = (MODULE32) GetProcAddress(kernel, "Module32First");
module_next = (MODULE32) GetProcAddress(kernel, "Module32Next");
- if (snap && heap_first && heap_next && heaplist_first &&
- heaplist_next && process_first && process_next &&
+ if (snap && process_first && process_next &&
thread_first && thread_next && module_first &&
- module_next && (handle = snap(TH32CS_SNAPALL,0))
- != INVALID_HANDLE_VALUE)
+ module_next && (handle = snap(
+ TH32CS_SNAPMODULE
+ | TH32CS_SNAPPROCESS
+ | TH32CS_SNAPTHREAD
+ , 0))
+ != INVALID_HANDLE_VALUE)
{
- /* heap list and heap walking */
- /* HEAPLIST32 contains 3 fields that will change with
- * each entry. Consider each field a source of 1 byte
- * of entropy.
- * HEAPENTRY32 contains 5 fields that will change with
- * each entry. Consider each field a source of 1 byte
- * of entropy.
- */
- ZeroMemory(&hlist, sizeof(HEAPLIST32));
- hlist.dwSize = sizeof(HEAPLIST32);
- if (good) starttime = GetTickCount();
-#ifdef _MSC_VER
- 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,
- hlist.th32HeapID))
- {
- int entrycnt = 80;
- do
- RAND_add(&hentry,
- hentry.dwSize, 5);
- while (heap_next(&hentry)
- && (!good || (GetTickCount()-starttime)<MAXDELAY)
- && --entrycnt > 0);
- }
- }
- __except (EXCEPTION_EXECUTE_HANDLER)
- {
- /* ignore access violations when walking the heap list */
- ex_cnt_limit--;
- }
- } while (heaplist_next(handle, &hlist)
- && (!good || (GetTickCount()-starttime)<MAXDELAY)
- && ex_cnt_limit > 0);
- }
-
-#else
- if (heaplist_first(handle, &hlist))
- {
- do
- {
- RAND_add(&hlist, hlist.dwSize, 3);
- hentry.dwSize = sizeof(HEAPENTRY32);
- if (heap_first(&hentry,
- hlist.th32ProcessID,
- hlist.th32HeapID))
- {
- int entrycnt = 80;
- do
- RAND_add(&hentry,
- hentry.dwSize, 5);
- while (heap_next(&hentry)
- && --entrycnt > 0);
- }
- } while (heaplist_next(handle, &hlist)
- && (!good || (GetTickCount()-starttime)<MAXDELAY));
- }
-#endif
-
/* process walking */
/* PROCESSENTRY32 contains 9 fields that will change
* with each entry. Consider each field a source of
More information about the openssl-dev
mailing list