RFR: 8313796: AsyncGetCallTrace crash on unreadable interpreter method pointer

[Richard Startin]

On Mon, 7 Aug 2023 20:42:48 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> We have observed invalid pointers to the interpreted method at Datadog. The fix is based on a discussion with and a code snippet from @parttimenerd.
>
> 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.

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

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