Add Array wrapper types with lifetimes

[rib]

Overview

Instead of all the array-based APIs being based on sys types like
jbyteArray (which can easily lead to the use of invalid local
references) this introduces types like JByteArray<'local> that
have a lifetime parameter to ensure the local reference can't
be read if it is deleted or goes out of scope.

JByteArray and JIntArray etc are based on a generic
JPrimitiveArray<T> which takes any of the primitive types that
may be put into arrays, like jbyte or jint.

JObjectArray is handled separately, considering that it can't be used
with AutoArray or AutoPrimitiveArray.

The implementations of AutoArray and AutoPrimitiveArray were updated
so they can be created based on a JPrimitiveAarray.

While updating the lifetime parameters to the AutoArray and
AutoPrimitiveArray APIs numerous implementation inconsistencies were
found, even though they should only really differ in how they acquire /
release the pointer to array elements. This patch also normalizes the
implementation of both of these APIs, so they are easily comparable. It
may even be possible to unify these at some point.

Closes: #396
Closes #41

Definition of Done

• There are no TODOs left in the code
• Change is covered by automated tests
• The coding guidelines are followed
• Public API has documentation
• User-visible changes are mentioned in the Changelog
• The continuous integration build passes

[rib]

@argv-minus-one this has a bit of overlap with the kind of thing that was update in #398 and it would be great if you'd be able to take a look over this too if possible

[argv-minus-one]

Nice. My project mostly uses Java byte arrays to move data between Java and Rust, so this is just what I needed. I'll give this a try tomorrow.

[rib]

just to add; I had intended to open this as a "draft" PR but just noticed I must have clicked the wrong button in the end when opening the PR. This was a first pass that I did fairly quickly yesterday but wanted to post here to get some initial feedback. Thanks for taking a look.

[rib]

Something I had meant to highlight too was that I ended up removing the AsRef<JObject<'local>> implementation for AutoArray and AutoPrimitiveArray.

On the one hand I wanted to remove the owned JObjects that were being created for these structs which looked amiss but, more generally, I think of .as_ref() a bit like a type cast and since I don't think either of these types really represent the array itself it feels like a misuse to use .as_ref() to query a kind of property instead of acting like a type cast.

I currently feel like AutoArray and AutoPrimitiveArray are poorly named types because they don't so much represent arrays as much as they represent an auto-release guard for a temporary pointer for accessing the elements of an array.

I think of them more like AutoElements and AutoElementsCritical.

I could see that it might be desirable to be able to query the link between these and the array that's being accessed but I'd think of it like querying a relationship/link to a parent object and I'd expect that to have a more explicit API instead of using .as_ref().

[argv-minus-one]

Here are my thoughts so far. Let me know if you'd like me to work on any of them.

Merge AutoArray and AutoPrimitiveArray

I wonder if we could make JNIEnv::get_primitive_array_critical return AutoArray too, and remove AutoPrimitiveArray. They're pretty similar in API and behavior.

There is one important difference, though: AutoPrimitiveArray holds a &mut JNIEnv, to prevent the caller from making JNI calls inside the critical region, whereas AutoArray contains its own unsafe_cloned JNIEnv. I think this can be dealt with by adding a lifetime parameter 'env to AutoArray that represents:

• If critical, the lifetime of the &mut JNIEnv. This prevents other use of the JNIEnv while the AutoArray is live.
• If not critical, 'static. This allows other use of the JNIEnv while the AutoArray is live.

pub struct AutoArray<'local, 'other_local, 'env, T> {
    env: AutoArrayKind<'local, 'env>,
    ..
}

enum AutoArrayKind<'local, 'env> {
    // Not a critical `AutoArray`. Contains a `JNIEnv` created by `JNIEnv::unsafe_clone`.
    Normal(JNIEnv<'local>),
    
    // A critical `AutoArray`. Mutably borrows the `JNIEnv` it was created with.
    Critical(&'env mut JNIEnv<'local>),
}

impl<'local> JNIEnv<'local> {
    pub fn get_array_elements<'other_local, T>(
        &mut self,
        ..
    ) -> Result<AutoArray<'local, 'other_local, 'static, T>> {_}

    pub fn get_primitive_array_critical<'env, 'other_local, T>(
        &'env mut self,
        ..
    ) -> Result<AutoArray<'local, 'other_local, 'env, T>> {_}
}

The destructor for AutoArray would need to look at what variant self.env is to decide which JNI function to release the array with. For AutoArrayKind::Normal, use Release<Type>ArrayElements; for AutoArrayKind::Critical, use ReleasePrimitiveArrayCritical.

Deref to primitive slice

I'd really like to see an implementation of Deref<Target = [T]> and DerefMut for AutoArray<T> (and AutoPrimitiveArray<T>, if we're keeping it). Currently, there is no way to actually use AutoArray without unsafely dereferencing the raw array pointer.

To make that work, AutoArray::new would need to get the size of the array, convert it to usize, and store that where the Deref implementation can find it along with the array pointer.

Use after free

I see that AutoArray and AutoPrimitiveArray no longer take ownership of the local reference to the array. I agree with you that this is a good idea, but in that case, they should store a borrowed &JPrimitiveArray, not just the raw jarray. Currently, it looks like you can delete the local reference to the array while an AutoArray is still live, causing use-after-free in AutoArray's methods and destructor:

let mut env: JNIEnv;
let array_ref: JPrimitiveArray;

let auto_array = env.get_int_array_elements(&array_ref, _)?;
env.delete_local_ref(array_ref)?;
auto_array.size()?; // UB: uses the local reference after it is freed!

JNIEnv::get_<type>_array_elements

Should the JNIEnv::get_<type>_array_elements methods be removed? I don't see any need for them now that there's a generic JNIEnv::get_array_elements method.

[rib]

For reference, I've made a second pass over this now.

For now I've tried to use a more consistent lifetime name of 'ptr to name the lifetime of the pointer to the array elements.

I'll look at splitting out the change to add as_raw() methods to types like JString which I added for consistency.

This iteration also ends up making quite a lot of changes to both AutoArray and AutoPrimitiveArray for the sake of normalizing their implementation, and I was also starting to consider that they could potentially be unified later through the TypeArray trait.

[argv-minus-one]

JString already has an as_raw method from Deref<Target = JObject>.

[rib]

Merge AutoArray and AutoPrimitiveArray

I wonder if we could make JNIEnv::get_primitive_array_critical return AutoArray too, and remove AutoPrimitiveArray. They're pretty similar in API and behavior.

There is one important difference, though: AutoPrimitiveArray holds a &mut JNIEnv, to prevent the caller from making JNI calls inside the critical region, whereas AutoArray contains its own unsafe_cloned JNIEnv. I think this can be dealt with by adding a lifetime parameter 'env to AutoArray that represents:

Yeah, I was having similar thoughts.

I was also pondering whether the API to access elements with a "critical" section should simply be marked as unsafe (along with clearer docs explaining that JNI can't be used in the critical section) instead of holding mutable env reference to try and block any use of JNI. In the documentation for GetPrimitiveArrayCritical there is a notable exception that allows nested critical sections in case you want to access multiple arrays, and if we take the mutable reference to the env then we limit that use case.

[rib]

JString already has an as_raw method from Deref<Target = JObject>.

ah, thanks for the reminder. I guess I was initially adding as_raw() implementations that would return the more specific sys types for the arrays and then thought it was maybe an oversight that we hadn't added as_raw() methods for JString and JThrowable. I've dropped those now.

[rib]

I see that AutoArray and AutoPrimitiveArray no longer take ownership of the local reference to the array. I agree with you that this is a good idea, but in that case, they should store a borrowed &JPrimitiveArray, not just the raw jarray. Currently, it looks like you can delete the local reference to the array while an AutoArray is still live, causing use-after-free in AutoArray's methods and destructor:

Ah yep, makes sense.

[argv-minus-one]

I notice that this PR also contains 910ee0d and eb82c38, which don't have anything to do with arrays. Is that intentional? If not, you can remove them from this branch with git rebase --onto master eb82c38 (assuming your local master is the same as what's in this GitHub repository).

[rib]

I notice that this PR also contains 910ee0d and eb82c38, which don't have anything to do with arrays. Is that intentional? If not, you can remove them from this branch with git rebase --onto master eb82c38 (assuming your local master is the same as what's in this GitHub repository).

Yeah, sorry for the distraction - I originally noted this in the PR description. When I first made the PR it was intended to be a draft to test out the feasibility of adding these wrapper types and I just did that based on the branch for #399. I used that branch as a base mainly for the sake of being based on the non-Copy local ref changes which would have led to lots of conflicts to rebase. I've just rebased on master.

[rib]

@argv-minus-one I've just added a patch that implements Deref and DerefMut and updated the tests to check these. This makes it so the array length is implicitly called during ::new() to ensure we always know the array length upfront. These internally call a new_with_len() constructor in case we ever want to create an AutoArray with a pre-determined length.

.size() is replaced with .len() that doesn't return a Result.

[rib]

Regarding the idea to try and consolidate AutoArray and AutoPrimitiveArray I'm currently a little reluctant to look at that here currently (though still open to experimenting later via a separate PR) my current gut feeling is that it may end up kind of klunky / over complicated having to deal with the env ownership differences. It's also notable that .commit() is another difference since JNI_COMMIT is fraught with issues across different JNI implementations when using critical sections.

That said - I'd be very tempted to rename AutoPrimitiveArray to something like AutoArrayCritical to at least try and better represent the difference between these two APIs.

[argv-minus-one]

I've changed my project to use these new features, and they work very nicely.

I did need to use the bytemuck library to get a [u8] from a Java byte array, which normally gives [i8]. I imagine this will be a common need whenever using Java byte arrays to move serialized data across JNI, so it might be worth recommending bytemuck in the documentation.

[maurolacy]

Regarding the idea to try and consolidate AutoArray and AutoPrimitiveArray I'm currently a little reluctant to look at that here currently (though still open to experimenting later via a separate PR) my current gut feeling is that it may end up kind of klunky / over complicated having to deal with the env ownership differences.

I remember trying to do that at the time, and in the end deciding that it was simpler / clearer to have just two different impls.

It's also notable that .commit() is another difference since JNI_COMMIT is fraught with issues across different JNI implementations when using critical sections.

Yes. JNI_COMMIT is not even consistent in openjdk, depending on the flags, in particular -Xcheck:jni.

That said - I'd be very tempted to rename AutoPrimitiveArray to something like AutoArrayCritical to at least try and better represent the difference between these two APIs.

Go ahead. I tried to come up with representative names at the time, which is not easy; having Critical in the name sounds like a good idea.

[rib]

Okey, not really wanting to bike shed naming but still; @argv-minus-one + @maurolacy can you let me know if you'd be ok with the names AutoElements / AutoElementsCritical or would you rather go with the more similar AutoArray / AutoArrayCritical names?

Personally I think that the use of Array in the name is a little misleading - more so now that this PR adds JPrimitiveArray + JByteArray/JIntArray etc as a wrapper type for Array references.

The AutoArray type doesn't own the array itself - it's an auto-release guard that gives temporary access to the elements of an array so I tend to think that AutoElements / AutoElementsCritical help to intuitively differentiate what these are for compared to JPrimitiveArray + JByteArray etc.

Please react with 🎉 for AutoArray / AutoArrayCritical or 🚀 for AutoElements / AutoElementsCritical

[maurolacy]

Please react with 🎉 for AutoArray / AutoArrayCritical or 🚀 for AutoElements / AutoElementsCritical

Both work for me, but I tend to stick with 'Array' for historical reasons.

[rib]

I've changed my project to use these new features, and they work very nicely.

I did need to use the bytemuck library to get a [u8] from a Java byte array, which normally gives [i8]. I imagine this will be a common need whenever using Java byte arrays to move serialized data across JNI, so it might be worth recommending bytemuck in the documentation.

It could even be worth adding a sys type for casting bytes as unsigned. If we added a jubyte that would allow Rust code to interpret any Java byte as unsigned if that's what makes sense.

[rib]

Okey, I've rebased on master, squashed the doc fixes and dropped the patch that made get_array_elements[_critical] borrow the array mutably (and updated the CHANGELOG, MIGRATION guide and docs accordingly)

Hopefully this is ok to land now @argv-minus-one

[argv-minus-one]

I've pushed two commits to your branch:

• 1b56c74 adds a missing unsafe to the JNIEnv::get_array_elements example in the migration guide.
• de909b1 changes the changelog and migration guide to avoid implying that AutoElements can be used without unsafe, which is unfortunately no longer true.

I've also added a comment at src/wrapper/jnienv.rs line 1750.

Everything else looks good to me.