RFR: 8282306: os::is_first_C_frame(frame*) crashes on invalid link access [v2]

[Thomas Stuefe]

On Wed, 23 Feb 2022 16:10:25 GMT, Johannes Bechberger <duke at openjdk.java.net> wrote:

>> This PR introduces a new method `can_access_link` into the frame class to check the accessibility of the link information. It furthermore adds a new `os::is_first_C_frame(frame*, Thread*)` that uses the `can_access_link` method
>> and the passed thread object to check the validity of frame pointer, stack pointer, sender frame pointer and sender stack pointer. This should reduce the possibilities for crashes.
>
> Johannes Bechberger has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Improve use of C macros

Hi Johannes,

Thanks for doing this, solving this makes sense.

But I'm not sure yours is the right approach. I think it would better to use SafeFetch to check the addresses in the relevant registers. 

Using Safefetch would mean that we don't depend on the existence of Thread (which may be NULL, especially in signal contexts). It would work if the registers erroneously point into unmapped or guarded portions of the stack, or if Thread is corrupted or outdated. And it would be way simpler, since it would not require a new version of is_first_C_frame.

I also find the interface - passing Thread* to the function just for it to then do error checking - slightly off. Without any comment on the prototype explaining what this argument is for, this causes head scratching. And semantically, there is only one instance of Thread this can ever be called for.

A function like this:

// check if frame is valid within the Thread's stack
bool Thread::is_valid_frame(const frame*)

would actually be clearer.

And if this error check is necessary, why do we then need two variants of is_first_c_frame? Should the error check not always happen?

But bottom line, I think safefetch would be a simpler and more robust approach.

Cheers, Thomas

src/hotspot/cpu/aarch64/frame_aarch64.inline.hpp line 155:

> 153: inline intptr_t* frame::link() const              { return (intptr_t*) *(intptr_t **)addr_at(link_offset); }
> 154: 
> 155: inline bool frame::can_access_link(Thread *thread) const { return thread->is_in_full_stack((address)addr_at(link_offset)); }

is there a reason Thread* is non-const in all your variants of can_access_link and is_first_c_frame?

src/hotspot/cpu/ppc/frame_ppc.inline.hpp line 120:

> 118: }
> 119: 
> 120: inline bool frame::can_access_link(Thread *thread) const { return true; }

Why are ppc and s390 different from other platforms? If there is a valid reason, could you please add a short comment?

src/hotspot/cpu/zero/frame_zero.inline.hpp line 85:

> 83: }
> 84: 
> 85: inline bool frame::can_access_link(Thread *t) const {

Did you test zero? Would this not just crash it?

src/hotspot/share/runtime/os.cpp line 1223:

> 1221:   return true; // native stack isn't walkable on windows this way.
> 1222: #else
> 1223:   return !fr->can_access_link(t) || os::is_first_C_frame(fr) ||

Check t for NULL.

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

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/7591