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

[David Holmes]

Hi Mauro,

On 17/12/2020 6:12 pm, Mauro Lacy wrote:
> On Tue, 15 Dec 2020 08:18:28 GMT, Mauro Lacy <github.com+11656534+maurolacy at openjdk.org> wrote:
> 
>>> 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 the `-Xcheck:jni` flag is set or not. This is inconsistent behaviour. Probably introduced when trying to satisfy both usage patterns.
>>>
>>> Besides, I don't think the same is true of the other `Release` methods. This happens only with the `Critical` variant.
>>
>>> Besides, I don't think the same is true of the other `Release` methods. This happens only with the `Critical` variant.
>>
>> Sorry for (contributing to) the confusion. The reported double free error always mentions `ReleasePrimitiveArrayCritical`, but as Dmitry says, this ends up affecting both APIs.
> 
> Hi David,
> 
> Thanks for researching this thoroughly.
> What you propose is reasonable, given the circumstances: undocumented or badly documented usage in the specs, and a lot of existing code (even in books, etc) that use `JNI_COMMIT` basically as `0`. We faced the same weeks ago, when looking for examples, documentation, and use cases for this functionality.
> 
> A couple comments:
> - Given that `JNI_COMMIT` is then effectively useless, maybe a good idea would be to deprecate it. This will break existing code, true, but a deprecation notice can be used, by example in the form of a compilation warning or so, for code that calls / uses it. Same in the docs, i. e. this is kept for historical reasons but will be deprecated soon, and so on.

Not withstanding the lack of clarity in the spec I think JNI_COMMIT 
still has a theoretical use whenever a copy is made. But we will look at 
issues of deprecation etc in the context of JDK-8258185.

> - I think in this case the best for us would be to simply drop support for `JNI_COMMIT` in the Rust JNI library. At least, for the ArrayCritical methods.

That sounds good.

Thank you very much for raising this issue and working through the details.

David
-----

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