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

[Patricio Chilano Mateo]

On Fri, 8 Aug 2025 04:36:42 GMT, David Holmes <dholmes at openjdk.org> wrote:

> Seems reasonable - though I dislike that we have to add ASAN and CPU specific code this way.
> 
Initially I added it to avoid the extra branch in this fast path except when using ASan builds. I checked and the compiler is smart enough to avoid that and just generates an extra load of `_preempt` and arithmetic instructions that use a register instead of a constant. So maybe we could make it unconditional. But I thought the macros also make it clear this is only needed for this type of build.

> 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

Fixed.

> 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

Fixed.

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

Fixed.

> 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;
>   }

Done.

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

PR Comment: https://git.openjdk.org/jdk/pull/26660#issuecomment-3169053521
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2263820051
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2263818906
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2263819043
PR Review Comment: https://git.openjdk.org/jdk/pull/26660#discussion_r2263819286