Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8297967: Make frame::safe_for_sender safer #11461

Closed
wants to merge 4 commits into from
Closed

8297967: Make frame::safe_for_sender safer #11461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b4b8430
Make frame::safe_for_sender safer with SafeFetch
parttimenerd Dec 6, 2022
0241e46
More SafeFetch
parttimenerd Dec 6, 2022
98afe54
Remove check for value that might be null
parttimenerd Dec 7, 2022
b0c9088
Remove errorneously added check
parttimenerd Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/hotspot/cpu/aarch64/frame_aarch64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
#include "runtime/javaCalls.hpp"
#include "runtime/monitorChunk.hpp"
#include "runtime/os.inline.hpp"
#include "runtime/safefetch.hpp"
#include "runtime/signature.hpp"
#include "runtime/stackWatermarkSet.hpp"
#include "runtime/stubCodeGenerator.hpp"
Expand Down Expand Up @@ -132,6 +133,11 @@ bool frame::safe_for_sender(JavaThread *thread) {
if (!fp_safe) {
return false;
}
// check that the accessed stack slots are safe too
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
Comment on lines +137 to +139
Copy link
Member

@shipilev shipilev Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, that looks much better

) return false;

// for interpreted frames, the value below is the sender "raw" sp,
// which can be different from the sender unextended sp (the sp seen
Expand Down Expand Up @@ -264,8 +270,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] == NULL) return false;

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

// could try and do some more potential verification of native frame if we could think of some...

Expand Down
9 changes: 7 additions & 2 deletions src/hotspot/cpu/arm/frame_arm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "runtime/javaCalls.hpp"
#include "runtime/monitorChunk.hpp"
#include "runtime/os.inline.hpp"
#include "runtime/safefetch.hpp"
#include "runtime/signature.hpp"
#include "runtime/stubCodeGenerator.hpp"
#include "runtime/stubRoutines.hpp"
Expand Down Expand Up @@ -104,6 +105,11 @@ bool frame::safe_for_sender(JavaThread *thread) {
if (!fp_safe) {
return false;
}
// check that the accessed stack slots are safe too
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
) return false;

sender_pc = (address) this->fp()[return_addr_offset];
sender_sp = (intptr_t*) addr_at(sender_sp_offset);
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// could try and do some more potential verification of native frame if we could think of some...

Expand Down
9 changes: 8 additions & 1 deletion src/hotspot/cpu/riscv/frame_riscv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "runtime/javaCalls.hpp"
#include "runtime/monitorChunk.hpp"
#include "runtime/os.inline.hpp"
#include "runtime/safefetch.hpp"
#include "runtime/signature.hpp"
#include "runtime/stackWatermarkSet.hpp"
#include "runtime/stubCodeGenerator.hpp"
Expand Down Expand Up @@ -129,6 +130,11 @@ bool frame::safe_for_sender(JavaThread *thread) {
if (!fp_safe) {
return false;
}
// check that the accessed stack slots are safe too
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
) return false;

sender_pc = (address)this->fp()[return_addr_offset];
// for interpreted frames, the value below is the sender "raw" sp,
Expand Down Expand Up @@ -250,7 +256,8 @@ 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;
Comment on lines +259 to +260
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return true;
}
Expand Down
8 changes: 7 additions & 1 deletion src/hotspot/cpu/x86/frame_x86.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "runtime/handles.inline.hpp"
#include "runtime/javaCalls.hpp"
#include "runtime/monitorChunk.hpp"
#include "runtime/safefetch.hpp"
#include "runtime/signature.hpp"
#include "runtime/stackWatermarkSet.hpp"
#include "runtime/stubCodeGenerator.hpp"
Expand Down Expand Up @@ -123,6 +124,11 @@ bool frame::safe_for_sender(JavaThread *thread) {
if (!fp_safe) {
return false;
}
// check that the accessed stack slots are safe too
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
) return false;

sender_pc = (address) this->fp()[return_addr_offset];
// for interpreted frames, the value below is the sender "raw" sp,
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



// could try and do some more potential verification of native frame if we could think of some...
Expand Down