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

[Mauro Lacy]

On Fri, 11 Dec 2020 11:33:41 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.
>> 
>> 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.
>
>> 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
> 
> I've taken a look and yes, I think you're right. The line
> `        (*env)->ReleasePrimitiveArrayCritical(env, jOut, outBufP, JNI_COMMIT);`
> 
> should probably use `JNI_ABORT` if there's an error, and `0` if it's a normal cleanup.
> 
> On second look, `0` looks like the obvious choice. If there's a jump due to an error, `outBufP` will be `NULL`, and the line wouldn't be executed in the first place.

I'll send a PR for that cryptoki fix too, if you like. I'll now try to register into the issue tracker, so I can create a proper issue for it.

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

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