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
--- 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
_______________________________________________ openssl-dev mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev