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

[David Holmes]

Hi Dan,

Thanks for the Review.

On 25/12/2020 4:08 am, Daniel D.Daugherty wrote:
> 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.

All nits fixed.

> 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?

TestCheckedReleaseCriticalArray won't execute in any tier as it is a 
manual test.

TestCheckedReleaseArrayElements runs in tier1 like the majority of 
runtime tests. It likely runs in tier4 as well but that isn't really 
relevant as it already sets -Xcheck:jni on the launched VM.

Thanks,
David

> 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
>