Mark JavaVM::get_env unsafe and take version argument

JavaVM::get_env now requires the caller to determine that the JavaVM
supports at least JNI 1.4 since there are two notable difficulties
with trying to check the version internally:

1. There is a chicken and egg problem with trying to use `GetEnv`
   to get a `JNIEnv` from a `JavaVM` without knowing that it supports JNI
   1.2
2. We require 1.4 as the minimum supported version but can't call
   `GetVersion` to validate a returned `JNIEnv` while there may be exceptions
   pending (since GetVersion is not in the list of JNI functions that are safe
   to call with pending exceptions).

This also documents how `::get_env()` must not be used to materialize
a (mutable) JNIEnv with a lifetime that doesn't correspond to the top JNI
stack frame for local object references.

Ref: #436 (reply in thread)

Fixes: #500

src/wrapper/executor.rs

@@ -73,7 +73,7 @@ impl Executor {
     {
         assert!(capacity > 0, "capacity should be a positive integer");
 
-        let mut jni_env = self.vm.attach_current_thread_as_daemon()?;
+        let mut jni_env = self.vm.attach_current_thread_permanently()?;
         jni_env.with_local_frame(capacity, |jni_env| f(jni_env))
     }
 

src/wrapper/java_vm/vm.rs

@@ -8,7 +8,7 @@ use std::{
 
 use log::{debug, error};
 
-use crate::{errors::*, sys, JNIEnv};
+use crate::{errors::*, sys, JNIEnv, JNIVersion};
 
 #[cfg(feature = "invocation")]
 use {
@@ -260,9 +260,12 @@ impl JavaVM {
     /// [block]: https://docs.oracle.com/en/java/javase/12/docs/specs/jni/invocation.html#unloading-the-vm
     /// [attach-as-daemon]: struct.JavaVM.html#method.attach_current_thread_as_daemon
     pub fn attach_current_thread_permanently(&self) -> Result<JNIEnv> {
-        match self.get_env() {
-            Ok(env) => Ok(env),
-            Err(_) => self.attach_current_thread_impl(ThreadType::Normal),
+        // Safety: NOT SAFE CURRENTLY: https://github.com/jni-rs/jni-rs/discussions/436#discussioncomment-5421738
+        unsafe {
+            match self.get_env(JNIVersion::V1_4) {
+                Ok(env) => Ok(env),
+                Err(_) => self.attach_current_thread_impl(ThreadType::Normal),
+            }
         }
     }
 
@@ -280,11 +283,14 @@ impl JavaVM {
     /// [block]: https://docs.oracle.com/en/java/javase/12/docs/specs/jni/invocation.html#unloading-the-vm
     /// [attach-as-daemon]: struct.JavaVM.html#method.attach_current_thread_as_daemon
     pub fn attach_current_thread(&self) -> Result<AttachGuard> {
-        match self.get_env() {
-            Ok(env) => Ok(AttachGuard::new_nested(env)),
-            Err(_) => {
-                let env = self.attach_current_thread_impl(ThreadType::Normal)?;
-                Ok(AttachGuard::new(env))
+        // Safety: NOT SAFE CURRENTLY: https://github.com/jni-rs/jni-rs/discussions/436#discussioncomment-5421738
+        unsafe {
+            match self.get_env(JNIVersion::V1_4) {
+                Ok(env) => Ok(AttachGuard::new_nested(env)),
+                Err(_) => {
+                    let env = self.attach_current_thread_impl(ThreadType::Normal)?;
+                    Ok(AttachGuard::new(env))
+                }
             }
         }
     }
@@ -332,8 +338,19 @@ impl JavaVM {
     /// that is already attached is a no-op, and will not change its status to a daemon thread.
     ///
     /// The thread will detach itself automatically when it exits.
-    pub fn attach_current_thread_as_daemon(&self) -> Result<JNIEnv> {
-        match self.get_env() {
+    ///
+    /// # Safety
+    ///
+    /// The use of daemon threads is only relevant in applications that might later try to
+    /// call [`JavaVM::destroy()`] and it would be inherently unsound to allow any Rust code
+    /// to continue beyond [`JavaVM::destroy()`] if it could possibly attempt to access
+    /// the destroyed [`JavaVM`].
+    ///
+    /// This API is so unsafe to consider for its intended purpose that it will
+    /// likely be removed from this crate, in favor of relegating the
+    /// functionality to the `jni-sys` crate instead.
+    pub unsafe fn attach_current_thread_as_daemon(&self) -> Result<JNIEnv> {
+        match self.get_env(JNIVersion::V1_4) {
             Ok(env) => Ok(env),
             Err(_) => self.attach_current_thread_impl(ThreadType::Daemon),
         }
@@ -349,18 +366,47 @@ impl JavaVM {
     /// Get the `JNIEnv` associated with the current thread, or
     /// `ErrorKind::Detached`
     /// if the current thread is not attached to the java VM.
-    pub fn get_env(&self) -> Result<JNIEnv> {
+    ///
+    /// You must specify what JNI `version`` you require, with a minimum of
+    /// [`JNIVersion::V1_4`]
+    ///
+    /// # Safety
+    ///
+    /// You must know that the [`JavaVM`] supports at least JNI >= 1.4
+    ///
+    /// (The implementation is not able to call `GetEnv` before 1.2 and the
+    /// implementation can't validate the `JNIEnv` version by calling
+    /// `GetVersion` if exceptions might be pending since `GetVersion` is not
+    /// documented as safe to call with pending exceptions)
+    ///
+    /// You must not use this API to materialize a [`JNIEnv`] if there is
+    /// already another [`JNIEnv`] or local [`JObject`] reference in scope,
+    /// since this risks associating a mutable [`JNIEnv`] with a `'local` stack
+    /// frame lifetime that doesn't correspond to the top of the JNI stack for
+    /// local object references.
+    ///
+    /// A [`JNIEnv`] has a lifetime parameter that ties it to a local JNI stack
+    /// frame (which holds local object references) and the safe [`JNIEnv`] API
+    /// will only allow it to remain mutable if its `'local` lifetime
+    /// corresponds to the top of the JNI stack. If you materialize a [`JNIEnv`]
+    /// with this API you will get a mutable [`JNIEnv`] and it's important you
+    /// don't inadvertantly associate the [`JNIEnv`] with a lifetime from a
+    /// pre-existing [`JObject`] that might belong to a lower stack frame.
+    ///
+    pub unsafe fn get_env(&self, version: JNIVersion) -> Result<JNIEnv> {
         let mut ptr = ptr::null_mut();
+        if version < JNIVersion::V1_4 {
+            return Err(Error::UnsupportedVersion)
+        }
         unsafe {
-            let res = java_vm_call_unchecked!(self, v1_2, GetEnv, &mut ptr, sys::JNI_VERSION_1_2);
+            let res = java_vm_call_unchecked!(self, v1_2, GetEnv, &mut ptr, version.into());
             jni_error_code_to_result(res)?;
-
-            JNIEnv::from_raw(ptr as *mut sys::JNIEnv)
+            Ok(JNIEnv::from_raw_unchecked(ptr as *mut sys::JNIEnv))
         }
     }
 
     /// Creates `InternalAttachGuard` and attaches current thread.
-    fn attach_current_thread_impl(&self, thread_type: ThreadType) -> Result<JNIEnv> {
+    unsafe fn attach_current_thread_impl(&self, thread_type: ThreadType) -> Result<JNIEnv> {
         let guard = InternalAttachGuard::new(self.clone());
         let env_ptr = unsafe {
             if thread_type == ThreadType::Daemon {

src/wrapper/objects/global_ref.rs

@@ -2,7 +2,7 @@ use std::{mem, ops::Deref, sync::Arc};
 
 use log::{debug, warn};
 
-use crate::{errors::Result, objects::JObject, sys, JNIEnv, JavaVM};
+use crate::{errors::Result, objects::JObject, sys, JNIEnv, JavaVM, JNIVersion};
 
 // Note: `GlobalRef` must not implement `Into<JObject>`! If it did, then it would be possible to
 // wrap it in `AutoLocal`, which would cause undefined behavior upon drop as a result of calling
@@ -100,7 +100,9 @@ impl Drop for GlobalRefGuard {
             Ok(())
         };
 
-        let res = match self.vm.get_env() {
+        // Safety: we can assume we couldn't have created the global reference in the first place without
+        // having already required the JavaVM to support JNI >= 1.4
+        let res = match unsafe { self.vm.get_env(JNIVersion::V1_4) } {
             Ok(env) => drop_impl(&env),
             Err(_) => {
                 warn!("Dropping a GlobalRef in a detached thread. Fix your code if this message appears frequently (see the GlobalRef docs).");

src/wrapper/objects/weak_ref.rs

@@ -5,7 +5,7 @@ use log::{debug, warn};
 use crate::{
     errors::Result,
     objects::{GlobalRef, JObject},
-    sys, JNIEnv, JavaVM,
+    sys, JNIEnv, JavaVM, JNIVersion,
 };
 
 // Note: `WeakRef` must not implement `Into<JObject>`! If it did, then it would be possible to
@@ -169,7 +169,9 @@ impl Drop for WeakRefGuard {
             Ok(())
         }
 
-        let res = match self.vm.get_env() {
+        // Safety: we can assume we couldn't have created the weak reference in the first place without
+        // having already required the JavaVM to support JNI >= 1.4
+        let res = match unsafe { self.vm.get_env(JNIVersion::V1_4) } {
             Ok(env) => drop_impl(&env, self.raw),
             Err(_) => {
                 warn!("Dropping a WeakRef in a detached thread. Fix your code if this message appears frequently (see the WeakRef docs).");

tests/executor.rs

@@ -132,7 +132,7 @@ fn test_destroy() {
             // JavaVM before it gets destroyed, including dropping the AutoLocal
             // for the `MATH_CLASS`
             {
-                let mut env = jvm.attach_current_thread_as_daemon().unwrap();
+                let mut env = unsafe { jvm.attach_current_thread_as_daemon().unwrap() };
                 println!("daemon thread attach");
                 attach_barrier.wait();
                 println!("daemon thread run");

tests/executor_nested_attach.rs

@@ -4,7 +4,7 @@ use std::{sync::Arc, thread::spawn};
 
 use jni::{
     errors::{Error, JniError},
-    Executor, JavaVM,
+    Executor, JavaVM, JNIVersion,
 };
 
 mod util;
@@ -52,7 +52,8 @@ fn check_detached(vm: &JavaVM) {
 }
 
 fn is_attached(vm: &JavaVM) -> bool {
-    vm.get_env()
+    // Safety: assumes tests are only run against a JavaVM that implements JNI >= 1.4
+    unsafe { vm.get_env(JNIVersion::V1_4) }
         .map(|_| true)
         .or_else(|jni_err| match jni_err {
             Error::JniCall(JniError::ThreadDetached) => Ok(false),

tests/threads_attach_guard.rs

@@ -1,6 +1,7 @@
 #![cfg(feature = "invocation")]
 
 mod util;
+use jni::JNIVersion;
 use util::{attach_current_thread, call_java_abs, jvm};
 
 #[test]
@@ -14,5 +15,5 @@ fn thread_attach_guard_detaches_on_drop() {
     }
     assert_eq!(jvm().threads_attached(), 0);
     // Verify that this thread is really detached.
-    assert!(jvm().get_env().is_err());
+    unsafe { assert!(jvm().get_env(JNIVersion::V1_4).is_err()) };
 }

tests/threads_detach_daemon.rs

@@ -8,7 +8,7 @@ use util::{attach_current_thread_as_daemon, call_java_abs, jvm};
 #[test]
 fn daemon_thread_detaches_when_finished() {
     let thread = spawn(|| {
-        let mut env = attach_current_thread_as_daemon();
+        let mut env = unsafe { attach_current_thread_as_daemon() };
         let val = call_java_abs(&mut env, -3);
         assert_eq!(val, 3);
         assert_eq!(jvm().threads_attached(), 1);

tests/threads_explicit_detach.rs

@@ -1,6 +1,7 @@
 #![cfg(feature = "invocation")]
 
 mod util;
+use jni::JNIVersion;
 use util::{attach_current_thread, call_java_abs, detach_current_thread, jvm};
 
 #[test]
@@ -17,5 +18,5 @@ pub fn explicit_detach_detaches_thread_attached_locally() {
         detach_current_thread();
     }
     assert_eq!(jvm().threads_attached(), 0);
-    assert!(jvm().get_env().is_err());
+    unsafe { assert!(jvm().get_env(JNIVersion::V1_4).is_err()) };
 }

tests/threads_explicit_detach_daemon.rs

@@ -1,12 +1,13 @@
 #![cfg(feature = "invocation")]
 
 mod util;
+use jni::JNIVersion;
 use util::{attach_current_thread_as_daemon, call_java_abs, detach_current_thread, jvm};
 
 #[test]
 pub fn explicit_detach_detaches_thread_attached_as_daemon() {
     assert_eq!(jvm().threads_attached(), 0);
-    let mut guard = attach_current_thread_as_daemon();
+    let mut guard = unsafe { attach_current_thread_as_daemon() };
     let val = call_java_abs(&mut guard, -1);
     assert_eq!(val, 1);
     assert_eq!(jvm().threads_attached(), 1);
@@ -17,5 +18,5 @@ pub fn explicit_detach_detaches_thread_attached_as_daemon() {
         detach_current_thread();
     }
     assert_eq!(jvm().threads_attached(), 0);
-    assert!(jvm().get_env().is_err());
+    unsafe { assert!(jvm().get_env(JNIVersion::V1_4).is_err()) };
 }

tests/threads_explicit_detach_permanent.rs

@@ -1,6 +1,7 @@
 #![cfg(feature = "invocation")]
 
 mod util;
+use jni::JNIVersion;
 use util::{attach_current_thread_permanently, call_java_abs, detach_current_thread, jvm};
 
 #[test]
@@ -17,5 +18,5 @@ pub fn explicit_detach_detaches_thread_attached_permanently() {
         detach_current_thread();
     }
     assert_eq!(jvm().threads_attached(), 0);
-    assert!(jvm().get_env().is_err());
+    unsafe { assert!(jvm().get_env(JNIVersion::V1_4).is_err()) };
 }

tests/threads_nested_attach_daemon.rs

@@ -9,7 +9,7 @@ use util::{
 #[test]
 pub fn nested_attaches_should_not_detach_daemon_thread() {
     assert_eq!(jvm().threads_attached(), 0);
-    let mut env = attach_current_thread_as_daemon();
+    let mut env = unsafe { attach_current_thread_as_daemon() };
     let val = call_java_abs(&mut env, -1);
     assert_eq!(val, 1);
     assert_eq!(jvm().threads_attached(), 1);
@@ -39,7 +39,7 @@ pub fn nested_attaches_should_not_detach_daemon_thread() {
 
     // Nested attach_as_daemon is a no-op.
     {
-        let mut env_nested = attach_current_thread_as_daemon();
+        let mut env_nested = unsafe { attach_current_thread_as_daemon() };
         let val = call_java_abs(&mut env_nested, -5);
         assert_eq!(val, 5);
         assert_eq!(jvm().threads_attached(), 1);

tests/threads_nested_attach_guard.rs

@@ -1,6 +1,7 @@
 #![cfg(feature = "invocation")]
 
 mod util;
+use jni::JNIVersion;
 use util::{
     attach_current_thread, attach_current_thread_as_daemon, attach_current_thread_permanently,
     call_java_abs, jvm,
@@ -39,7 +40,7 @@ pub fn nested_attaches_should_not_detach_guarded_thread() {
 
     // Nested attach_as_daemon is a no-op.
     {
-        let mut env_nested = attach_current_thread_as_daemon();
+        let mut env_nested = unsafe { attach_current_thread_as_daemon() };
         let val = call_java_abs(&mut env_nested, -5);
         assert_eq!(val, 5);
         assert_eq!(jvm().threads_attached(), 1);
@@ -50,5 +51,5 @@ pub fn nested_attaches_should_not_detach_guarded_thread() {
     // despite nested "permanent" attaches.
     drop(env);
     assert_eq!(jvm().threads_attached(), 0);
-    assert!(jvm().get_env().is_err());
+    unsafe { assert!(jvm().get_env(JNIVersion::V1_4).is_err()) };
 }

tests/threads_nested_attach_permanently.rs

@@ -1,6 +1,7 @@
 #![cfg(feature = "invocation")]
 
 mod util;
+use jni::JNIVersion;
 use util::{
     attach_current_thread, attach_current_thread_as_daemon, attach_current_thread_permanently,
     call_java_abs, jvm,
@@ -39,11 +40,11 @@ pub fn nested_attaches_should_not_detach_permanent_thread() {
 
     // Nested attach_as_daemon is a no-op.
     {
-        let mut env_nested = attach_current_thread_as_daemon();
+        let mut env_nested = unsafe { attach_current_thread_as_daemon() };
         let val = call_java_abs(&mut env_nested, -5);
         assert_eq!(val, 5);
         assert_eq!(jvm().threads_attached(), 1);
     }
     assert_eq!(jvm().threads_attached(), 1);
-    assert!(jvm().get_env().is_ok());
+    unsafe { assert!(jvm().get_env(JNIVersion::V1_4).is_ok()) };
 }

tests/util/mod.rs

@@ -50,7 +50,7 @@ pub fn attach_current_thread() -> AttachGuard<'static> {
 }
 
 #[allow(dead_code)]
-pub fn attach_current_thread_as_daemon() -> JNIEnv<'static> {
+pub unsafe fn attach_current_thread_as_daemon() -> JNIEnv<'static> {
     jvm()
         .attach_current_thread_as_daemon()
         .expect("failed to attach jvm daemon thread")