0.21 mutability requirements on `JNIEnv` methods break use of `catch_unwind` about jni-rs HOT 15 OPEN

No need to apologise! I understand the whack-a-mole analogy completely. The 0.21 update was mostly great to see from my perspective, having more guarantees surrounding lifetimes is a good thing in FFIs.

By the sounds of it, AssertUnwind is safe to use so I'll roll with that for now.

I'll update this thread should anything strange come up. I think it's probably worth popping something in the docs regarding this for those trying to catch panics in this way.

from jni-rs.

@AlexKorn: Yes, that's legal.

from jni-rs.

Right, this is an unfortunate side effect of the changes made for 0.21, sorry about that. More than once now I've found myself compare this project to a game of whack a mole when it comes to addressing safety concerns.

As you say, you can currently use AssertUnwindSafe

The JNIEnv is really just a thin wrapper over a pointer to a table of function pointers managed by JNI / the JVM implementation.

As far as Rust is concerned this isn't really stateful (so you shouldn't be able to observe an inconsistent state for this table due to a panic).

The design allows the JVM to provide a different table of functions for different threads but I've never seen anything to suggest that JNI functions would ever try and dynamically fiddle with that table of pointers on the fly - and even if it did it would still have to remain consistent from the pov of Rust code.

The JVM can store private thread-local storage within the JNIEnv but that also has to always remain consistent from the pov of Rust code. It might be mutated within the JNI/JVM implementation but that code won't panic / unwind.

At the very least the new &mut JNIEnv is as unwind safe as the previous &JNIEnv was.

One thing I've wondered about briefly is whether the jni crate can provide some built-in helpers for catching panics and re-throwing as Java exceptions, similar to with_local_frame. Maybe this would be able to improve the ergonomics here.

Although the closure would still need to be passed a &mut JNIEnv argument we could at least deal with wrapping it within AssertUnwindSafe automatically. I'm not quite sure what the ergonomics of that would feel like without trying it out.

I've also been thinking about this from the pov of trying to make JNIEnv stateful - whereby it wouldn't just be a thin wrapper over the JNI C pointer but would instead be our own struct that lets jni-rs cache certain things like method IDs for commonly used types. Right now the utility APIs for things like JList or JMap aren't ideal at all because they have to repeatedly query the method IDs for those classes since we have nowhere to cache them.

If we were to introduce a StatefulJNIEnv type of some kind then it would be good if we had a built-in API for handling catching panics (+ re-throwing exceptions) that could always be used consistently to map from a raw JNIEnv passed to a native method to a closure that would be passed a StatefulJNIEnv.

Right now there isn't another issue tracking this, so thanks for bringing this up.

Sorry that it's not a great answer for now.

from jni-rs.

I want to handle panics and errors consistently and without boiler plate code; I simply want to throw Java exceptions and print to stdout if that fails. I think this is probably befitting to what most people want to do, so for what it's worth I put the below traits and implementations together. I toyed with putting this on crates.io or submitting a PR but I didn't want to start something that might deviate from this project or cause you problems.

Note, it's not properly tested! Any feedback would be welcome and appreciated. This approach (especially the dummy value trait) is heavily inspired by libsingal.

// See https://github.com/rust-lang/rfcs/issues/1389
pub fn describe_panic(any: &Box<dyn std::any::Any + Send>) -> String {
    if let Some(msg) = any.downcast_ref::<&str>() {
        msg.to_string()
    } else if let Some(msg) = any.downcast_ref::<String>() {
        msg.to_string()
    } else {
        "(break on rust_panic to debug)".to_string()
    }
}

/// Provides a dummy value to return when an exception is thrown.
/// TODO flesh this out with more types.
pub trait JniDummyValue {
    fn dummy_value() -> Self;
}

impl JniDummyValue for ObjectHandle {
    fn dummy_value() -> Self {
        0
    }
}

impl JniDummyValue for jint {
    fn dummy_value() -> Self {
        0
    }
}

impl JniDummyValue for jobject {
    fn dummy_value() -> Self {
        std::ptr::null_mut()
    }
}

impl JniDummyValue for jboolean {
    fn dummy_value() -> Self {
        0
    }
}

impl JniDummyValue for () {
    fn dummy_value() -> Self {}
}

impl JniDummyValue for JString<'_> {
    fn dummy_value() -> Self {
        unsafe { JString::from_raw(jstring::dummy_value()) }
    }
}

pub(crate) trait JNIExt {
    type Error;

    /// Called when an error is returned from the closure executed by `throw_on_failure`.
    fn handle_error(&mut self, error: Self::Error);
    /// Called when an unwinding panic is returned from the closure executed by `throw_on_failure`.
    fn handle_panic(&mut self, panic_err: Box<dyn std::any::Any + Send>);

    /// Invokes the given closure and throws a Java exception from an `Result::Err` or unwinding
    /// panic if either occurs.
    ///
    /// # Note
    /// The actual calls to `JNIEnv::throw` are made in the `handle_error` and `handle_panic`
    /// implementations. Where an error variant of a Result is returned from the given closure it is
    /// passed to the `handle_error` method, and where an unwinding panic is caught it is passed to
    /// the `handle_panic` method.
    fn throw_on_failure<
        F: FnOnce(&mut Self) -> Result<R, Self::Error> + UnwindSafe,
        R: JniDummyValue,
    >(
        &mut self,
        f: F,
    ) -> R {
        let mut wrapped_env = AssertUnwindSafe(self);

        match catch_unwind({
            let mut other_wrapped_env = AssertUnwindSafe(&mut wrapped_env);

            move || (f)(***other_wrapped_env)
        }) {
            Ok(Ok(r)) => r,
            Ok(Err(ext_error)) => {
                (**wrapped_env).handle_error(ext_error);
                R::dummy_value()
            },
            Err(catch_unwind_err) => {
                (**wrapped_env).handle_panic(catch_unwind_err);
                R::dummy_value()
            },
        }
    }
}

impl JNIExt for JNIEnv<'_> {
    type Error = Foo;

    fn handle_error(&mut self, error: Self::Error) {
        let exc_msg = match error {
            Foo::SomeErr(_) => "java/lang/RuntimeException",
            _ => "java/lang/AssertionError"
        };

        if let Err(err) = self.throw_new(exc_msg, error.to_string()) {
            println!("failed to throw Java exception from error {}: {}", error, err);
        }
    }

    fn handle_panic(&mut self, panic_err: Box<dyn Any + Send>) {
        let panic_msg = describe_panic(&panic_err);

        if let Err(err) = self.throw_new("java/lang/Exception", panic_msg.as_str()) {
            println!("failed to throw Java exception for {}: {}", panic_msg, err);
        }
    }
}

And usage:

#[no_mangle]
pub extern "system" fn Java_HelloWorld_hello<'local>(
    mut env: JNIEnv<'local>,
    _class: JClass<'local>,
    input: JString<'local>,
) -> JString<'local> {
    env.throw_on_failure(|jni_env| {
        let input: String = jni_env
            .get_string(&input)
            .expect("Couldn't get java string!")
            .into();

        let output = jni_env
            .new_string(format!("Hello, {}!", input))
            .expect("Couldn't create java string!");

        Ok(output)
    })
}

from jni-rs.

Perfect, yeah that's the kind of thing I'd been imagining but hadn't considered the idea of making it a trait like this. I wasn't sure off the top of my head whether it was going to be possible to move the env into the catch_unwind closure via AssertUnwindSafe but effectively hide that and unwrap when calling the user's closure but it looks like that works fine - nice.

It seems like it would be helpful to have something like this just be part of the jni API, out of the box, ideally, since pretty much any code that's implementing a native Java method needs to do something like this to stop panics crossing the FFI boundary.

I like the idea of making it a trait like this so it's possible to just override how you map an Error or panic into an exception that's appropriate for your app / library.

Regarding the vague ideas around having some kind of stateful JNIEnv, that's not something I'm investigating / prioritizing atm, so I'm not going to worry about how that might potentially interact with this for now, since there's nothing concrete to consider yet.

It would be great if you'd be able to even create a PR for this considering that it looks like you've done most of the work already.

I'd maybe call the "Dummy" values "Default" values from the pov that it's conceptually like you're passing back default initialized values. "Default" matches the Java and Rust parlance here.

Actually, we already implement the Default trait for types like JObject, JClass, JString etc so can we just rely on the Default trait instead of needing the dummy types here?

from jni-rs.

I have some code for handling panics in my project. I've been busy lately, but I'll make a PR from it when I have a moment.

It's pretty straightforward. It catches an unwinding panic, tries to get the panic message, throws java.lang.Error, and returns Default::default(). It also throws java.lang.Error if it gets a jni::error::Error unless a Java exception is already pending.

And yes, it uses catch_unwind and AssertUnwindSafe. It's not actually unwind-safe—it's definitely possible to observe invalid state in Java object fields after an unwinding panic in Rust—but because Java object fields are always mutable and have no single owner, unwind safety with JNI is impossible anyway.

from jni-rs.

@argv-minus-one thanks for your input. I hadn't looked into pending exceptions. I'm pretty new to Java, so JNI is a whole other thing I'm yet to fully get my head around.

Can you provide any more info, or even just links to your point regarding unwind-safety in Rust with Java objects in general? I'm also confused about pending exceptions how to handle them.

EDIT: this seems like a good description: https://docs.oracle.com/en/java/javase/11/docs/specs/jni/design.html#java-exceptions

from jni-rs.

@rib

It would be great if you'd be able to even create a PR for this considering that it looks like you've done most of the work already.

I'll try get something in this week. I'm keen to see what @argv-minus-one is doing in their implementation, but if I've not heard anything I'll just pop a PR in a tag him or something.

Actually, we already implement the Default trait for types like JObject, JClass, JString etc so can we just rely on the Default trait instead of needing the dummy types here?

I'd missed this. Will include it.

from jni-rs.

Can you provide any more info, or even just links to your point regarding unwind-safety in Rust with Java objects in general?

Unwind safety is Rust's version of the language-neutral concept of exception safety. The Rust compiler tries to enforce unwind safety by not allowing you to catch_unwind if the closure captures anything mutable, like &mut _ or &RefCell<_>, so that the caller can't see any half-finished mutations if the closure panics.

Rust also protects you from observing partially-finished mutations of data stored in a static global variable and protected by a Mutex, by way of the mutex becoming poisoned if there's a panic while it's locked. (Oddly, there is no such protection when using RefCell with LocalKey.)

With JNI, not only are all Java objects always mutable, it's possible to observe mutations via a GlobalRef stored in a static global variable without any closure capture at all, so there's no way for Rust's enforced unwind safety to work in the presence of JNI.

from jni-rs.

I can speak to the libsignal use case: we want panics in Rust code to turn into AssertionExceptions in Java. There's no expectation (from us) of protecting Java objects from partial mutation or any of that; we just want to drop any work we're doing and return to the JVM. Many of our operations are stateless and so this logically can't lead to any problems except when we call back into the JVM for "load" and "store" operations, which we can treat as "panic-atomic" (either a Rust panic happens before the Java method starts executing, or after execution of that method has finished, but not during).

You could argue that this means we should just AssertUnwindSafe, and that would be a fair position to take, but it would mean we'd lose out on rustc checking whether our Rust code has anything unwind-unsafe in it.

from jni-rs.

And yes, it uses catch_unwind and AssertUnwindSafe. It's not actually unwind-safe—it's definitely possible to observe invalid state in Java object fields after an unwinding panic in Rust—but because Java object fields are always mutable and have no single owner, unwind safety with JNI is impossible anyway.

I suppose the definition of 'unwind safety' can get a little subjective / application specific.

My thinking here was to just be concerned about whether the JNIEnv can get into a dangerous, inconsistent state from the pov of Rust code.

This wouldn't just mean that state changes are observable - it would imply to me that the env is actually dangerous to use from Rust due to some invariant being broken that Rust code depends on (such as a corrupt vtable).

General state changes being observable is comparable to any struct that has internal mutability but since we're talking about such a vast collection of state (an entire JVM) then it's certainly very plausible that some observable state changes could be considered inconsistent (and therefore not unwind safe) for a specific application / use case.

At least one place where we know the JNIEnv could get into a more dangerously inconsistent state would be in combination with critical sections while accessing array elements, which is probably good to track in this context. We did discuss moving the critical section into a closure with catch_unwind here: #400 (comment) but since it currently requires unsafe code to create a critical section currently then it also seems reasonable for now that the burden of handling this is on the user of the unsafe API.

Since the env state does ultimately encompass an entire JVM then it seems like our only practical choice is to focus on being satisfied that the env can't get into a dangerous, corrupt state from the pov of Rust code that hits a panic + unwinds. Any other kind of logical inconsistency in JVM state probably just needs to be left as a higher-level concern.

from jni-rs.

I've been looking into writing a failure handler, and I've run into a couple of problems.

JNIEnv methods all can panic

The failure handler must not panic. Problem: the macros used by all JNIEnv methods emit log calls for every JNI function call, and the logger implementation may panic.

Should the log calls be removed? If not, should additional non-logging macros be added, and used for JNIEnv methods that the panic handler calls?

JNIEnv methods all can return an error

Pretty much all JNIEnv methods, like exception_check, can return an error. The failure handler needs to be able to call at least JNIEnv::exception_check, JNIEnv::exception_clear, and JNIEnv::fatal_error infallibly.

I see two ways that any JNIEnv method can return an error:

jni_method! checks if JNIEnv::internal is null

jni_method! calls deref! on JNIEnv::internal and its pointee, which checks that these pointers are not null.

Is this necessary? Unless I'm mistaken, JNIEnv::internal and its pointee are expected to always be non-null. Are there any situations where it's valid for either of these pointers to be null?

jni_method! checks if the JNI function pointer is null

jni_method! checks if the requested function pointer in the interface function table is not null, even JNI 1.1 functions like FatalError that should always be available.

As far as I know, there are only a few function pointers that are potentially null, namely those whose slots were reserved as null in the original JNI 1.1 specification. They are:

• FromReflectedMethod
• FromReflectedField
• ToReflectedMethod
• ToReflectedField
• PushLocalFrame
• PopLocalFrame
• NewLocalRef
• EnsureLocalCapacity
• Any future functions that are inserted into the first four slots of the table, which are still reserved as of JNI 17.

All other functions that have been added to the table since the first version of JNI were added to the end of the table. Trying to read any of them when the JVM doesn't support them presumably results in undefined behavior, not a null pointer, since you're reading past the end of the table.

@jrobsonchase, in jni-rs/jni-sys@5937169 you changed the non-reserved fields of jni_sys::JNINativeInterface_ from plain function pointers to Optional function pointers. Would you happen to remember the reason for this change? I'm thinking we should change them back, except those in the list above.

from jni-rs.

What about using unsafe_clone() for obtaining copy of JNIEnv used only for constructing exception? like this:

let mut env_copy = unsafe { env.unsafe_clone() };
match std::panic::catch_unwind(..function_logic..) {
    Ok(result) = result,
    Err(_) => {
        env_copy.throw("Panic message");
        .. return fake result..
    }
}

Does throwing exception create local reference that can shoot in leg with UB?

from jni-rs.

@AlexKorn: Almost certainly yes. If you look up the exception's class by name, that creates a local reference. If you instantiate it (with JNIEnv::new_object or the like), that creates another local reference.

It's technically possible to throw an exception without creating any local references, by using JNIEnv::throw on a pre-existing reference to an exception object. Creating a new exception object, though, will inevitably create at least one local reference.

That said, that's pretty much what you have to do. The failure handler I was writing also uses unsafe_clone.

from jni-rs.

@argv-minus-one Thank you for the answer. I get the point that we're creating local references during throw invocation, but do I grok reference frames correctly: reference frame of JNIEnv that you obtain as argument for exported function will not be exited until the function returns, and unsafe_clone is just raw copying of pointer, in above scenario we do not meddle with such things as creating another reference frame and passing something from it to outer scope like in #392, so making copy of our top-level (within exported function) JNIEnv and using it in function context is legal?

from jni-rs.