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

[Dmitry Timofeev]

On Fri, 11 Dec 2020 15:23:20 GMT, Mauro Lacy <github.com+11656534+maurolacy at openjdk.org> wrote:

>>> You can't "register" for the OpenJDK JBS, you are only granted write
>>> access once you have Author status in the OpenJDK project. I will file
>>> an issue but the security-libs folk will need to decide what to do about it.
>> 
>> OK. Please let me know the issue #, so I can keep an eye on it.
>> 
>>> 
>>> With regards to an earlier comment regarding the inconsistent behaviour
>>> - yes this is only a -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.
>> 
>> Fair enough. So, as you said earlier, this is in the end a matter of interpretation of what constitutes correct usage.
>> 
>>> 
>>> Again I will look further into this, on Monday.
>> 
>> Have a nice weekend.
>> 
>>> 
>>> Cheers,
>>> David
>
> 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.

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

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