On 7/6/16 10:57 AM, Ivan Gerasimov wrote:
Hello!
When fixing JDK-8069048 a potential race was overlooked:
1) Thread A wants to call _endthreadex() and registers itself,
2) Thread B wants to call exit(), takes its turn and raises the flag
`process_exiting`,
3) Thread A continues, and gets captured in the loop at 4020, in
SuspendThread(GetCurrentThread()),
4) Now, the thread B will have to wait for all the registered threads,
including the thread A, which will never wake up.
Would you please help review the fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8160892
WEBREV: http://cr.openjdk.java.net/~igerasim/8160892/00/webrev/
src/os/windows/vm/os_windows.cpp
Just to make sure the race is clear (in my mind at least):
Thread A
- executes if-block at L3913-3968; in particular it
registers itself on L3957
- executes if-block at L4017-4027; in the old code,
the thread would block for ever in L4022-4026; in
the new code, the thread will only block if it did
not register itself.
Thread B:
- executes if-block at L3906-3910 and succeeds in
setting the process_exiting flag
- executes if-block at L3969-4012; in particular
Thread B calls WaitForMultipleObjects() on L3994;
in the old code, WaitForMultipleObjects() times
out because Thread A is blocked.
L3963: registered_itself = true;
L3964: }
L3965:
L3966: // The current exiting thread has stored its handle
in the array, and now
L3967: // should leave the critical section before calling
_endthreadex().
The existing comment on L3966-3967 belongs after L3963 and before
L3964, i.e., at the end of the else-block.
If you agree with moving the comment on L3966-3967, then
consider this next request:
L3959: warning("DuplicateHandle failed (%u) in %s: %d\n",
L3960: GetLastError(), __FILE__, __LINE__);
Please consider adding the following comment after the warning:
// We can't register this thread (no more handles) so this thread
// may be racing with a thread that is calling exit(). If the
thread
// that is calling exit() has managed to set the process_exiting
// flag, then this thread will be caught in the SuspendThread()
// infinite loop below which closes that race. A small timing
// window remains before the process_exiting flag is set, but it
// is only exposed when we are out of handles.
L4021: // don't let the current thread proceed to exit() or
_endthreadex()
Clarify comment: 'current thread'
-> 'current unregistered thread'
Very nice and very quick job in finding this race!
Thumbs up on what you have since my comments are only related
to moving/adding comments.
Dan
With kind regards,
Ivan
