Making co-existing jni-sys wrappers as safe as possible

[fpoli]

While building duchess we found a data race in the OpenJDK implementation of JNI_GetCreatedJavaVMs. When called concurrently with JNI_CreateJavaVM, JNI_GetCreatedJavaVMs might return pointers to partially-initialized JVMs (OpenJDK bug), which then causes segmentation faults.

The naive fix for duchess and any other library that aims to provide a safe Rust API around jni-sys is to use a mutex to synchronize calls to JNI_CreateJavaVM and JNI_GetCreatedJavaVMs. However, if each library has its own mutex, the overall Rust API is still technically unsound because one application might depend on multiple such libraries (or different versions of the same library), each making calls to JNI_CreateJavaVM and JNI_GetCreatedJavaVMs that are effectively unsynchronized.

We were discussing how to make co-existing JNI libraries (e.g. jni, j4rs, duchess) as safe as possible. Adding something to jni-sys seems a good approach, because jni-sys is used by all such libraries. Two options are:

• Option A: Add a "start the JVM safely" mechanism to jni-sys.
• Option B: Add a global Mutex<()> to jni-sys, leaving to the safe JNI libraries the duty of properly syncronizing the JNI calls that are (due to bugs or by design) thread-unsafe.

Option A requires more code in jni-sys; option B requires less code in jni-sys but more in the safe JNI libraries. Both options assume that all safe JNI libraries will use the same jni-sys version. I don't know if there is a way to enforce that.

[nikomatsakis]

It is possible to enforce that everybody uses the same version through some means, though I forget the details. I think you can declare that you link against something external in a way that makes cargo force you to have exactly one version. Or maybe I'm only talking about something we thought about adding =) but I'm pretty sure it exists. I'll ask around.

[nikomatsakis]

This is what I am talking about:

https://doc.rust-lang.org/cargo/reference/resolver.html#links

[rib]

I'm not entirely sure we should be trying to workaround upstream OpenJDK bugs in jni-sys - and I'm not sure we technically can in this case (at least not entirely).

We can only workaround a race condition in JNI_GetCreatedJavaVMs if we know that jni-sys is strictly the only way that JNI_GetCreatedJavaVMs could be called and even if we knew there was only one version of jni-sys we can't automatically rule out the possibility that JNI is used via some other bindings - maybe indirectly via another native library written in C/C++ or even written in Rust using something like bindgen instead of jni-sys (e.g. this project uses bindgen: https://github.com/Dushistov/flapigen-rs/tree/master/android-tests)

I'm curious what your use case for GetCreateJavaVMs is. This currently isn't exposed / used in the jni crate.

I think it's quite usual to acquire a VM pointer in some platform specific way unless you explicitly created the VM (in which case there wouldn't be a race)

One standard mechanism for acquiring a VM pointer that you didn't create is via a JNI_OnLoad symbol for libraries but that doesn't work well for Rust when there can only be a single JNI_OnLoad implementation.

My use case for JNI is generally on Android and in that case we currently have a standard ndk-context that we use as a singleton for sharing the JVM. https://docs.rs/ndk-context/latest/ndk_context/

Since we have a few things we'd like to update with that ndk-context crate I've discussed before creating something equivalent under the jni-rs organization that wouldn't be Android specific and I wonder if something like that could help alleviate the need for calling JNI_GetCreatedJavaVMs.

[rib]

I couldn't remember where we had this discussion before, but here's an issue I filed a while ago to track the idea of creating a replacement for the ndk-context crate for sharing a JVM pointer: jni-rs/jni-rs#421 - seems potentially relevant here.

[rib]

Since this issue is currently also about the wider question of how to enable safe interop with multiple jni wrapper crates I also just wanted to link to this issue I opened for the jni crate to investigate hazards around multiple versions of the jni crate and having inconsistent thread-local-storage state for tracking JVM thread attachments: jni-rs/jni-rs#422

There are probably a number of risks related to different jni crates trying to independently manage thread detachment and getting into situations where one crate will detach a thread from the JVM because its world view says that's OK, but it doesn't realize there was another crate still relying on that attachment.