8266742: Check W^X state on possible safepoint #3920

AntonKozlov · 2021-05-07T15:37:12Z

Hi,

Please review a check for W^X mode at safepoint. As described in the bug, this explicit check aims to catch the wrong W^X mode that may lead to a crash (such crashes are rather intermittent).

This check helped to verify the fix for https://bugs.openjdk.java.net/browse/JDK-8265292 and should help to discover similar issues. It is disabled by default to avoid unexpected failures in the regular use, but eventually it should be turned on unconditionally.

I still owe a complete W^X approach description. Hope this patch does not depend much on that.

Progress

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

Issue

JDK-8266742: Check W^X state on possible safepoint

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Gerard Ziemski (@gerard-ziemski - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3920/head:pull/3920
$ git checkout pull/3920

Update a local copy of the PR:
$ git checkout pull/3920
$ git pull https://git.openjdk.java.net/jdk pull/3920/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3920

View PR using the GUI difftool:
$ git pr show -t 3920

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3920.diff

bridgekeeper · 2021-05-07T15:38:08Z

👋 Welcome back akozlov! 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 · 2021-05-07T15:40:04Z

@AntonKozlov The following label will be automatically applied to this pull request:

hotspot-runtime

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.

mlbridge · 2021-05-07T15:45:28Z

Webrevs

01: Full - Incremental (589db510)
00: Full (f979fd08)

dholmes-ora

Hi Anton,

A few comments.

Thanks,
David

dholmes-ora · 2021-05-10T05:23:29Z

src/hotspot/os/bsd/globals_bsd.hpp

+                         range,                                         \
+                         constraint)                                    \
+                                                                        \
+  AARCH64_ONLY(develop(bool, WXCheckAtSafepoint, false,                 \


Why not enable always as this is a develop build? Do we even need to manage this on a flag?

Also not really "safepoint" as the check also applies to handshakes.

Even in debug builds the check may be too assertive. This check exposes 22 failures in tier1 in jfr, gc, compiler, serviceability, and foreign (hope to be fixed soon), and they look like actual possible issues. So I suggest to have the option but keep it disabled. But if one meets with something similar to JDK-8265292, then it will be possible to reproduce otherwise intermittent crash.

src/hotspot/share/runtime/thread.hpp

AntonKozlov · 2021-05-13T16:31:51Z

Thanks! I've changed the option and the method name.

gerard-ziemski · 2021-05-14T18:13:14Z

src/hotspot/share/runtime/safepointMechanism.inline.hpp

@@ -65,6 +65,16 @@ bool SafepointMechanism::should_process(JavaThread* thread) {
 }

 void SafepointMechanism::process_if_requested(JavaThread* thread) {


Looking at the stack from the crash:

V [libjvm.dylib+0x8c1060] _ZN21MarkActivationClosure12do_code_blobEP8CodeBlob+0x3c V [libjvm.dylib+0x8fea78] _ZN10JavaThread11nmethods_doEP15CodeBlobClosure+0x84 V [libjvm.dylib+0x401100] _ZN14HandshakeState18process_self_innerEv+0x18c V [libjvm.dylib+0x400f60] _ZN14HandshakeState15process_by_selfEv+0x48 V [libjvm.dylib+0x832690] _ZN18SafepointMechanism25process_if_requested_slowEP10JavaThread+0x54 V [libjvm.dylib+0x8fe4b8] _ZN10JavaThread44check_safepoint_and_suspend_for_native_transEPS_+0x24 V [libjvm.dylib+0x92395c] _ZN19ProgrammableInvoker13invoke_nativeEPFvPhES0_P10JavaThread+0x118 V [libjvm.dylib+0x923aa8] PI_invokeNative+0x104 J 2565 jdk.internal.foreign.abi.ProgrammableInvoker.invokeNative(JJ)V jdk.incubator.foreign@17-ea (0 bytes) @ 0x00000001189eeed0

why aren't we adding the assertion closer to the frame that crashed, where we need it, like for example:

HandshakeState::process_self_inner()

If we push the check too low we only do it when we hit this code and there is something to process, but we want to detect when we reach this code in the wrong state even if we wouldn't actually crash at this time due to there not being an active poll.

David

David, thanks, this is exactly how it meant to be.

The crash happens when a thread is synchronized for MarkActivationClosure. Having the check in the HandshakeState::process_self_inner would catch the wrong state when a thread is synchronized also for something else. The patch catches the wrong state if a thread polls synchronization, it does not need to actually synchronize to trigger the check. So we'll catch more.

Thank you for the explanation.

dholmes-ora

Changes looks good to me.

Thanks,
David

openjdk · 2021-05-17T07:03:52Z

@AntonKozlov 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:

8266742: Check W^X state on possible safepoint

Reviewed-by: dholmes, gziemski

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 141 new commits pushed to the master branch:

cf97252: 8264561: javap get NegativeArraySizeException on bad instruction
b8856b1: 8263614: javac allows local variables to be accessed from a static context
ea36836: 8267236: Versioned platform link in TestMemberSummary.java
d5a15f7: 8263438: Unused method AbstractMemberWriter.isInherited
dd5a84c: 8267162: Add jtreg test group definitions for langtools
39a454b: 8260331: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "ERROR: icon and imageIcon not same."
a29612e: 8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException
a555fd8: 8264734: Some SA classes could use better hashCode() implementation
2313a21: 8266637: CHT: Add insert_and_get method
7b736ec: 8266489: Enable G1 to use large pages on Windows when region size is larger than 2m
... and 131 more: https://git.openjdk.java.net/jdk/compare/c665dba591ae5c15c9ca49e14d1aaa4eea38e7ae...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

gerard-ziemski

Looks good, thanks!

AntonKozlov · 2021-05-17T19:13:31Z

Thank you for reviews!

/integrate

openjdk · 2021-05-17T19:15:39Z

@AntonKozlov Since your change was applied there have been 143 commits pushed to the master branch:

79b3944: 8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS
3c010a7: 8265705: aarch64: KlassDecodeMovk mode broken
cf97252: 8264561: javap get NegativeArraySizeException on bad instruction
b8856b1: 8263614: javac allows local variables to be accessed from a static context
ea36836: 8267236: Versioned platform link in TestMemberSummary.java
d5a15f7: 8263438: Unused method AbstractMemberWriter.isInherited
dd5a84c: 8267162: Add jtreg test group definitions for langtools
39a454b: 8260331: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "ERROR: icon and imageIcon not same."
a29612e: 8255661: TestHeapDumpOnOutOfMemoryError fails with EOFException
a555fd8: 8264734: Some SA classes could use better hashCode() implementation
... and 133 more: https://git.openjdk.java.net/jdk/compare/c665dba591ae5c15c9ca49e14d1aaa4eea38e7ae...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3b11d81.

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

Check W^X at safepoint

f979fd0

openjdk bot added the rfr Pull request is ready for review label May 7, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label May 7, 2021

dholmes-ora reviewed May 10, 2021

View reviewed changes

Change option and method name

589db51

gerard-ziemski suggested changes May 14, 2021

View reviewed changes

dholmes-ora approved these changes May 17, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 17, 2021

gerard-ziemski approved these changes May 17, 2021

View reviewed changes

openjdk bot closed this May 17, 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 May 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8266742: Check W^X state on possible safepoint #3920

8266742: Check W^X state on possible safepoint #3920

AntonKozlov commented May 7, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 7, 2021

openjdk bot commented May 7, 2021

mlbridge bot commented May 7, 2021 •

edited

Loading

dholmes-ora left a comment

dholmes-ora May 10, 2021

AntonKozlov May 13, 2021 •

edited

Loading

AntonKozlov commented May 13, 2021

gerard-ziemski May 14, 2021

dholmes-ora May 17, 2021

AntonKozlov May 17, 2021

gerard-ziemski May 17, 2021

dholmes-ora left a comment

openjdk bot commented May 17, 2021 •

edited

Loading

gerard-ziemski left a comment

AntonKozlov commented May 17, 2021

openjdk bot commented May 17, 2021

		@@ -65,6 +65,16 @@ bool SafepointMechanism::should_process(JavaThread* thread) {
		}

		void SafepointMechanism::process_if_requested(JavaThread* thread) {

8266742: Check W^X state on possible safepoint #3920

8266742: Check W^X state on possible safepoint #3920

Conversation

AntonKozlov commented May 7, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 7, 2021

openjdk bot commented May 7, 2021

mlbridge bot commented May 7, 2021 • edited Loading

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora May 10, 2021

Choose a reason for hiding this comment

AntonKozlov May 13, 2021 • edited Loading

Choose a reason for hiding this comment

AntonKozlov commented May 13, 2021

gerard-ziemski May 14, 2021

Choose a reason for hiding this comment

dholmes-ora May 17, 2021

Choose a reason for hiding this comment

AntonKozlov May 17, 2021

Choose a reason for hiding this comment

gerard-ziemski May 17, 2021

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented May 17, 2021 • edited Loading

gerard-ziemski left a comment

Choose a reason for hiding this comment

AntonKozlov commented May 17, 2021

openjdk bot commented May 17, 2021

AntonKozlov commented May 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented May 7, 2021 •

edited

Loading

AntonKozlov May 13, 2021 •

edited

Loading

openjdk bot commented May 17, 2021 •

edited

Loading