AttachGuard detaches too soon when dropped out of order

[wleslie]

Repro:

let jvm = JavaVM::new(InitArgsBuilder::new().build().unwrap()).unwrap();
let env1 = jvm.attach_current_thread().unwrap();
let mut env2 = jvm.attach_current_thread().unwrap();
std::mem::drop(env1);
// the thread is no longer attached, even though env2 is alive!
env2.find_class("java/lang/String").unwrap();
// seg fault!

It looks like the implementation assumes that AttachGuard instances will be dropped in last-in-first-out order. Whichever AttachGuard is created first is designated as the one that will detach on drop. But the instance that was created first isn't necessarily the same one that will be dropped last.

[rib]

Ah, tricky.

Yeah the implementation seems to try and mirror how the C API lets you redundantly call AttachCurrentThread and it will be a NOP that just return the JNIEnv in that case.

The implementation of attach_current_thread() sees when the attach is redundant and in that case it returns a guard that won't do anything on Drop. (so it's not exactly that there's any detailed consideration of lifo ordering except that the first guard that really does attach the thread will be the only guard that does anything on Drop)

I'm not exactly sure why it was implemented this way but I guess it was the simplest approach that reflected how the C API works.

It looks like there needs to be a per-thread ref-count for these guards to work like this.

With needing to have a thread-local reference counter that also makes me wonder what hazards there could be if there are multiple versions of the jni crate linked into one application - each with a private set of thread-local variables.

Similar to the current implementation I think there would need to be some kind of special case to recognize if the first attach increment for the thread was redundant (another version of the jni crate or some other non-Rust code might have attached the thread) and we'd probably have to just assume something else will be responsible for detaching the thread in this case.

[rib]

For reference also see this recent discussion around the soundness of APIs that return a JNIEnv here: #436 (reply in thread)