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

[Thomas Stuefe]

On Mon, 14 Mar 2022 11:28:33 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:
> 
>   Add check for Thread to CanUseSafefetch

Looks good to me. If you add the comment about JDK-8282475 (see inline remark) this is fine.

Cheers, Thomas

src/hotspot/share/runtime/safefetch.inline.hpp line 57:

> 55: // returns true if SafeFetch32 and SafeFetchN can be used safely (stubroutines are already generated)
> 56: inline bool CanUseSafeFetch32() {
> 57: #if defined (__APPLE__) && defined(AARCH64)

Pls in front of this __APPLE__ section and the one below add a comment like `// workaround for JDK-8282475`. That will remind us to remove this again once that bug is fixed.

src/hotspot/share/runtime/safefetch.inline.hpp line 61:

> 59:     return false;
> 60:   }
> 61: #endif // __APPLE__ && AARCH64

Totally valid way to work around JDK-8282475, but note that `os::is_readable_pointer()` defaults to true for `CanSafeFetch=false` (see https://github.com/openjdk/jdk/blob/6013d09e82693a1c07cf0bf6daffd95114b3cbfa/src/hotspot/share/runtime/os.cpp#L1042-L1046)

The story behind that is that there are more use cases of `is_readable_memory()` where - if one cannot use SafeFetch - it makes sense to optimistically assume the location is readable. E.g. in error handling, where secondary crashes are annoying but not deadly. The contract is that if you really care about this, use CanUseSafeFetch beforehand (usually it was fine to ignore it since CanUseSafeFetch only affected a small time window during VM initialization).

With your patch, it means that now os::is_readable_memory always returns true if Thread::current is NULL. So in that case, we bypass your protection. I think that is acceptable for now. Once JDK-8282475 is solved we can remove this again.

I wondered whether this affects your gtest test case, but in that test case Thread::current is not NULL because its a TEST_VM case, so we are in the VM.

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

Marked as reviewed by stuefe (Reviewer).

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