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

dholmes-ora · 2020-12-17T11:45:53Z

The fix in JDK-8193234 had an unintended consequence for the ReleaseArrayElements 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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

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

Reviewers

Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to 8786d58
Harold Seigel (@hseigel - Reviewer) ⚠️ Review applies to b989bc5

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1816/head:pull/1816
$ git checkout pull/1816

Step 1: added a regression test

…t can trigger a secondary crash.

…p formatting.

…imitiveArrayCritical

bridgekeeper · 2020-12-17T11:46:37Z

👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-12-17T11:47:26Z

@dholmes-ora The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

dholmes-ora · 2020-12-17T11:49:39Z

/label remove hotspot
/label add hotspot-runtime

openjdk · 2020-12-17T11:49:56Z

@dholmes-ora
The hotspot label was successfully removed.

openjdk · 2020-12-17T11:50:26Z

@dholmes-ora
The hotspot-runtime label was successfully added.

mlbridge · 2020-12-17T11:54:12Z

Webrevs

dholmes-ora · 2020-12-24T05:42:14Z

/help

openjdk · 2020-12-24T05:42:38Z

@dholmes-ora Available commands:

cc - add or remove an additional classification label
contributor - adds or removes additional contributors for a PR
covered - used when employer has signed the OCA
csr - require a compatibility and specification request (CSR) for this pull request
help - shows this text
integrate - performs integration of the changes in the PR
issue - edit the list of issues that this PR solves
label - add or remove an additional classification label
reviewer - manage additional reviewers for a PR
reviewers - set the number of additional required reviewers for this PR
signed - used after signing the OCA
solves - edit the list of issues that this PR solves
sponsor - performs integration of a PR that is authored by a non-committer
summary - updates the summary in the commit message
test - used to run tests

mlbridge · 2020-12-24T06:09:57Z

Mailing list message from David Holmes on hotspot-runtime-dev:

Ping!

Thanks,
David

On 17/12/2020 9:54 pm, David Holmes wrote:

dcubed-ojdk

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?

dcubed-ojdk · 2020-12-24T17:58:55Z

test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseArrayElements.java

+                                           i + " but got " + source[i]);
+            }
+        }
+        for (int i = count; i <source.length; i++) {


nit - need space after '<'.

dcubed-ojdk · 2020-12-24T17:59:42Z

test/hotspot/jtreg/runtime/jni/checked/TestCheckedReleaseArrayElements.java

+            int start = i * sliceLength;
+            fill(arr, start, sliceLength);
+            System.out.println("Array during: " + Arrays.toString(arr));
+            check(arr, (i+1) * sliceLength);


nit - need spaces around '+'.

dcubed-ojdk · 2020-12-24T18:02:38Z

test/hotspot/jtreg/runtime/jni/checked/libTestCheckedReleaseCriticalArray.c

+    // write-back using JNI_COMMIT to test for memory leak
+    (*env)->ReleasePrimitiveArrayCritical(env, iarr, arr, JNI_COMMIT);
+  }
+  // we skip the test is the VM makes a copy - as it will definitely leak


typo - s/is the/if the/

openjdk · 2020-12-24T18:07:13Z

@dholmes-ora This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: dcubed, hseigel

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2020-12-29T07:15:57Z

Mailing list message from David Holmes on hotspot-runtime-dev:

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

mlbridge · 2021-01-04T21:25:02Z

Mailing list message from David Holmes on hotspot-runtime-dev:

Ping! Can I get a second review please.

Thanks,
David

On 29/12/2020 5:10 pm, David Holmes wrote:

hseigel

These changes look good! Are any changes needed to the JNI Spec?
Thanks, Harold

mlbridge · 2021-01-06T13:18:57Z

Mailing list message from David Holmes on hotspot-runtime-dev:

On 6/01/2021 2:49 am, Harold Seigel wrote:

On Tue, 29 Dec 2020 07:10:20 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

David Holmes has updated the pull request incrementally with one additional commit since the last revision:

Fix minor nits from Dan's review.

These changes look good! Are any changes needed to the JNI Spec?

Thanks for the review Harold. These changes don't impact the JNI spec,
but there is a separate issue:

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

to examine what the spec says in relation to these modes.

David

dholmes-ora · 2021-01-08T04:10:45Z

/integrate

openjdk · 2021-01-08T04:11:48Z

@dholmes-ora Pushed as commit 712014c.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

dholmes-ora added 7 commits December 16, 2020 23:43

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

d309214

Step 1: added a regression test

Step 2: Only try to print the GuardedMemory info in debug builds as i…

97789c7

…t can trigger a secondary crash.

Step 3: Fix incorrect function names in the error messages and cleanu…

12e4c18

…p formatting.

Step 4: Revert the change from JDK-8193234

9a58386

Step 5: Add the memory-leak test from JDK-8193234 as a manual test

787096d

Step 6: Fix the JNI_COMMIT memory leak only for the case of ReleasePr…

d2417af

…imitiveArrayCritical

Updated test to fix nativepath problem.

8786d58

openjdk bot added the hotspot hotspot-dev@openjdk.org label Dec 17, 2020

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Dec 17, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 17, 2020

dholmes-ora marked this pull request as ready for review December 17, 2020 11:50

openjdk bot added the rfr Pull request is ready for review label Dec 17, 2020

dcubed-ojdk approved these changes Dec 24, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 24, 2020

Fix minor nits from Dan's review.

b989bc5

hseigel approved these changes Jan 5, 2021

View reviewed changes

Merge branch 'master' into 8258077

507e930

dholmes-ora mentioned this pull request Jan 8, 2021

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

Closed

3 tasks

openjdk bot closed this Jan 8, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 8, 2021

dholmes-ora deleted the 8258077 branch January 8, 2021 04:12

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

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

dholmes-ora commented Dec 17, 2020 •

edited by openjdk bot

bridgekeeper bot commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

dholmes-ora commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

mlbridge bot commented Dec 17, 2020 •

edited

dholmes-ora commented Dec 24, 2020

openjdk bot commented Dec 24, 2020

mlbridge bot commented Dec 24, 2020

dcubed-ojdk left a comment

dcubed-ojdk Dec 24, 2020

dcubed-ojdk Dec 24, 2020

dcubed-ojdk Dec 24, 2020

openjdk bot commented Dec 24, 2020 •

edited

mlbridge bot commented Dec 29, 2020

mlbridge bot commented Jan 4, 2021

hseigel left a comment

mlbridge bot commented Jan 6, 2021

dholmes-ora commented Jan 8, 2021

openjdk bot commented Jan 8, 2021

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

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

Conversation

dholmes-ora commented Dec 17, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

dholmes-ora commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

openjdk bot commented Dec 17, 2020

mlbridge bot commented Dec 17, 2020 • edited

Webrevs

dholmes-ora commented Dec 24, 2020

openjdk bot commented Dec 24, 2020

mlbridge bot commented Dec 24, 2020

dcubed-ojdk left a comment

Choose a reason for hiding this comment

dcubed-ojdk Dec 24, 2020

Choose a reason for hiding this comment

dcubed-ojdk Dec 24, 2020

Choose a reason for hiding this comment

dcubed-ojdk Dec 24, 2020

Choose a reason for hiding this comment

openjdk bot commented Dec 24, 2020 • edited

mlbridge bot commented Dec 29, 2020

mlbridge bot commented Jan 4, 2021

hseigel left a comment

Choose a reason for hiding this comment

mlbridge bot commented Jan 6, 2021

dholmes-ora commented Jan 8, 2021

openjdk bot commented Jan 8, 2021

dholmes-ora commented Dec 17, 2020 •

edited by openjdk bot

mlbridge bot commented Dec 17, 2020 •

edited

openjdk bot commented Dec 24, 2020 •

edited