RFR: 8297967: Make frame::safe_for_sender safer [v6]

[Aleksey Shipilev]

On Mon, 24 Apr 2023 09:52:05 GMT, Johannes Bechberger <jbechberger at openjdk.org> wrote:

>> 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.
>
> Johannes Bechberger has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Remove errorneously added check
>  - Remove check for value that might be null
>  - More SafeFetch
>  - Make frame::safe_for_sender safer with SafeFetch

Some drive-by comments here.

src/hotspot/cpu/aarch64/frame_aarch64.cpp line 139:

> 137:       if (SafeFetchN(this->fp() + return_addr_offset, 0) == 0 ||
> 138:           SafeFetchN(this->fp() + interpreter_frame_sender_sp_offset, 0) == 0 ||
> 139:           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;
      }

src/hotspot/cpu/arm/frame_arm.cpp line 219:

> 217:   // Will the pc we fetch be non-zero (which we'll find at the oldest frame)
> 218: 
> 219:   if ((address) this->fp()[return_addr_offset] == NULL) return false;

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

src/hotspot/cpu/riscv/frame_riscv.cpp line 260:

> 258:   // Will the pc we fetch be non-zero (which we'll find at the oldest frame)
> 259: 
> 260:   if ((address) this->fp()[return_addr_offset] == NULL) return false;

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

src/hotspot/cpu/x86/frame_x86.cpp line 265:

> 263:   // Will the pc we fetch be non-zero (which we'll find at the oldest frame)
> 264: 
> 265:   if ((address) this->fp()[return_addr_offset] == NULL) return false;

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

-------------

PR Review: https://git.openjdk.org/jdk/pull/11461#pullrequestreview-1457965340
PR Review Comment: https://git.openjdk.org/jdk/pull/11461#discussion_r1214583007
PR Review Comment: https://git.openjdk.org/jdk/pull/11461#discussion_r1214584013
PR Review Comment: https://git.openjdk.org/jdk/pull/11461#discussion_r1214584272
PR Review Comment: https://git.openjdk.org/jdk/pull/11461#discussion_r1214584344