RFR: 8303624: The java.lang.Thread.FieldHolder can be null for JNI attaching threads

[David Holmes]

On Tue, 7 Mar 2023 12:06:27 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> To support virtual threads a number of fields were moved out of `java.lang.Thread` into a separate `FieldHolder` object. The VM was updated to then access certain thread fields via the `FieldHolder`. 
>> 
>> The code for attaching a thread to the VM, specifically `allocate_threadObj` didn't allow for the `Thread` constructor throwing an exception, and so failing to allocate the `FieldHolder` before attempting to access a field through the `FieldHolder`. This resulted in assertion failures in `javaClasses.cpp` (or crashes in a product build). That code is fixed to ensure we cease processing if the constructor throws an exception.
>> 
>> In addition, we need to recognise that whilst a native thread is attaching via JNI, it is partially initialized (to varying degrees) but also visible through JVMTI. Though the window is small JVMTI could get hold of an attaching thread and then invoke methods that would try to access uninitialized state. When this was state of `Thread` instance there was no problem as the object existed in its zero-initialized form (it had been allocated directly but no constructor run). However, anythng accessed via the `FieldHolder` is now a problem as the `FieldHolder` may not exist. So we modify all of the `FieldHolder` get/set methods to account for it being null: setters will do nothing, while getters return the default zero value for the field.
>> 
>> Testing: tiers 1-3 as a sanity test
>> 
>> There's no way to write a regression test for this.
>> 
>> Thanks.
>
> If I read this correctly, a JNI attaching thread has its JavaThread on the threads list, the j.l.Thread object is allocated but not yet initialized or the initialization fails (in both cases, holder is null). If a JVMTI agent gets a reference to the thread at this point (GetAllThreads, instrumented the constructor, several other ways) then functions such as GetThreadInfo will attempt to access fields and crash.
> 
> The changes look okay to me except for SET_FIELDHOLDER_FIELD being a no-op when holder is null. It should be okay for a JVMTI agent to read a default value but if set_priority,  set_daemon, or set_thread_status can be being called when the initialization fails then it suggests there the error handling might isn't complete somewhere.

@AlanBateman thanks for looking at this. Note that there are no API's for setting the daemon status once the thread is alive (which it is) nor directly changing the thread's status (that can only be done via actions of the thread itself). That only leaves an API call to set the priority as a potential oddity. But I don't see what we can do about it - there is no place to set the priority now it is in the not-yet-existing FieldHolder. Also any such change of priority is either racing with a successful completion of the thread constructor, which will also set priority, or racing with a failed attach in which case the priority value is irrelevant. 

It is a general flaw that JVMTI allows you to get a hold of a partially initialized thread, and race with the initialization of that thread (the spec says nothing about this possibility - nor should it). So trying to reason what is correct behaviour in such circumstances is very hard.

> it suggests there the error handling might isn't complete somewhere

I don't understand what you mean by this.

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

PR: https://git.openjdk.org/jdk/pull/12892