Skip to content
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

Removes the Options around the function pointers #25

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

rib
Copy link
Contributor

@rib rib commented Jun 21, 2023

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)

--

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.

@rib rib force-pushed the rib/pr/non-exhaustive-jni-native-interface branch from 4fba703 to 802ac85 Compare June 21, 2023 09:26
@rib rib force-pushed the rib/pr/no-function-pointer-options branch from 13c03d9 to 5fb8742 Compare June 21, 2023 09:26
Base automatically changed from rib/pr/non-exhaustive-jni-native-interface to master August 31, 2023 19:43
@rib rib marked this pull request as ready for review August 31, 2023 19:47
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)
@rib rib force-pushed the rib/pr/no-function-pointer-options branch from 5fb8742 to 8139e9f Compare August 31, 2023 19:50
@rib rib merged commit 99e8729 into master Aug 31, 2023
@rib rib deleted the rib/pr/no-function-pointer-options branch August 31, 2023 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant