RFR: 8258077: Using -Xcheck:jni can lead to a double-free after JDK-8193234

[Mauro Lacy]

On Fri, 11 Dec 2020 08:17:13 GMT, Mauro Lacy <github.com+11656534+maurolacy at openjdk.org> wrote:

>> There are no tests for this, this was about external users of JNI encountering the leak.  I also just re-checked the JDK source and it seems this code also suffers from the COMMIT-only problem:
>> 
>>  ./jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c
>> 
>> The debate is over what constitutes correct and incorrect usage here - as I just wrote in the bug report. I will see if there is a reasonable way to support both.
>
>> There are no tests for this, this was about external users of JNI encountering the leak. I also just re-checked the JDK source and it seems this code also suffers from the COMMIT-only problem:
>> 
>> ./jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_crypt.c
>> 
>> The debate is over what constitutes correct and incorrect usage here - as I just wrote in the bug report.
> 
> Yes. The fact that the method name starts with `Release`, and is being used for `COMMIT` doesn't help in that regard.
> This is probably for another place / discussion, but IMO that functionality should be in different API calls (`CommitPrimitiveArrayCritical`, and `Commit<Type>ArrayElements`) which do just that: commit the buffer changes.
> The use of `JNI_COMMIT` in `ReleasePrimitiveArrayCritical` and `Release<Type>ArrayElements` is misleading, to say the least.
> 
> In fact, in the Rust lib we just separated those into different methods.
> Ref. [ReleaseMode](https://github.com/jni-rs/jni-rs/blob/master/src/wrapper/objects/release_mode.rs) vs. [commit()](https://github.com/jni-rs/jni-rs/blob/ff6aafd98974fb8d9fff2eac4a05bf477002af09/src/wrapper/objects/auto_primitive_array.rs#L50).
> 
>> I will see if there is a reasonable way to support both.
> 
> I don't think so. Not consistently, at least.

David, I can't comment in the issue page (AFAIK), but I agree with what you wrote there.

Just one more thing: The behaviour of `ReleasePrimitiveArrayCritical` with `JNI_COMMIT` **changes** depending on if you use the `-Xcheck:jni` flag or not. This is inconsistent behaviour. Probably introduced when trying to satisfy both use cases.

Besides, I don't think the same is true of the other `Release` methods. This happens only with the `Critical` variant.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1697