jni.md

category	description
Android	Unsafe and unsound. Has responded to fixes well though.

jni

version	thoroughness	understanding	rating
0.13.0	low	medium	negative

issue	severity	comment
#197	high	Definite soundness issue

0.13.0

Decent start, but soundness issues. Also poor method caching.

Rated files were at least reviewed to thoroughness + understanding medium, but the rest was only reviewed to thoroughness low.

Detail

File	Rating	Notes
.github/PULL_REQUEST_TEMPLATE.md	+1
.travis/run_travis_job.sh	+1
.vscode/tasks.json	+1
benches/api_calls.rs	+1	Various API concerns but this file is fine.
examples/HelloWorld.h	+1	Matches HelloWorld.java... generated by javah?
examples/HelloWorld.java	+1
examples/Makefile	+1
src/wrapper/descriptors/class_desc.rs	+1
src/wrapper/descriptors/desc.rs	+1
src/wrapper/descriptors/exception_desc.rs	+1
src/wrapper/descriptors/field_desc.rs	+1
src/wrapper/descriptors/method_desc.rs	+1
src/wrapper/descriptors/mod.rs	+1
src/wrapper/java_vm/init_args.rs	+1
src/wrapper/java_vm/mod.rs	+1
src/wrapper/java_vm/vm.rs	-1	Not 100% sure if it's sound to detatch threads out from under other Java code. Some unsafe attach fns also confuse me as to why they're unsafe.
src/wrapper/objects/auto_local.rs	-1	Not 100% sure if AutoLocal::new is sound based on scary JVM crash warnings
src/wrapper/objects/global_ref.rs	+1
src/wrapper/objects/jbytebuffer.rs	-1	Allowing construction from arbitrary jobject s will likely be unsound later
src/wrapper/objects/jclass.rs	-1	Ditto
src/wrapper/objects/jfieldid.rs	-1	Ditto
src/wrapper/objects/jlist.rs	-1	Called it - internal is used in safe fns, unsound looking as fuck.
src/wrapper/objects/jmap.rs
src/wrapper/objects/jmethodid.rs
src/wrapper/objects/jobject.rs
src/wrapper/objects/jstaticfieldid.rs
src/wrapper/objects/jstaticmethodid.rs
src/wrapper/objects/jstring.rs
src/wrapper/objects/jthrowable.rs
src/wrapper/objects/jvalue.rs
src/wrapper/objects/mod.rs
src/wrapper/strings/ffi_str.rs
src/wrapper/strings/java_str.rs
src/wrapper/strings/mod.rs
src/wrapper/errors.rs
src/wrapper/executor.rs
src/wrapper/jnienv.rs
src/wrapper/macros.rs
src/wrapper/signature.rs
src/wrapper/version.rs
src/lib.rs
src/sys.rs	+1	pub use jni_sys::*
tests/util/example_proxy.rs
tests/util/mod.rs
tests/executor_nested_attach.rs
tests/executor.rs
tests/java_integers.rs
tests/jmap.rs
tests/jni_api.rs
tests/jni_global_ref_is_deleted.rs
tests/jni_global_refs.rs
tests/threads_attach_guard.rs
tests/threads_detach_daemon.rs
tests/threads_detach.rs
tests/threads_explicit_detach_daemon.rs
tests/threads_explicit_detach_permanent.rs
tests/threads_explicit_detach.rs
tests/threads_nested_attach_daemon.rs
tests/threads_nested_attach_guard.rs
tests/threads_nested_attach_permanently.rs
.appveyor.yml
.gitignore
.travis.yml
Cargo.toml	+1
Cargo.toml.orig	+1
CHANGELOG.md	+1
clippy.toml	+1
CODE_OF_CONDUCT.md	+1
CONTRIBUTING.md	+1
LICENSE-APACHE	+1
LICENSE-MIT	+1
README.md	+1

Other	Rating	Notes
unsafe	-1	Unsound
fs	+1	Not used
io	+1	Not used
docs	+1	Well documented
tests	+1	Decent testing

benches/api_calls.rs

Line	Notes
52	..._unchecked is safe? Look at call_static_method_unchecked carefully.
69	Not all ..._unchecked are safe?
154	Must manually drop local refs? Lame.
226	No use of black box?
	+1

src/wrapper/descriptors/method_desc.rs

Line	Notes
24	I feel like having an implicit "<init>" instead of a struct of some sort is potentially confusing?
	+1

src/wrapper/java_vm/init_args.rs

Line	Notes
46	Could use more doc-tests
50	Silently ignoring unsupported options is a little lame
70	JavaVM::build in doc comments, not new ?
101	Pretty gosh darn heckin' sketchy if you ask me... relies on opts never being modified after this point. Fortunately this type's contents are nice and private/local, so that's enforced.
	+1

src/wrapper/java_vm/vm.rs

Line	Notes
134	unsafe impl Send + Sync - I believe this is safe for JavaVM (as used here), but not for JNIEnv (keep a look out for that later)
150	unsafe { ... } - ptr casts are a bit sketchy, otherwise LGTM.
158	+1
165	unsafe fn - +1
185	attach_current_thread_permanently - possible noop if already attached, meaning it might be temporary!
232	detach_current_thread - doc comments make this sound possibly unsound?
270	unsafe { ... } - +1
280	unsafe { ... } - +1
364	unsafe fn - This... actually looks sound? What am I missing?
386	unsafe fn - This... actually looks sound? What am I missing?
409	unsafe { ... } - +1
	-1 - Can detatched threads cause unsoundness? What am I missing for unsafe fn?

src/wrapper/objects/global_ref.rs

Line	Notes
36	unsafe impl Send + Sync - should be safe?
48	unsafe fn - presumably because this takes a jobject. LGTM.
66	unsafe fn - presumably because this takes a jobject. LGTM.
	+1

src/wrapper/objects/jbytebuffer.rs

Line	Notes
11	jobject is just a pointer, so this general purpouse crate-exported method means using JByteBuffer s is unsafe!
32	there's no guarantee a given JObject is a JByteBuffer, but this succeeds unconditionally.
	-1 - I suspect invalid jobject s will cause soundness issues later

src/wrapper/objects/jlist.rs

Line	Notes
20	Eww, methods cached per-object?
46	jobject -> JObject -> JList can be constructed with an invalid pointer...
69	...making all safe fns on this type unsound. Use of 'safe' _unchecked methods also concerns me.
	-1

src/wrapper/macros.rs

Line	Rating	Notes
...
26	+1	non_null
...
105	+1	java_vm_unchecked - 'unchecked' refers to error codes. unsafe macro, $java_vm must be valid.
132	-1	java_vm_method - I wish this forced the caller to use unsafe { ... }. unsafe macro, $jnienv must be valid.
...

TIL

• Use javah to generate rust, perhaps?
• [build-dependencies]