8297967: Make frame::safe_for_sender safer #11461

parttimenerd · 2022-12-01T16:47:48Z

Makes frame::safe_for_sender safer by checking that the location of the return address, sender stack pointer, and link address is accessible. This makes the method safer in the case of broken frames.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Integration blocker

⚠️ Too few reviewers with at least role reviewer found (have 0, need at least 1) (failed with the updated jcheck configuration)

Issue

JDK-8297967: Make frame::safe_for_sender safer (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/11461/head:pull/11461
$ git checkout pull/11461

Update a local copy of the PR:
$ git checkout pull/11461
$ git pull https://git.openjdk.org/jdk.git pull/11461/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11461

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11461.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2022-12-01T16:50:16Z

👋 Welcome back parttimenerd! 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 · 2022-12-01T16:53:40Z

@parttimenerd 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.

mlbridge · 2022-12-01T16:57:57Z

Webrevs

src/hotspot/cpu/aarch64/frame_aarch64.cpp

src/hotspot/cpu/x86/frame_x86.cpp

src/hotspot/cpu/aarch64/frame_aarch64.cpp

parttimenerd · 2022-12-02T07:09:02Z

I converted it back to a draft and look into next week with more vigor.

parttimenerd · 2022-12-06T12:52:54Z

My new version just uses SafeFetchN in the place where I consistently get segfaults in my fuzzer and it works.

parttimenerd · 2022-12-06T12:54:56Z

The issue might happen when ASGCT (and my ASGST for which I wrote a fuzzer) are called with modified sp and fp, which async-profiler might do.

parttimenerd · 2022-12-06T14:05:33Z

I added a few more safe fetches, safe_for_sender should now be guarded against any broken fp.

TheRealMDoerr

Quite a few SafeFetch checks! But it's only used for profiling purposes, so checking a bit more sounds ok to me. Let's hear what other people think.
Please update the description and the JBS issue according to your new version and describe how broken values may reach this function.

bridgekeeper · 2023-01-04T15:35:16Z

@parttimenerd This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

parttimenerd · 2023-01-04T15:38:47Z

This is ongoing work.

openjdk · 2023-02-17T19:17:22Z

@parttimenerd this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout parttimenerd_8297967
git fetch https://git.openjdk.org/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

bridgekeeper · 2023-04-15T02:02:22Z

@parttimenerd This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

TheRealMDoerr · 2023-04-24T14:00:08Z

I'm still trying to understand the underlying problem. I guess that FP points into a read protected (or uncommitted) part of the stack which isn't caught by the fp_safe checks for some reason. Using 3 safefetch checks is a bit much, because we shouldn't have gaps within a frame. On the other hand, checking exactly the 3 fields we need sounds like a good idea and your checks should be affordable from performance point of view. I'm ok with it, but I'd like to hear more opinions.

parttimenerd · 2023-04-24T14:04:21Z

The issue happens reproducibly while fuzzing profiling APIs. The change would remove a large class of errors that I found during testing. The issues can happen in theory too, due to e.g. a modified frame passed in by the profiler.

TheRealMDoerr · 2023-04-24T14:14:47Z

Can you dump the broken frame when one of your safefetch checks fail or create a hs_err file? That might shed more light into the problem.

parttimenerd · 2023-04-24T14:21:25Z

This happens when the frames frame and stack pointer are randomized (by adding a random number between 0 and 100000) during the fuzzing.

bridgekeeper · 2023-05-22T21:40:40Z

@parttimenerd This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

shipilev

Some drive-by comments here.

shipilev · 2023-06-02T16:27:49Z

src/hotspot/cpu/aarch64/frame_aarch64.cpp

+      if (SafeFetchN(this->fp() + return_addr_offset, 0) == 0 ||
+          SafeFetchN(this->fp() + interpreter_frame_sender_sp_offset, 0) == 0 ||
+          SafeFetchN(this->fp() + link_offset, 0) == 0


So these three are sender_sp, sender_unextended_sp and saved_fp a few lines below, right? So we can just SafeFetch-check those vars after we got them? This also highlights we want to check that those pointers are safe to fetch from.

I.e.:

// for interpreted frames, the value below is the sender "raw" sp, // which can be different from the sender unextended sp (the sp seen // by the sender) because of current frame local variables sender_sp = (intptr_t*) addr_at(sender_sp_offset); sender_unextended_sp = (intptr_t*) this->fp()[interpreter_frame_sender_sp_offset]; saved_fp = (intptr_t*) this->fp()[link_offset]; saved_fp = (intptr_t*) this->fp()[link_offset]; if ((SafeFetchN(sender_sp, 0) == 0) || (SafeFetchN(sender_unextended_sp) == 0) || (SafeFetchN(saved_fp, 0) == 0)) { return false; }

you're right, that looks much better

shipilev · 2023-06-02T16:29:02Z

src/hotspot/cpu/arm/frame_arm.cpp

@@ -210,8 +216,7 @@ bool frame::safe_for_sender(JavaThread *thread) {

  // Will the pc we fetch be non-zero (which we'll find at the oldest frame)

-  if ((address) this->fp()[return_addr_offset] == nullptr) return false;
-
+  if ((address) this->fp()[return_addr_offset] == NULL) return false;


Accidental revert: "nullptr" -> "NULL".

I started the PR before the whole move, thanks for noticing this :)

shipilev · 2023-06-02T16:29:19Z

src/hotspot/cpu/riscv/frame_riscv.cpp

+
+  if ((address) this->fp()[return_addr_offset] == NULL) return false;


Excess newline. Accidental revert: "nullptr" -> "NULL".

shipilev · 2023-06-02T16:29:24Z

src/hotspot/cpu/x86/frame_x86.cpp

@@ -256,7 +262,7 @@ bool frame::safe_for_sender(JavaThread *thread) {

  // Will the pc we fetch be non-zero (which we'll find at the oldest frame)

-  if ( (address) this->fp()[return_addr_offset] == nullptr) return false;
+  if ((address) this->fp()[return_addr_offset] == NULL) return false;


Accidental revert: "nullptr" -> "NULL".

parttimenerd · 2023-06-02T16:47:21Z

@shipilev do you think that bringing this PR is worthwhile? It only hardens the API when it is slightly misused (being passed in broken ucontexts).

shipilev · 2023-06-02T18:33:15Z

@shipilev do you think that bringing this PR is worthwhile? It only hardens the API when it is slightly misused (being passed in broken ucontexts).

In my mind, if this have a non-zero probability of happening during the normal non-fuzzing execution, then we should be on the safe side and check. A profiler that crashes the app is never a good thing. I got here through the bug report here -- so it might be a signal that problems like these happen IRL.

parttimenerd · 2023-06-03T08:43:04Z

This might possible. I'll work on it then next week.

bridgekeeper · 2023-07-01T12:25:13Z

@parttimenerd This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-07-29T16:37:02Z

@parttimenerd This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

openjdk bot added the rfr Pull request is ready for review label Dec 1, 2022

openjdk bot added the hotspot [email protected] label Dec 1, 2022

TheRealMDoerr reviewed Dec 1, 2022

View reviewed changes

src/hotspot/cpu/aarch64/frame_aarch64.cpp Outdated Show resolved Hide resolved

RealFYang reviewed Dec 2, 2022

View reviewed changes

src/hotspot/cpu/aarch64/frame_aarch64.cpp Outdated Show resolved Hide resolved

dholmes-ora suggested changes Dec 2, 2022

View reviewed changes

src/hotspot/cpu/aarch64/frame_aarch64.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/x86/frame_x86.cpp Outdated Show resolved Hide resolved

src/hotspot/cpu/aarch64/frame_aarch64.cpp Outdated Show resolved Hide resolved

parttimenerd marked this pull request as draft December 2, 2022 07:09

openjdk bot removed the rfr Pull request is ready for review label Dec 2, 2022

parttimenerd force-pushed the parttimenerd_8297967 branch from a890300 to f776255 Compare December 6, 2022 12:50

parttimenerd marked this pull request as ready for review December 6, 2022 12:51

openjdk bot added the rfr Pull request is ready for review label Dec 6, 2022

TheRealMDoerr reviewed Dec 6, 2022

View reviewed changes

parttimenerd marked this pull request as draft January 4, 2023 15:38

openjdk bot removed the rfr Pull request is ready for review label Jan 4, 2023

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 17, 2023

parttimenerd added 3 commits April 24, 2023 11:24

Make frame::safe_for_sender safer with SafeFetch

b4b8430

More SafeFetch

0241e46

Remove check for value that might be null

98afe54

parttimenerd force-pushed the parttimenerd_8297967 branch from 971553f to 98afe54 Compare April 24, 2023 09:24

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 24, 2023

Remove errorneously added check

b0c9088

parttimenerd requested review from dholmes-ora, TheRealMDoerr and RealFYang April 24, 2023 09:47

parttimenerd marked this pull request as ready for review April 24, 2023 09:48

openjdk bot added the rfr Pull request is ready for review label Apr 24, 2023

shipilev reviewed Jun 2, 2023

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jun 15, 2023

bridgekeeper bot closed this Jul 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297967: Make frame::safe_for_sender safer #11461

8297967: Make frame::safe_for_sender safer #11461

parttimenerd commented Dec 1, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

mlbridge bot commented Dec 1, 2022 •

edited

Loading

parttimenerd commented Dec 2, 2022

parttimenerd commented Dec 6, 2022

parttimenerd commented Dec 6, 2022

parttimenerd commented Dec 6, 2022

TheRealMDoerr left a comment

bridgekeeper bot commented Jan 4, 2023

parttimenerd commented Jan 4, 2023

openjdk bot commented Feb 17, 2023

bridgekeeper bot commented Apr 15, 2023

TheRealMDoerr commented Apr 24, 2023

parttimenerd commented Apr 24, 2023

TheRealMDoerr commented Apr 24, 2023

parttimenerd commented Apr 24, 2023

bridgekeeper bot commented May 22, 2023

shipilev left a comment

shipilev Jun 2, 2023 •

edited

Loading

parttimenerd Jun 2, 2023

shipilev Jun 2, 2023

parttimenerd Jun 2, 2023

shipilev Jun 2, 2023

shipilev Jun 2, 2023

parttimenerd commented Jun 2, 2023

shipilev commented Jun 2, 2023 •

edited

Loading

parttimenerd commented Jun 3, 2023

bridgekeeper bot commented Jul 1, 2023

bridgekeeper bot commented Jul 29, 2023


		if ((address) this->fp()[return_addr_offset] == NULL) return false;

8297967: Make frame::safe_for_sender safer #11461

8297967: Make frame::safe_for_sender safer #11461

Conversation

parttimenerd commented Dec 1, 2022 • edited by openjdk bot Loading

Progress

Integration blocker

Issue

Reviewing

Webrev

bridgekeeper bot commented Dec 1, 2022

openjdk bot commented Dec 1, 2022

mlbridge bot commented Dec 1, 2022 • edited Loading

Webrevs

parttimenerd commented Dec 2, 2022

parttimenerd commented Dec 6, 2022

parttimenerd commented Dec 6, 2022

parttimenerd commented Dec 6, 2022

TheRealMDoerr left a comment

Choose a reason for hiding this comment

bridgekeeper bot commented Jan 4, 2023

parttimenerd commented Jan 4, 2023

openjdk bot commented Feb 17, 2023

bridgekeeper bot commented Apr 15, 2023

TheRealMDoerr commented Apr 24, 2023

parttimenerd commented Apr 24, 2023

TheRealMDoerr commented Apr 24, 2023

parttimenerd commented Apr 24, 2023

bridgekeeper bot commented May 22, 2023

shipilev left a comment

Choose a reason for hiding this comment

shipilev Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

parttimenerd Jun 2, 2023

Choose a reason for hiding this comment

shipilev Jun 2, 2023

Choose a reason for hiding this comment

parttimenerd Jun 2, 2023

Choose a reason for hiding this comment

shipilev Jun 2, 2023

Choose a reason for hiding this comment

shipilev Jun 2, 2023

Choose a reason for hiding this comment

parttimenerd commented Jun 2, 2023

shipilev commented Jun 2, 2023 • edited Loading

parttimenerd commented Jun 3, 2023

bridgekeeper bot commented Jul 1, 2023

bridgekeeper bot commented Jul 29, 2023

parttimenerd commented Dec 1, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 1, 2022 •

edited

Loading

shipilev Jun 2, 2023 •

edited

Loading

shipilev commented Jun 2, 2023 •

edited

Loading