RFR: 8313796: AsyncGetCallTrace crash on unreadable interpreter method pointer [v2]

[Andrew Haley]

On Mon, 7 Aug 2023 21:03:28 GMT, Richard Startin <duke at openjdk.org> wrote:

>> src/hotspot/cpu/aarch64/frame_aarch64.cpp line 514:
>> 
>>> 512:     return false;
>>> 513:   }
>>> 514:   Method* m = *m_addr;
>> 
>> Suggestion:
>> 
>>   Method* m_addr = interpreter_frame_method_addr();
>>   if (m_addr == nullptr) {
>>     return false;
>>   }
>>   Method* m = SafeFetchN(m_addr, nullptr);
>>   if (m == nullptr) {
>>     return false;
>>   }
>> 
>> 
>> Reason: more robust against changes in memory map, faster.
>
> Thanks for raising this. It doesn't look like `SafeFetchN` is async signal safe because it calls into this:
> 
> 
> template <class T>
> ATTRIBUTE_NO_ASAN static bool _SafeFetchXX_internal(const T *adr, T* result) {
> 
>   T n = 0;
> 
>   // Set up a jump buffer. Anchor its pointer in TLS. Then read from the unsafe address.
>   // If that address was invalid, we fault, and in the signal handler we will jump back
>   // to the jump point.
>   sigjmp_buf jb;
>   if (sigsetjmp(jb, 1) != 0) {
>     // We faulted. Reset TLS slot, then return.
>     pthread_setspecific(g_jmpbuf_key, nullptr);
>     *result = 0;
>     return false;
>   }
> 
>   // Anchor jump buffer in TLS
>   pthread_setspecific(g_jmpbuf_key, &jb);
> 
>   // unsafe access
>   n = *adr;
> 
>   // Still here... All went well, adr was valid.
>   // Reset TLS slot, then return result.
>   pthread_setspecific(g_jmpbuf_key, nullptr);
>   *result = n;
> 
>   return true;
> 
> }
> 
> 
> This means we shouldn't be calling `os:: is_readable_pointer` either here, because it calls `SafeFetch32` which calls into the same function above. I had originally intended to simply perform a null check here (because that's the condition we've actually observed) and will push a change to revert to the null check.

> Thanks for raising this. It doesn't look like `SafeFetchN` is async signal safe because it calls into this:
> 
> ```c++
> template <class T>
> ATTRIBUTE_NO_ASAN static bool _SafeFetchXX_internal(const T *adr, T* result) {
> 
> ```
> 
> This means we shouldn't be calling `os:: is_readable_pointer` either here, because it calls `SafeFetch32` which calls into the same function above. I had originally intended to simply perform a null check here (because that's the condition we've actually observed) and will push a change to revert to the null check.

That's not how `SafeFetchN` is normally implemented. On Linux and BSD it's more like


    # Support for intptr_t SafeFetchN(intptr_t* address, intptr_t defaultval);
    #
    #  x1 : address
    #  x0 : defaultval
SafeFetchN_impl:
_SafeFetchN_fault:
    ldr      x0, [x0]
    ret
_SafeFetchN_continuation:
    mov      x0, x1
    ret


which should be fine.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15178#discussion_r1286391557