-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
0.21 mutability requirements on JNIEnv
methods break use of catch_unwind
#432
Comments
JNIEnv
methods breaks use of catch_unwind
JNIEnv
methods breaks use of catch_unwind
JNIEnv
methods breaks use of catch_unwind
JNIEnv
methods breaks use of catch_unwind
JNIEnv
methods breaks use of catch_unwind
JNIEnv
methods break use of catch_unwind
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 The 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 One thing I've wondered about briefly is whether the Although the closure would still need to be passed a I've also been thinking about this from the pov of trying to make If we were to introduce a 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. |
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. |
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)
})
} |
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 It seems like it would be helpful to have something like this just be part of the I like the idea of making it a trait like this so it's possible to just override how you map an Regarding the vague ideas around having some kind of stateful 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 |
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 And yes, it uses |
@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 |
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.
I'd missed this. Will include it. |
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 Rust also protects you from observing partially-finished mutations of data stored in a With JNI, not only are all Java objects always mutable, it's possible to observe mutations via a |
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. |
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 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 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. |
I've been looking into writing a failure handler, and I've run into a couple of problems.
|
The 0.21.X crates feature a major refactor that breaks the code. Don't upgrade to them until some issues are resolved. (See jni-rs/jni-rs#432 for more info.)
What about using unsafe_clone() for obtaining copy of JNIEnv used only for constructing exception? like this:
Does throwing exception create local reference that can shoot in leg with UB? |
@AlexKorn: Almost certainly yes. If you look up the exception's class by name, that creates a local reference. If you instantiate it (with It's technically possible to throw an exception without creating any local references, by using That said, that's pretty much what you have to do. The failure handler I was writing also uses |
@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? |
@AlexKorn: Yes, that's legal. |
…C 8410) (#46) * Add DER & PEM support for SigningKeySeed and VerificationKeyBytes (RFC 8410) - Add encoding to and decoding from DER bytes and PEM strings for SigningKeySeed and VerificationKeyBytes. - Add some functions so that the Java code mirrors, to a certain degree, the JDK 15 interface for Ed25519 keys and signatures. - Add encoding and decoding for signatures (technically identity functions). - Miscellaneous cleanup. * Accommodate extra octet string in private key DER bytes - In RFC 8410, DER-encoded private keys are in an octet string that's encapsulated by another octet string. Add the extra octet string, and adjust tests as necessary. - In the tests, use the private key from RFC 8410, Sect. 10.3. * Update pkcs8 to 0.7.0 * Cleanup - Enhance PEM capabilities for SigningKey and VerificationKeyBytes. This also allowed for some tests to be simplified. - From -> TryFrom for some VerificationKeyBytes impls. * Upgrade JNI Rust bindings to PKCS8 0.7.5 - Make necessary changes to support the newer crate. - Fix an unrelated compiler warning. * More fixups - Get code to compile after updating to the latest Rust. - Fix a couple of failing tests (add LF to expected encoding output). * Major update - Update pkcs8 crate to 0.10.0, and update code as required to support the crate. This includes supporting the Decode(Public/Private)Key and Encode(Public/Private)Key traits so as to take advantage of Ed25519 DER and PEM code in the crate. - Add the latest ed25519 crate (2.2.0) to support KeypairBytes and other features. - Remove the signature code and implement Signature (Signer and Verifier traits) from the "signatures" crate included with the pkcs8 crate. - Update the JNI code. This includes mandating Scala 3 usage. - Minor cleanup (including warning fixes) and changes to make the code a bit clearer. A follow-up commit will clean up the tests and probably add support for v2 private DER keys. * Further code cleanup - Update pkcs8 crate to 0.10.1. - Fix PEM feature code. - Update Ed25519 JNI code as needed. - Remove dead code. - Re-enable a couple of unit tests. Note that a couple of Ed25519 JNI unit tests are still failing. A follow-up PR will have the fix. * Add missing DER/PEM files for unit tests * Add JNI comments to resolve publisher warnings When executing `sbt publishLocal` and generating a JAR file, there are warnings regarding some functions not having public comments. Add public comments as needed. * JNI README update * Comment touchup * Review fixups - Finish adding PEM/PKCS8 tags and cfg items as needed to separate the features from default compilation. - Revert some minor name changes. - Make the JNI README more precise with regards to requirements. - Add ARM64 macOS support to JNI. Untested but it should work, and it doesn't break Intel Macs. - Miscellaneous cleanup, including fixing cargo and sbt warnings. * Upgrade jni crate to 0.20.0 The 0.21.X crates feature a major refactor that breaks the code. Don't upgrade to them until some issues are resolved. (See jni-rs/jni-rs#432 for more info.) * Upgrade jni crate to 0.21.1 - A path forward to upgrading to 0.21.X was suggested by the jni-rs library developer (jni-rs/jni-rs#439 (comment)). Upgrade the code, improving the safety of the JNI code. - Cargo.toml fixups. * cargo clippy / cargo fmt cleanup Also do minor JNI README cleanup. * Use an export to clean up some tests a bit --------- Co-authored-by: Douglas Roark <douglas.roark@gemini.com>
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but more notably, they suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we have to call `GetVersion` to determine the full set of pointers that are valid. The original 1.1 spec did leave some gaps in the original table that where filled with NULLs these were only to be used in future versions (so there is never any optional pointer). Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but more notably, they suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we have to call `GetVersion` to determine the full set of pointers that are valid. The original 1.1 spec did leave some gaps in the original table that where filled with NULLs these were only to be used in future versions (so there is never any optional pointer). Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but, more notably, they also suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we have to call `GetVersion` to determine the full set of pointers that are valid. The original 1.1 spec did leave some gaps in the original table that where filled with NULLs these were only to be used in future versions (so there is never any optional pointer). Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but, more notably, they also suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we sometimes have to call `GetVersion` to determine the full set of pointers that are valid. The original 1.1 spec did leave some gaps in the original table that where filled with NULLs these were only to be used in future versions (so there is never any optional pointer). Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but, more notably, they also suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we sometimes have to call `GetVersion` to determine the full set of pointers that are valid. Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but, more notably, they also suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we sometimes have to call `GetVersion` to determine the full set of pointers that are valid. Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
The discussion here was one of things I had in mind when working these changes to the low-level macros we use for making JNI calls, to simplify them, and (imho) make them a lot safer too: #478 In particular, regarding #432 (comment) that PR makes several basic JNI functions infallible (including I'd be good to get your feedback if you get a chance @argv-minus-one |
None of the JNI 1.1 function pointers were optional, and neither are any of the function pointers in later versions of the spec. Having the `Option` implies that null pointer checks are required but, more notably, they also suggest that null pointer checks could be used for JNI >= 1.2 pointers, which could be a dangerous mistake since these are effectively extending beyond the table of pointers that was defined for JNI 1.1 and should be assumed to be invalid pointers that mustn't be touched. It's also notable that we sometimes have to call `GetVersion` to determine the full set of pointers that are valid. Recently the use of `Option` also raised some questions about our ability to, infallibly, handle Rust panics when we want to map a panic to a Java exception via JNI: jni-rs/jni-rs#432 (comment)
Prior to 0.21 it was possible to wrap calls to JNIEnv methods in a closure to
catch_unwind
, allowing graceful handling of panics to raise Java exceptions. TheUnwindSafe
trait is not automatically implemented for&mut T
meaning that the new mutability requirements on many of the JNIEnv methods have made this approach invalid.Here's a basic example of how I use
catch_unwind
that now won't compile:I'm fairly new to JNI, but I believe it's standard practice to catch panics this way when working with it. I know you can leverage
AssertUnwindSafe
to assertUnwindSafe
but it's not clear whether it's safe to do so for&mut JNIEnv
.Finally, apologies if I've missed something in the docs or the migration guide, but I can't seem to find anything about handling panics.
Is it safe to use
AssertUnwindSafe
with&mut JNIEnv
? For example, the following compiles but I don't know if it's actually safe (fyi I'm not familiar with this approach but I'd prefer not to wrap the entire closure):I'm not sure if it's possible to manually impl
UnwindSafe
for&mut JNIEnv
but is there an issue/feature to add this, or is there a different approach entirely?Thanks for your time!
The text was updated successfully, but these errors were encountered: