RFR: JDK-8278309: [windows] use of uninitialized OSThread::_state
Thomas Stuefe
stuefe at openjdk.java.net
Tue Dec 7 09:07:33 UTC 2021
May I have reviews for this trivial fix.
On Windows, we use `OSThread::_state` in `os::create_thread` before it has been initialized. This causes an assert to fire in `Thread::is_JavaThread_protected` (`assert(target->is_handshake_safe_for(current_thread)`)
This only happens if the following is true:
- We log os=info level, thereby firing the "Thread started.." log output the parent thread of a newly started child thread writes. Since JDK-8268773, we also print the thread name. `Thread::name()` uses `Thread::is_JavaThread_protected`, but on Windows, the thread state has not been set yet.
- This is an assert, so only debug, but in debug newly malloced memory is poisoned with "F1F1F1F1...", which hides the error since `Thread::is_JavaThread_protected` compares the thread state like this:
if (target->osthread() == NULL || target->osthread()->get_state() <= INITIALIZED) {
return true;
}
and the compiler interprets the "F1F1F1F1" as a negative large value. Changing the init pattern to 0x01, or adding an explicit cast to unsigned, causes the assert to fire as soon as logging is switched on.
---
Musings:
I wondered whether we should change the thread state comparison to unsigned like this:
if (target->osthread() == NULL || (unsigned)target->osthread()->get_state() <= INITIALIZED) {
which would have shown the error right away after JDK-8268773. This is matter of taste though since one could say that at this point we expect the enum to be filled correctly with one of its values, and guarding against uninitialized memory should belong somewhere else, maybe in a debug-only verify function.
--
Just my personal opinion, but `OSThread` could do with a bit of cleanup. E.g. using an initializer list to init its values, and possibly doing the per-platform-factoring out in a different way. That include-file-in-the-middle-of-class composition technique is terrible. It confuses IDEs and makes analyzing the code difficult, and the code is not really difficult in what it does.
----
Tests: GHAs (twice, once with experimentally set init word to 1 to see that the patch works)
-------------
Commit messages:
- initialize osthread state on windows
Changes: https://git.openjdk.java.net/jdk/pull/6734/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=6734&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8278309
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/6734.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/6734/head:pull/6734
PR: https://git.openjdk.java.net/jdk/pull/6734
More information about the hotspot-dev
mailing list