RFR (S): 8238676: jni crashes on accessing it from process exit hook

[David Holmes]

Hi Gerard,

Are you okay with this?

Thanks,
David

On 26/02/2020 10:11 am, David Holmes wrote:
> Hi Gerard,
> 
> Thanks for taking a look at this.
> 
> On 26/02/2020 2:45 am, gerard ziemski wrote:
>> hi David,
>>
>> On 2/18/20 8:04 PM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8238676
>>> webrev: http://cr.openjdk.java.net/~dholmes/8238676/webrev/
>>>
>>> If an application tries to use JNI from an atexit handler, the 
>>> attempt can't succeed (the VM has already logically terminated) but 
>>> it should not crash. The JNI Invocation API code was missing some 
>>> checks in places and wasn't aware of the possibility of trying to 
>>> make calls from the VMThread.
>>>
>>> Testing:
>>>  - new test added for the JNI Invocation API
>>>  - tiers 1-3
>>>
>>> Thanks,
>>> David
>>
>> In jni_GetEnv() we have:
>>
>> 4120   if (vm_created == 0) {
>> 4121     *penv = NULL;
>> 4122     ret = JNI_EDETACHED;
>> 4123     return ret;
>> 4124   }
>>
>> but in jni_DetachCurrentThread() we have:
>>
>> 4063   if (vm_created == 0) {
>> 4064     HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_ERR);
>> 4065     return JNI_ERR;
>> 4066   }
>>
>> should lines 4064,4065 perhaps be:
>>
>> 4064     HOTSPOT_JNI_DETACHCURRENTTHREAD_RETURN(JNI_EDETACHED);
>> 4065     return JNI_EDETACHED;
>>
>> to be consistent?
> 
> Well ... ideally the JNI spec would have considered the question of 
> "what if the JavaVM is no longer live?" and had an JNI_ENOJVM errro 
> code. But it doesn't. Nor does it allow for any JNI method to return 
> JNI_ERR for unexpected errors situations. So we have to adhere to the 
> specifications for each method.
> 
> For GetEnv the spec only allows for returning JNI_EDETACHED, 
> JNI_EVERSION or JNI_OK. As you can't be attached to a VM that doesn't 
> exist then JNI_EDETACHED is the only possible return code - and it isn't 
> wrong, it just doesn't tell you why you aren't attached.
> 
> If you apply the same logic to DetachCurrentThread, and follow the spec 
> then it states
> 
> "Trying to detach a thread that is not attached is a no-op."
> 
> which suggests we do nothing and report JNI_OK. But I think that would 
> be a disservice to the programmer in the case where the JVM is no longer 
> live. And we're allowed to return "a suitable JNI error code on failure" 
> so that is what I chose to do. And I return JNI_ERR rather than 
> JNI_EDETACHED for two reasons:
> 
> 1. It is consistent with what we do for attach in the same circumstance:
> 
> jint JNICALL jni_AttachCurrentThread(JavaVM *vm, void **penv, void 
> *_args) {
>    HOTSPOT_JNI_ATTACHCURRENTTHREAD_ENTRY(vm, penv, _args);
>    if (vm_created == 0) {
>      HOTSPOT_JNI_ATTACHCURRENTTHREAD_RETURN((uint32_t) JNI_ERR);
>      return JNI_ERR;
>    }
> 
> 2. It would be confusing to return JNI_EDETACHED as that means "thread 
> detached from the VM" yet the spec says "Trying to detach a thread that 
> is not attached is a no-op.". So I need to convey that there is a more 
> specific underlying error here.
> 
>> Looks good otherwise.
> 
> Thanks for the review.
> 
> David
> -----
> 
>>
>> cheers
>>