JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads #1695

tstuefe · 2020-12-08T13:35:24Z

In our primary hotspot signal handlers, SafeFetch handling is limited to JavaThread objects:

  JavaThread* thread = NULL;
...
  if(t->is_Java_thread()) {
    thread = (JavaThread*)t;
  }
...
  if (info != NULL && uc != NULL && thread != NULL) {
    pc = (address) os::Linux::ucontext_get_pc(uc);
    if (StubRoutines::is_safefetch_fault(pc)) {

As a result of this, using SafeFetch may crash non-JavaThreads if the location is invalid. E.g. using SafeFetch inside a VMOperation may crash the VM.

This is unfortunate since SafeFetch is used for os::is_readable_pointer() which explicitly promises to not crash. It is used e.g. in os::print_hex_dump(). There is also no reason why SafeFetch would not work for non-JavaThreads. In fact, SafeFetch handling for the secondary signal handler works just fine for all threads.

The patch makes handling of SafeFetch faults independent on whether the crashing thread is a JavaThread (indeed, whether we have a current Thread at all). This had been the case for AIX and Linux ppc, s390 before, since we already fixed this issue for our platform, so we know this works.

I also hauled the SafeFetch handling out of the platform dependent part of the signal handler into the generic signal handler. This removes some duplicate coding.

To be consistent, I moved the SafeFetch handling for Zero up into the generic signal handler too. Zero did not have a problem, but this reduces code.

I added a gtest which reproduces the issue and used that to check that the patch works.

Thanks, Thomas

Progress

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

Issue

JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads

Reviewers

Martin Doerr (@TheRealMDoerr - Reviewer) ⚠️ Review applies to a7a9432
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to eb0efc1
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to a7a9432
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to eb0efc1

Download

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

bridgekeeper · 2020-12-08T13:35:59Z

👋 Welcome back stuefe! 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-08T13:36:43Z

@tstuefe 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 · 2020-12-08T14:42:22Z

Webrevs

TheRealMDoerr

Thank you for fixing it!
Good that we have signals_posix.cpp, now.
Seems like os::min_page_size() is an address which is not readable on any OS we have. So test looks also good to me.

openjdk · 2020-12-09T09:59:50Z

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

8257828: SafeFetch may crash if invoked in non-JavaThreads

Reviewed-by: mdoerr, kbarrett, coleenp, dholmes

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

e118292: 8258040: Reenable fixed problemlisted test
74b79c6: 8257964: Broken Calendar#getMinimalDaysInFirstWeek with java.locale.providers=HOST
f9c9bf0: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner
8273514: 8166026: Refactor java/lang shell tests to java
ff75ad5: 8258059: Clean up MethodData::profile_unsafe
b5592c0: 8257970: Remove julong types in os::limit_heap_by_allocatable_memory
b28b094: 8257145: Performance regression with -XX:-ResizePLAB after JDK-8079555
fa20186: 8257676: Simplify WeakProcessorPhase
b90b7f5: 8196090: javax/swing/JComboBox/6559152/bug6559152.java fails
1d15ebe: 8243205: Modularize JVM flags declaration
... and 69 more: https://git.openjdk.java.net/jdk/compare/35e81536e79fa31ecd2cf2267d64801f067475b2...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.

kimbarrett

if (info != NULL && uc != NULL && thread != NULL) {

So the cause of the problem is the thread != NULL in that test. And just
removing all of those would fix the problem.

But I agree that eliminating all the (near) copied variants and merging this
functionality into signals_posix is good.

This is unfortunate since SafeFetch is used [...]

Another place this hits is OopStorage, which uses SafeFetchN in two places.
Correct code shouldn't have a problem with one of them because the access
won't be invalid (if code is correct). But the other could be a problem,
though I think not seen in practice.

Might there be any similar problem on Windows?

This might be kind of related?
https://bugs.openjdk.java.net/browse/JDK-8185734

coleenp

This looks good to me. Moving this to the posix signal handler is so nice.

dholmes-ora

Hi Thomas,
This looks good - I like the common handling (though I dislike the need for the zero case :( ).
Thanks,
David

tstuefe · 2020-12-12T07:24:21Z

Hi,

I made this work on Windows too (thanks @kimbarrett for reminding me). This involved fixing HandleException, which is
roughly the pendant of ucontext_set_pc() for Windows. I also combined dumplicate coding for the three Windows architectures.

I excluded the os_safefetch_negative gtest for Windows, that won't work as long as we miss SEH for gtests, see https://bugs.openjdk.java.net/browse/JDK-8185734. This is not a big deal though since the os_safefetch_negative_at_safepoint gtest still works, which tests SafeFetch in the VMThread. I experimented with locally adding a SEH catcher around the SafeFetch call in the test, but I removed that coding because it introduced too much complexity for little gain.

I also fixed a small issue for AIX where the zero page is readable so we need another guaranteed-to-be-invalid address. Since we need invalid addresses in a number of places in text code, I plan to centralize this definition at some point.

Thanks, Thomas

kimbarrett

New Windows changes look okay to me, but I'm not an expert in that area.

dholmes-ora

One query, but changes can go in as-is.
Thanks,
David

dholmes-ora · 2020-12-14T04:44:43Z

src/hotspot/os/windows/os_windows.cpp

+  if (thread != nullptr && thread->is_Java_thread()) {
+    thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->PC_NAME);


Is this a full fix? If the thread is not a JavaThread we no longer hit a problem in as_Java_thread() but is doing nothing for a non-JavaThread actually the right thing to do?

The saved_exception_pc mechanism is only implemented for JavaThreads. But all threads go through the code below, where we change the pc in the context structure.

mlbridge · 2020-12-14T07:35:58Z

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

On 14/12/2020 3:24 pm, Thomas Stuefe wrote:

On Mon, 14 Dec 2020 04:44:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:

Make SafeFetch work on Windows + AIX fix

src/hotspot/os/windows/os_windows.cpp line 2258:

2256: // Save pc in thread
2257: if (thread != nullptr && thread->is_Java_thread()) {
2258: thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->PC_NAME);

Is this a full fix? If the thread is not a JavaThread we no longer hit a problem in as_Java_thread() but is doing nothing for a non-JavaThread actually the right thing to do?

The saved_exception_pc mechanism is only implemented for JavaThreads. But all threads go through the code below, where we change the pc in the context structure.

My query is whether it makes sense for only JavaThreads to have this
saved (C++?) exception pc? What role does it play and why would it not
be applicable to all threads? (Sorry bit of a digression but trying to
understand the bigger picture.)

Thanks,
David

tstuefe · 2020-12-14T08:32:08Z

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

On 14/12/2020 3:24 pm, Thomas Stuefe wrote:

On Mon, 14 Dec 2020 04:44:43 GMT, David Holmes wrote:

Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
Make SafeFetch work on Windows + AIX fix

src/hotspot/os/windows/os_windows.cpp line 2258:

2256: // Save pc in thread
2257: if (thread != nullptr && thread->is_Java_thread()) {
2258: thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->PC_NAME);

Is this a full fix? If the thread is not a JavaThread we no longer hit a problem in as_Java_thread() but is doing nothing for a non-JavaThread actually the right thing to do?

The saved_exception_pc mechanism is only implemented for JavaThreads. But all threads go through the code below, where we change the pc in the context structure.

My query is whether it makes sense for only JavaThreads to have this
saved (C++?) exception pc? What role does it play and why would it not
be applicable to all threads? (Sorry bit of a digression but trying to
understand the bigger picture.)

Thanks,
David

IIUC, JavaThread::_saved_exception_pc contains the pc from which a safepoint was triggered, and to which we may want to return once the safepoint was handled. Safepoint handling only works for java threads, so limiting this to JavaThread makes sense.

mlbridge · 2020-12-14T23:59:59Z

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

On 14/12/2020 6:34 pm, Thomas Stuefe wrote:

On Mon, 14 Dec 2020 04:46:25 GMT, David Holmes <dholmes at openjdk.org> wrote:

Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:

Make SafeFetch work on Windows + AIX fix

One query, but changes can go in as-is.
Thanks,
David

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-runtime-dev](mailto:hotspot-runtime-dev at openjdk.java.net):_

On 14/12/2020 3:24 pm, Thomas Stuefe wrote:

On Mon, 14 Dec 2020 04:44:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
Make SafeFetch work on Windows + AIX fix

src/hotspot/os/windows/os_windows.cpp line 2258:

2256: // Save pc in thread
2257: if (thread != nullptr && thread->is_Java_thread()) {
2258: thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->PC_NAME);

Is this a full fix? If the thread is not a JavaThread we no longer hit a problem in as_Java_thread() but is doing nothing for a non-JavaThread actually the right thing to do?

The saved_exception_pc mechanism is only implemented for JavaThreads. But all threads go through the code below, where we change the pc in the context structure.

My query is whether it makes sense for only JavaThreads to have this
saved (C++?) exception pc? What role does it play and why would it not
be applicable to all threads? (Sorry bit of a digression but trying to
understand the bigger picture.)

Thanks,
David

IIUC, JavaThread::_saved_exception_pc contains the pc from which a safepoint was triggered, and to which we may want to return once the safepoint was handled. Safepoint handling only works for java threads, so limiting this to JavaThread makes sense.

I see now that it relates to the safepoint polling page exception.

Thanks,
David
-----

tstuefe · 2020-12-15T07:00:24Z

/integrate

openjdk · 2020-12-15T07:01:16Z

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

381021a: Merge
afc4441: 8258094: AIX build fails after 8257602
164c8a6: 8258092: Link to early access platform documentation in TestHtmlTableTags.java
43dc3f7: 8254350: CompletableFuture.get may swallow InterruptedException
6d79ec8: 8258111: Problemlist compiler/blackhole tests for -Xcomp until JDK-8258101 is fixed
2001da3: 8257596: Clarify trusted final fields for record classes
b1afed7: 8257919: [JVMCI] profiling info didn't change after reprofile
b7ac32d: 8257598: Clarify what component values are used in Record::equals
a280182: 8258060: Update @jls tags for renamed/renumbered sections
bacf22b: 8256641: CDS VM operations do not lock the heap
... and 89 more: https://git.openjdk.java.net/jdk/compare/35e81536e79fa31ecd2cf2267d64801f067475b2...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3ab1dfe.

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

Initial

a7a9432

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

tstuefe marked this pull request as ready for review December 8, 2020 14:38

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

TheRealMDoerr approved these changes Dec 9, 2020

View reviewed changes

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

kimbarrett approved these changes Dec 9, 2020

View reviewed changes

coleenp approved these changes Dec 9, 2020

View reviewed changes

dholmes-ora approved these changes Dec 10, 2020

View reviewed changes

Make SafeFetch work on Windows + AIX fix

eb0efc1

kimbarrett approved these changes Dec 12, 2020

View reviewed changes

dholmes-ora approved these changes Dec 14, 2020

View reviewed changes

Add missing include

95e3a17

openjdk bot closed this Dec 15, 2020

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 Dec 15, 2020

tstuefe deleted the JDK-8257828-safefetch-in-non-java-threads branch January 15, 2021 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads #1695

JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads #1695

tstuefe commented Dec 8, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 8, 2020

openjdk bot commented Dec 8, 2020

mlbridge bot commented Dec 8, 2020 •

edited

Loading

TheRealMDoerr left a comment

openjdk bot commented Dec 9, 2020 •

edited

Loading

kimbarrett left a comment

coleenp left a comment

dholmes-ora left a comment

tstuefe commented Dec 12, 2020

kimbarrett left a comment

dholmes-ora left a comment

dholmes-ora Dec 14, 2020

tstuefe Dec 14, 2020

mlbridge bot commented Dec 14, 2020

tstuefe commented Dec 14, 2020

mlbridge bot commented Dec 14, 2020

tstuefe commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

		if (thread != nullptr && thread->is_Java_thread()) {
		thread->as_Java_thread()->set_saved_exception_pc((address)(DWORD_PTR)exceptionInfo->ContextRecord->PC_NAME);

JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads #1695

JDK-8257828: SafeFetch may crash if invoked in non-JavaThreads #1695

Conversation

tstuefe commented Dec 8, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 8, 2020

openjdk bot commented Dec 8, 2020

mlbridge bot commented Dec 8, 2020 • edited Loading

Webrevs

TheRealMDoerr left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 9, 2020 • edited Loading

kimbarrett left a comment

Choose a reason for hiding this comment

coleenp left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

tstuefe commented Dec 12, 2020

kimbarrett left a comment

Choose a reason for hiding this comment

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Dec 14, 2020

Choose a reason for hiding this comment

tstuefe Dec 14, 2020

Choose a reason for hiding this comment

mlbridge bot commented Dec 14, 2020

tstuefe commented Dec 14, 2020

mlbridge bot commented Dec 14, 2020

tstuefe commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

tstuefe commented Dec 8, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 8, 2020 •

edited

Loading

openjdk bot commented Dec 9, 2020 •

edited

Loading