RFR: 8359366: RunThese30M.java EXCEPTION_ACCESS_VIOLATION in JvmtiBreakpoints::clearall_in_class_at_safepoint

[David Holmes]

On Sat, 28 Jun 2025 05:02:56 GMT, Leonid Mesnik <lmesnik at 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.

Approach seems reasonable but it is worrisome that we still have these kinds of issues with class redefinition!  And why has this suddenly appeared? Did a recent code change introduce this bug?

There are a few typos in the change.

src/hotspot/share/prims/jvmtiImpl.cpp line 188:

> 186: 
> 187: void VM_ChangeBreakpoints::doit() {
> 188:  if (_bp->method() != Method::resolve_jmethod_id(_preservred_method)) {

Suggestion:

 if (_bp->method() != Method::resolve_jmethod_id(_preserved_method)) {

src/hotspot/share/prims/jvmtiImpl.cpp line 189:

> 187: void VM_ChangeBreakpoints::doit() {
> 188:  if (_bp->method() != Method::resolve_jmethod_id(_preservred_method)) {
> 189:    // the jmethod_id's method was updated if class redefintion happened for this class

Suggestion:

   // the jmethod_id's method was updated if class redefinition happened for this class

src/hotspot/share/prims/jvmtiImpl.cpp line 191:

> 189:    // the jmethod_id's method was updated if class redefintion happened for this class
> 190:    // after JvmtBreakpoint was created but before JVM_ChangeBreakpoints started
> 191:    // all class breakpoints are cleared during redefinition so don't set/clear this breakpoint

So basically one thread is trying to change this particular BP and it races with another thread that performs redefinition. If the redefinition thread wins then we are turning this current change into a no-op on the basis that the redefinition cleared all BPs anyway so we should not now set this one (if that was requested).

It is very unclear to me how the thread that requested the current change might respond to that request being ignored.

Please add some punctuation to the comment block as it is very hard to read at present. Thanks

src/hotspot/share/prims/jvmtiImpl.hpp line 144:

> 142:   int               _operation;
> 143:   JvmtiBreakpoint*  _bp;
> 144:   jmethodID         _preservred_method; //needed to track class redefintion

Suggestion:

  jmethodID         _preserved_method; //needed to track class redefinition

src/hotspot/share/prims/jvmtiImpl.hpp line 154:

> 152:     _operation = operation;
> 153:     assert(bp != nullptr, "bp != null");
> 154:     _preservred_method = bp->method()->jmethod_id();

Suggestion:

    _preserved_method = bp->method()->jmethod_id();

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/26031#pullrequestreview-2969873307
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174205095
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174206113
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174209649
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174201993
PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174202853