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

[Daniel D.Daugherty]

On Thu, 17 Dec 2020 11:45:53 GMT, David Holmes <dholmes at openjdk.org> wrote:

> The fix in JDK-8193234 had an unintended consequence for the Release<X>ArrayElements API, which is now fixed in this issue. 
> 
> I'd like to thank Mauro Lacy and Dmitry Timofeev for raising, analysing and discussing this issue. You can follow the thread here:
> 
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2020-December/047248.html
> 
> Although the fix itself is very basic I found a couple of other issues along the way, so I have staged the commits as follows for ease of understanding:
> 
> Step 1: added a regression test for the current double-free problem
> Step 2: Only try to print the GuardedMemory info in debug builds as it can lead to secondary crashes
> Step 3: Fix incorrect function names in the error messages and cleanup formatting
> Step 4: Revert the change from JDK-8193234
> Step 5: Add the memory-leak test from JDK-8193234 as a manual test
> Step 6: Fix the JNI_COMMIT memory leak only for the case of ReleasePrimitiveArrayCritical
> 
> Finally I had to tweak the test to fix a nativepath problem. 
> 
> Testing:
>  - tiers 1-4 (tier 4 includes the test run with -Xcheck:jni)
>  - local testing of the new tests
> 
> Thanks,
> David

I only have nits so it's your call on whether to make the changes.
I like the new tests! Thanks for doing that. In what Tier do these
new tests execute? Tier4 with "-Xcheck:jni" is a given, but do
they also run in an earlier Tier or two?

test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseArrayElements.java line 113:

> 111:             }
> 112:         }
> 113:         for (int i = count; i <source.length; i++) {

nit - need space after '<'.

test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseArrayElements.java line 79:

> 77:             fill(arr, start, sliceLength);
> 78:             System.out.println("Array during: " + Arrays.toString(arr));
> 79:             check(arr, (i+1) * sliceLength);

nit - need spaces around '+'.

test/hotspot/jtreg/runtime/jni/checked/libTestCheckedReleaseCriticalArray.c line 44:

> 42:     (*env)->ReleasePrimitiveArrayCritical(env, iarr, arr, JNI_COMMIT);
> 43:   }
> 44:   // we skip the test is the VM makes a copy - as it will definitely leak

typo - s/is the/if the/

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

Marked as reviewed by dcubed (Reviewer).

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