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

[Coleen Phillimore]

On Mon, 30 Jun 2025 04:38:41 GMT, David Holmes <dholmes 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.
>> 
>> Many thanks to Coleen for detailed explanation of class redefinition.
>
> 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)) {

I see what you're doing.  You're checking if the methodID is changed by redefinition. Can you just check if the method->is_old() and skip the breakpoint then?  Although the callers might want a breakpoint at the new method if it's emcp.  Maybe it could call Method::get_new_method() if is_old and set the breakpoint on the new method?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26031#discussion_r2174895443