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

[David Holmes]

Hi Anton,

On 3/03/2020 1:12 am, Anton Tarasov wrote:
> Hi David,
> 
> Thank you for fixing this! Is it possible to backport the fix to jdk11?

I can tag it for backporting once it is pushed, but I can't do the 
actual backport for OpenJDK 11u.

David

> Thanks,
> Anton.
> 
> On Sat, Feb 29, 2020 at 8:16 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     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
>      >>
>