jni-rs / jni-sys

Apache License 2.0
55 stars 20 forks source link

Removes the Options around the function pointers #25

Closed rib closed 1 year ago

rib commented 1 year ago

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:

https://github.com/jni-rs/jni-rs/issues/432#issuecomment-1470934365

--

For now I've just created this as a draft PR since I also think it could be worth considering changing JNINativeInterface_ into a union that would look something like:

union JNINativeInterface_ {
    all: JNINativeInterface_all_,
    1_1: JNINativeInterface_1_1_,
    1_2: JNINativeInterface_1_2_,
    ...
}

So then it becomes clearer that you need to access the function pointers according to the known version and in doing so you gain some safety because Rust can stop you from trying to call a JNI 1.2 method while dereferencing the .1_1 functions.

The .1_1 table would also include the padding for the reserved null functions in the middle of the table but can rename them so it would be harder to accidentally dereference them.