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

[David Holmes]

Hi Dmitry, Mauro,

Sorry for the top-post but just to summarise:

This particular issue is only about the GetCritical API in relation to 
the change in JDK-8193234 which introduced the explicit free of the 
internally allocated buffer used only for Xcheck:jni. Through (arguably) 
incorrect usage that internal buffer was causing a memory leak. I will 
try to address the specific double-free problem that relates to this 
issue (8258077).

There is a broader issue of the lack of clear documentation for the 
GetCritical and GetXArrayElements APIs in regards to the correct way to 
use the different modes. I've filed a spec bug for that:

https://bugs.openjdk.java.net/browse/JDK-8258185

There are cases where JNI_COMMIT is being used in the JDK when it 
probably should not e.g. the crypto code. I've filed a bug for that too:

https://bugs.openjdk.java.net/browse/JDK-8258186

Thanks,
David
-----

On 13/12/2020 6:38 am, Dmitry Timofeev wrote:
> On Sat, 12 Dec 2020 20:24:55 GMT, Dmitry Timofeev <github.com+7531824+dmitry-timofeev at openjdk.org> wrote:
> 
>>> One last comment: if `JNI_COMMIT` implies copying and freeing the buffer when it is a copy, then it's no different than mode `0`.
>>
>> Hi David,
>>
>> Thanks for taking a look at the issue! I've investigated this issue with Mauro.
>>
>>>   -Xcheck:jni issue because it is only in that case
>> that any buffer copying every occurs and so freeing actually does
>> something. Otherwise hotspot never copies and the release mode is
>> irrelevant with regard to freeing anything.
>>
>> I might be missing something, but it seems it depends on which method is used:
>> - In `GetCriticalArrayElements`, Hotspot never copies.
>> - In `GetXArrayElements`, Hotspot [always copies](https://github.com/openjdk/jdk/blob/6bc493188b265b1f9776aa62f77c161d75fc591e/src/hotspot/share/prims/jni.cpp#L2579) a Java array (except the case of 0-length array), hence it is essential to call `Release...` with `0` to eventually release the JVM-allocated buffer (or `JNI_ABORT`). `ReleaseXArrayElements` in normal mode, with no extra checks, is consistent with the specification: [it does not free the array on commit](https://github.com/openjdk/jdk/blob/6bc493188b265b1f9776aa62f77c161d75fc591e/src/hotspot/share/prims/jni.cpp#L2631-L2635). For comparison, Android VM [sometimes](https://cs.android.com/android/platform/superproject/+/master:art/runtime/jni/jni_internal.cc;l=2660;drc=master) copies.
>>
>> ---
>>> I'm more inclined today to think that the spec should have made it clear that multiple release calls are needed if you only COMMIT first.
>>
>> I agree that the spec could have made it clearer that a call to `Release...` with `JNI_COMMIT` must be followed by a `Release` with either `JNI_ABORT` or `0`. For example, JNI tips for Android say it explicitly in bold: https://developer.android.com/training/articles/perf-jni#primitive-arrays
>>
>>> Also note that the JNI_COMMIT flag does not release the array, and you will need to call Release again with a different flag eventually.
>>
>> To be fair, the JNI specification does say that most application shall use "0" mode, and other modes are for some advanced use-cases. I checked the usage of GetXArrayElements with JNI_COMMIT in Google code base, and all of the usages actually meant mode "0" and had to be patched. Also, we have a RAII wrapper similar to one in jni-rs with a separate `Commit()` method, and there are no usages of this method :)
>>
>>> But we can't revert JDK-8193234 without reintroducing the leak for anyone using only COMMIT mode - which we've implicitly endorsed now.
>>
>> It seems that for users of `GetXArrayElements` the leak is always there: any code that uses `JNI_COMMIT` _instead of_ `0` leaks on a VM that copies (e.g., on Hotspot and on Android VM). That doesn't seem to be true for the critical flavour, because it does not copy in normal mode, but such client code is still broken according to the spec.
> 
> Also, searched for `JNI_COMMIT` on Github and sampled several results from non-toy projects — all use `JNI_COMMIT` incorrectly instead of `0`. It would be lovely to add a clarification to the spec similar to one in Android JNI tips; and to raise awareness about `Xcheck:jni` mode, possibly, mention it prominently in the spec, so that more people use it to catch issues early in tests.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/1697
>