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

[Thomas Stuefe]

On Fri, 25 Feb 2022 13:02:37 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:
> 
>   Fix tests

Hi Johannes,

Getting closer. More remarks inline.

Cheers, Thomas

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

> 152: 
> 153: inline intptr_t* frame::link_or_null() const {
> 154:   auto ptr = (intptr_t **)addr_at(link_offset);

Please don't use auto. In general, use features and style that is adapted around you, and beyond that pls refer to the C++ style guide. When in Rome...

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

> 153: inline intptr_t* frame::link_or_null() const {
> 154:   auto ptr = (intptr_t **)addr_at(link_offset);
> 155:   if (os::is_readable_pointer((const void*)ptr)) {

You don't need this cast

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

> 157:   }
> 158:   return NULL;
> 159: }

You could shorten these four lines to a single one using `?`, especially since this code is duplicated across platforms.

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

> 1177: // Looks like all platforms can use the same function to check if C
> 1178: // stack is walkable beyond current frame.
> 1179: // Returns false if this is the cas

typo

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

> 1182: #ifdef _WINDOWS
> 1183:   return true; // native stack isn't walkable on windows this way.
> 1184: #else

This change has nothing to do with the bug.

I would leave this as it is and let the code below at least compile on windows. Then we know it does not bitrot there. I am also not clear why this would not work on windows, since we could optionally build with framepointers enabled, right? And don't we have frame pointers on 32-bit windows always? I may remember this wrong.

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

> 1191: 
> 1192:   uintptr_t usp    = (uintptr_t)fr->sp();
> 1193:   if ((usp & sp_align_mask) != 0 || !os::is_readable_pointer((const void*)usp)) return true;

remove cast

test/hotspot/gtest/runtime/test_os.cpp line 874:

> 872:   frame invalid_frame;
> 873:   EXPECT_TRUE(os::is_first_C_frame(&invalid_frame)); // the frame has zeroes for all values
> 874: 

Please add a test with valid looking but garbage pointers, to test that your safefetch really works. We usually do this by reserving + protecting a stripe of memory and using that one as guaranteed faulting pointer.

test/hotspot/gtest/runtime/test_os.cpp line 875:

> 873:   EXPECT_TRUE(os::is_first_C_frame(&invalid_frame)); // the frame has zeroes for all values
> 874: 
> 875:   auto cur_frame = os::current_frame(); // this frame has to have a sender

please use a type here, not auto

test/hotspot/gtest/runtime/test_os.cpp line 878:

> 876:   EXPECT_FALSE(os::is_first_C_frame(&cur_frame));
> 877:   #endif // _WIN32
> 878: }

missing newline

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

Changes requested by stuefe (Reviewer).

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