RFR: 8359222: [asan] jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest triggers stack-buffer-overflow error

[David Holmes]

On Wed, 6 Aug 2025 16:52:23 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review the following small change. On linux-aarch64, ASan reports an overflow error on a read that occurs when frames are copied from the stack to the heap—specifically during preemption on monitorenter and Object.wait when coming from compiled code. This read is benign and comes from reading either all (freeze fast path case) or part (freeze slow path case) of the `frame::metadata_words_at_bottom` stored in the callee frame (return pc+fp) for the top frame. In these preemption cases the callee frame is the frame of the VM native method being called (e.g. `Runtime1::monitorenter`). Now, the Arm 64-bit ABI doesn't specify a location where the frame record (returnpc+fp) has to be stored within a stack frame and GCC currently chooses to save it at the top of the frame (lowest address). So this causes ASan to treat the memory access in the callee as an overflow access to one of the locals stored in that frame. 
> 
> Accessing the callee to read `frame::metadata_words_at_bottom` is only necessary when the top frame is compiled. In that case, the callee is `Continuation.doYield` and those words contain the return pc and fp of the top frame we are freezing, where possibly fp might contain an oop. For the preemption cases mentioned above though, the return pc used for the top frame is always the one from the anchor, not the stack. Also, there is never a callee saved value in fp that needs to be preserved, the fp is always constructed again when thawing the frame. But because of sharing the code path with the compiled frame case we also read these words. I’ve added the full details on where these memory accesses occur in the freeze code, both slow and fast paths, in the bug comments.
> 
> The fix is to adjust these shared paths to avoid the read in the preemption case. I was able to reproduce the issue and have verified it is now fixed. I also ran the patch through mach5 tiers1-7.
> 
> Thanks,
> Patricio

Seems reasonable - though I dislike that we have to add ASAN and CPU specific code this way.

A couple of minor nits.

Thanks.

src/hotspot/cpu/aarch64/continuationFreezeThaw_aarch64.inline.hpp line 106:

> 104:     // For a compiled frame we need to re-read fp out of the frame because it may be an
> 105:     // oop and we might have had a safepoint in finalize_freeze, after constructing f.
> 106:     // For stub/native frames the value is not used while freezed, and will be constructed

Suggestion:

    // For stub/native frames the value is not used while frozen, and will be constructed

src/hotspot/cpu/x86/continuationFreezeThaw_x86.inline.hpp line 103:

> 101:     // For a compiled frame we need to re-read fp out of the frame because it may be an
> 102:     // oop and we might have had a safepoint in finalize_freeze, after constructing f.
> 103:     // For stub/native frames the value is not used while freezed, and will be constructed

Suggestion:

    // For stub/native frames the value is not used while frozen, and will be constructed

src/hotspot/share/runtime/continuationFreezeThaw.cpp line 796:

> 794:   // frame (lowest address). ASan treats this memory access in the callee as
> 795:   // an overflow access to one of the locals stored in that frame. For these
> 796:   // preemption case we don't need to read these words anyways so we avoid it.

Suggestion:

  // preemption cases we don't need to read these words anyways so we avoid it.

src/hotspot/share/runtime/continuationFreezeThaw.cpp line 797:

> 795:   // an overflow access to one of the locals stored in that frame. For these
> 796:   // preemption case we don't need to read these words anyways so we avoid it.
> 797:   adjust -= _preempt ? frame::metadata_words_at_bottom : 0;

Suggestion:

  if (_preempt) {
    adjust = 0;
  }

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26660#pullrequestreview-3099411836
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2261898883
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2261903871
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2261905677
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2261908614