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

[Johannes Bechberger]

On Mon, 7 Aug 2023 21:17:04 GMT, Andrew Haley <aph at openjdk.org> wrote:

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

@tstuefe implemented explicitly for being signal safe.

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

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