On Tue, 4 Aug 2026 04:33:28 GMT, David CARLIER <[email protected]> wrote:
>> In `vframeStreamCommon::skip_prefixed_method_and_wrappers()`, `prefix_len` >> is a `size_t` computed as `prefixed_name_len - name_len`, so the `prefix_len >> <= 0` guard only ever catches `prefix_len == 0`. When the next method on the >> stack has a longer name than the prefixed native method, the subtraction >> underflows, the guard passes, and `strcmp` reads at `prefixed_name + >> prefix_len`, which has wrapped to a pointer before the string. The walk is >> only reached when a JVMTI agent has registered a native method prefix with >> `SetNativeMethodPrefix`, which several profilers do. >> >> The fix checks the lengths before subtracting. `name_len >= >> prefixed_name_len` covers the `prefix_len == 0` case the old guard was meant >> to catch, so nothing changes for inputs that were already handled correctly, >> and `prefix_len` is now in `[1, prefixed_name_len)` for the comparisons that >> follow. >> >> The test registers `wrapped_` as a prefix and declares a native >> `wrapped_go()` next to a non-native `go()` with the same signature, so >> resolving `wrapped_go()` strips the prefix and binds it to `go()`'s entry >> point as a prefixed native. The native code then calls `RegisterNatives` >> with no methods on a boot loader class, since that asks the VM for the >> calling class at depth 1 and walks over the native frame, its wrapper and >> the longer-named caller below. >> >> Note that the test does not fail on an unfixed VM: `strcmp` stops at the >> first mismatching byte, so the over-read is a single byte inside the >> resource-area chunk the string came from, which no sanitizer will flag. It >> covers a path nothing else covers and locks in the guard. I confirmed the >> underflow is reached by temporarily printing the lengths from the VM. >> >> Testing: >> >> - [x] `test/hotspot/jtreg:tier1`, linux-x86_64 fastdebug: 3583 passed, 7 >> failed. The failures are `gc/TestTransparentHugePagesHeap.java` and >> `runtime/os/TestTracePageSizes.java`, which fail on this machine because of >> its transparent huge page settings and are unrelated to the change. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > David CARLIER has updated the pull request incrementally with one additional > commit since the last revision: > > Apply feedback: use NATIVE_METHOD_PREFIX naming for the test constant Changes requested by sspitsyn (Reviewer). test/hotspot/jtreg/serviceability/jvmti/SetNativeMethodPrefix/PrefixedNativeStackWalk.java line 39: > 37: * The agent registers "wrapped_" as a native method prefix, so resolving > the > 38: * native wrapped_walk() strips the prefix, finds the non-native walk() > and > 39: * binds wrapped_walk() to walk()'s native entry point. That marks > wrapped_walk() Nit: It seems that references to `wrapped_walk()` and `walk()` have to be replaced with `wrapped_go()` and `go()`. test/hotspot/jtreg/serviceability/jvmti/SetNativeMethodPrefix/libPrefixedNativeStackWalk.cpp line 49: > 47: memset(methods, 0, sizeof(methods)); > 48: > 49: jint res = jni->RegisterNatives(boot_cls, methods, 0); Nit: Could you add a comment why is the definition at line 46 needed? Can we just replace the `methods` with `nullptr` at line 49? ------------- PR Review: https://git.openjdk.org/jdk/pull/32180#pullrequestreview-4851863959 PR Review Comment: https://git.openjdk.org/jdk/pull/32180#discussion_r3710440804 PR Review Comment: https://git.openjdk.org/jdk/pull/32180#discussion_r3710502245
