RFR: JDK-8278309: [windows] use of uninitialized OSThread::_state

[Aleksey Shipilev]

On Tue, 7 Dec 2021 06:10:20 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> 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)

This looks fine as the limited fix.

But I do wonder if the initial state `ALLOCATED` should be stamped right in the `OSThread::OSThread` constructor. Also, I see other platforms, for example, Linux does:


  // set the correct thread state
  osthread->set_thread_type(thr_type);

  // Initial state is ALLOCATED but not INITIALIZED
  osthread->set_state(ALLOCATED);


Not sure how safe it is to add `set_thread_type`, but matching the comment for `set_state` is probably in order.

-------------

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6734