On Tue, 1 Jul 2025 03:17:41 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
>> The segv/eav happens in the case if JvmtiBreakpoint::_method's class
>> redefined old between getting the Method* from jmethodid in the
>> JvmtiEnv::SetBreakpoint(Method* method, jlocation location) {..} and
>> and actual setting breakpoint in the VM operation VM_ChangeBreakpoints.
>>
>> Here are details:
>> The breakpoint is set in 2 steps.
>> 1) method jvmti_SetBreakpoint(jvmtiEnv* env, jmethodID method, jlocation
>> location) convert jmethodID to Method* and call
>> JvmtiEnv::SetBreakpoint(Method* method, jlocation location)
>> where
>> JvmtiBreakpoint bp(method, location);
>> is created with this Method*
>> Note: it is done while thread is in VM state, so Method can't become is_old
>> while this is done.
>>
>> 2) The VMOp is used to add breakpoint into the list
>> VM_ChangeBreakpoints set_breakpoint(VM_ChangeBreakpoints::SET_BREAKPOINT,
>> &bp);
>> VMThread::execute(&set_breakpoint);
>> to call JvmtiBreakpoints::set_at_safepoint()
>> that can modify JvmtiBreakpoints list and set breakpoint in safepoint
>> without synchronization.
>>
>> So it might be possible that class redefinition VM_RedefineClasses
>> operation that redefine the class with this breakpoint happens between steps
>> 1) and 2)
>> VM_RedefineClasses::redefine_single_class()
>> clear all class-related breakpoints in the JvmtiBreakpoints, however the
>> "problematic" breakpoint is in VMThread queue and thus we are still
>> continue to do this operation.
>> So in the step 2) the the JvmtiBreakpoint with 'is_old' method is added to
>> the JvmtiBreakpoints and breakpoint is set.
>>
>> Then old method mights be purged any time once they are not on the stack
>> and any access to this breakpoint could lead to usage of Metthod* _method
>> pointing to deallocated metaspace.
>>
>> The VM_RedefineClasses clear all breakpoints so it is correct just to don't
>> proceed with current breakpoint also.
>>
>> Looks, like very unlikely but reproducing with stress test after some time.
>> Verified that the crash is not reproduced anymore with corresponding test
>> after the fix.
>>
>> Many thanks to Coleen for detailed explanation of class redefinition.
>
> Leonid Mesnik has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Update src/hotspot/share/prims/jvmtiImpl.cpp
>
> Co-authored-by: David Holmes
> <62092539+dholmes-...@users.noreply.github.com>
> - Update src/hotspot/share/prims/jvmtiImpl.cpp
>
> Co-authored-by: David Holmes
> <62092539+dholmes-...@users.noreply.github.com>
This looks great. Thank you for sorting through all this code and my
speculation about the problem to find the real problem. For the record, I
don't think my change to remove is_running_emcp() caused this or any bug. I
wrote a test case for it yesterday and that code seems fine. Nice work finding
the real problem here and the straightforward solution.
There are several places in the JVM where we have to check for is_old() methods
to exclude them for various things. is_old() methods leaking into places is a
common bug pattern. This change is consistent with this approach of fixing this.
src/hotspot/share/prims/jvmtiImpl.cpp line 262:
> 260: }
> 261:
> 262: // ensure that bp._method is not deallocated before
> VM_ChangeBreakpoints::doit()
Suggestion:
// Ensure that bp._method is not deallocated before
VM_ChangeBreakpoints::doit().
src/hotspot/share/prims/jvmtiImpl.cpp line 274:
> 272: }
> 273:
> 274: // ensure that bp._method is not deallocated before
> VM_ChangeBreakpoints::doit()
Suggestion:
// Ensure that bp._method is not deallocated before
VM_ChangeBreakpoints::doit().
-------------
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/26031#pullrequestreview-2975077594
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2177385911
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2177386666
