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

[Johannes Bechberger]

On Fri, 2 Jun 2023 16:30:17 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

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

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

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

you're right, that looks much better

> 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".

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

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

PR Comment: https://git.openjdk.org/jdk/pull/11461#issuecomment-1574031359
PR Review Comment: https://git.openjdk.org/jdk/pull/11461#discussion_r1214598440
PR Review Comment: https://git.openjdk.org/jdk/pull/11461#discussion_r1214598756