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

[David Holmes]

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
>