rustls / rustls-platform-verifier

A certificate verification library for rustls that uses the operating system's verifier
Apache License 2.0
57 stars 18 forks source link

upgrade jni v0.21 #63

Open flisky opened 6 months ago

flisky commented 6 months ago

~jni v0.19 doesn't work for me.~

~I use tokio multi threads runtime, and our app is killed by system -~

e.android.debug: java_vm_ext.cc:594] JNI DETECTED ERROR IN APPLICATION: a thread (tid 5917 is making JNI calls without being attached
e.android.debug: java_vm_ext.cc:594]     in call to PushLocalFrame

~It turns out java_vm::get_env() goes null even after calling AttachCurrentThread in a few threads, while other threads are okay.~ ~I have no idea. However, upgrade to jni v0.21.0 eliminates the problem for me.~

fixes #22

complexspaces commented 6 months ago

Thanks for the PR! It looks like the changes here aren't currently compiling for Android. Once CI is passing I'm happy to take a look through this.

It turns out java_vm::get_env() goes null even after calling AttachCurrentThread in a few threads, while other threads are okay. I have no idea. However, upgrade to jni v0.21.0 eliminates the problem for me.

That's very strange, and definitely something I'll look into on the side. We use this exact same JNI-abstraction inside 1Password's Android app (this code was copy/pasted out) and we've never seen this issue AFAICT. If you have any other JNI code in your app, it might be worth checking if its having a poor interaction with rustls-platform-verifier.

flisky commented 6 months ago

If you have any other JNI code in your app, it might be worth checking if its having a poor interaction with rustls-platform-verifier.

We're using mozilla/uniffi which uses JNA actually. We have to configure JNI environment mannually, and https://github.com/mozilla/uniffi-rs/issues/1778#issuecomment-1807979746 inspires us.

complexspaces commented 6 months ago

It would be great if jni could make another release before this merges. I don't want to push a very outdated version of windows-sys into everyone's dependency trees.

complexspaces commented 6 months ago

It turns out java_vm::get_env() goes null even after calling AttachCurrentThread in a few threads, while other threads are okay. I have no idea. However, upgrade to jni v0.21.0 eliminates the problem for me.

We also use a multi-threaded Tokio runtime at work and haven't encountered this, so I am curious where the difference is.

@flisky Are you able to/do you mind sharing your Android context initialization code? While looking at the SO post in your linked GitHub comment, some things stood out to me as strange. Notably its unclear why they are wrangling the list of JVMs and current thread instead of passing an environment from Kotlin/Java and using JniEnv::get_java_vm (which calls GetJavaVM) instead. You can see an example of this here for this crate's tests.

flisky commented 6 months ago

I create a demo project for this, and you can take a look at https://github.com/flisky/jnidemo/blob/main/src/android.rs Unfortunately, I cannot reproduce with that project. And I'll tweak it in next few days.

flisky commented 2 months ago

Hey @complexspaces, I'm back on this issue, and I finally find the root case.

During several days debugging, it turns out jna will also attach & detach thread if current thread is not attached.

We're usinguniffi callback to log tracing events, and the race happens:

  1. logging start (jna attached)
  2. verifier start (jni attached)
  3. logging finished (jna detached)
  4. verifying (jni calling).

Now, we're calling attach_current_thread_permanently manully when https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.on_thread_start, and make sure jni's logging disabled.

Thanks this bug we find out something to optimized. Hope it help:)

(By the way, jna & jni can be mixed used, because java loads native library only once.)