On Sat, 3 Oct 2026 23:45:18 GMT, Justus Garbe <[email protected]> wrote:
> A rare bug with the jdk has recently been filed again: > https://github.com/adoptium/adoptium-support/issues/1538, which reports the > already seen behavior in JDK-8352077: `unhandled instruction`. The original > issue was closed with no reproduction. I have managed to write a reproduction > with the new data. > > It seems that it is caused by a missing null check in the > `BreakpointInfo::set` path, which calls into: > `incr_number_of_breakpoints` which calls `build_method_counters`. Which has > multiple ways to return `nullptr` if either the oom flag is already set or > the allocation fails, which is done via allocate_no_exception since it runs > on a VMThread, therefore it cant actually call a GC or raise the metadata > limit. Therefore incr_number_of_breakpoints, never increases the number > (since method counters is nullptr). Compilation is then paused, because the > method cannot increase it's invocation counter, then after a purge caused by > a gc pass, the mark is unset. After that it can correctly create it's > counter, but since it never captured the breakpoints, the breakpoint is 0. > > The method then can be picked up by a compiler thread since > `number_of_breakpoints` is now 0 and a counter is real it falsely passes > `Method::is_not_compilable`. Then it copies the bytecode containing the > `breakpoint` instruction, and doesn't invoke the clean function since the > breakpoint count reports '0'. This causes a crash in the `ciBytecodeStream` > since `ciBytecodeStream::next_wide_or_table` assumes the only special > bytecodes can be `wide` or a type of switch, and bytecode is a special length > instruction. > > This also explains why this happens so rarely. For this to happen the > following need to happen in succession: > - The debugger must try to set breakpoints > - The method must never have been run > - Metadata space needs to be at the limit and not resized when its setting > the breakpoints and set the oom flag > - Class unloading must purge the flag afterwards > - The method must get compiled hot while the breakpoint is still installed > > 3 of the 5 conditions already happen when debugging with jdwp normally, from > platforms like intellij, the others are race / resource conditions that seem > happen very rarely. > > For the fix i propose to check the method counter allocation before actually > requesting a breakpoint set. Here done by checking if method counters are not > null before sending the vm request. I also had to expand the user facing > method to make sure the OOM error code is propagated properly (JVMTI_... Thank you for the catch and contribution. The fix is good in general. I've posted a couple of suggestions. I need more time to complete review of new test. src/hotspot/share/prims/jvmtiEnv.cpp line 2525: > 2523: if (result == JVMTI_ERROR_OUT_OF_MEMORY) > 2524: return JVMTI_ERROR_OUT_OF_MEMORY; > 2525: I have several suggestions: - the name `result` is inconsistent with other places in this file, need to rename it to `err`. - the type has to be `jvmtiError` - we normally use curly brackets for conditional blocks - I'd refactor this a little bit as below: jvmtiError err = jvmti_breakpoints.set(bp); if (err != JVMTI_ERROR_NONE) { return err; } test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/libBreakpointMetaspaceOOM.cpp line 29: > 27: #include "jvmti.h" > 28: > 29: #define LOG(...) do { printf(__VA_ARGS__); printf("\n"); fflush(stdout); > } while (0) You need to add this line: #include "jvmti_common.hpp" The macro `LOG` and some convenient functions like `check_jvmti_status()` or `check_jvmti_error()` can be used. test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/libBreakpointMetaspaceOOM.cpp line 40: > 38: } > 39: > 40: JNIEXPORT jint JNICALL Agent_OnLoad(JavaVM* vm, char* options, void* > reserved) { Nit: Place the `JNIEXPORT jint JNICALL` from line 40 at a separate line for consistency with other cases. test/hotspot/jtreg/serviceability/jvmti/SetBreakpoint/libBreakpointMetaspaceOOM.cpp line 73: > 71: } > 72: > 73: extern "C" { Nits: - Place the line 73 right after includes - Use `check_jvmti_status()` or `check_jvmti_error()` from the `jvmti_common.hpp` ------------- PR Review: https://git.openjdk.org/jdk/pull/33191#pullrequestreview-5439793369 PR Review Comment: https://git.openjdk.org/jdk/pull/33191#discussion_r4204841596 PR Review Comment: https://git.openjdk.org/jdk/pull/33191#discussion_r4204905729 PR Review Comment: https://git.openjdk.org/jdk/pull/33191#discussion_r4204962163 PR Review Comment: https://git.openjdk.org/jdk/pull/33191#discussion_r4204947025
