On Mon, 6 Jul 2026 04:31:05 GMT, David CARLIER <[email protected]> wrote:
>> Found while auditing the JVMTI local-variable accessors. >> >> `VM_BaseGetOrSetLocal` bounds-checks the requested slot before touching the >> frame's `StackValueCollection`. For a `long`/`double` the value spans two >> slots, so the check adds an `extra_slot` of 1: `_index + extra_slot >= >> method->max_locals()`. When an agent passes `slot == INT_MAX`, `_index + >> extra_slot` signed-overflows to `INT_MIN`, which is below `max_locals()`, so >> the guard is bypassed and we go on to index `locals->at(INT_MAX)` — out of >> bounds. That is an assertion failure in fastdebug and a SIGSEGV or silent >> corruption in product. >> >> Doing the arithmetic on the other side (`_index >= method->max_locals() - >> extra_slot`) avoids the overflow. The same check appears in both >> `check_slot_type_lvt` and `check_slot_type_no_lvt`, so both are fixed. >> >> Additional testing: >> - [x] Linux x86_64 server fastdebug, `serviceability/jvmti/GetLocalVariable` >> - [ ] Regular testing pipelines >> >> --------- >> - [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: > > 8387718: JVMTI GetLocal/SetLocal: slot bounds check overflows for > long/double slots > > VM_BaseGetOrSetLocal bounds-checks the requested slot before touching the > frame's StackValueCollection. For a long/double the value spans two slots, > so > the check adds an extra_slot of 1: _index + extra_slot >= > method->max_locals(). > When an agent passes slot == INT_MAX, _index + extra_slot signed-overflows > to > INT_MIN, which is below max_locals(), so the guard is bypassed and we go on > to > index locals->at(INT_MAX) -- out of bounds. That is an assertion failure in > fastdebug and a SIGSEGV or silent corruption in product. > > Doing the arithmetic on the other side (_index >= method->max_locals() - > extra_slot) avoids the overflow. The same check appears in both > check_slot_type_lvt and check_slot_type_no_lvt, so both are fixed. Thank you for taking care about this issue! I've posted some minor comments. Will make one more pass tomorrow. test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalSlotOverflow.java line 66: > 64: System.err.println("java.library.path: " + > System.getProperty("java.library.path")); > 65: throw ex; > 66: } The fragment at lines 60-66 with `System.loadLibrary(agentLib);` is not needed. The startup option `-agentlib:GetSetLocalSlotOverflow` initiates loading the native agent. We need to get rid of it in many tests around. test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalSlotOverflow.java line 73: > 71: > 72: // A non-native, non-static-receiver frame holding a few locals. The > agent > 73: // targets this frame (depth 1) with slot == INT_MAX. The actual local Nit: This comment is confusing. I'm not sure, I understand the intention. Should it say instead: "A java static frame holding a few locals." ? test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/GetSetLocalSlotOverflow.java line 77: > 75: // before any local is read. > 76: public static boolean runner() { > 77: long l = 0xCAFEBABEL; Nit: - The comment at line 74 should start from a capital letter. - Unneeded extra space after `long` at line 77. test/hotspot/jtreg/serviceability/jvmti/GetLocalVariable/libGetSetLocalSlotOverflow.cpp line 95: > 93: if (!caps.can_access_local_variables) { > 94: printf("Warning: Access to local variables is not implemented\n"); > 95: return JNI_ERR; Nit: I'd suggest to use the macro `LOG` instead of `printf` from the `jvmti_common.hpp`. It does the buffer flushing. You can find examples around this test folder. ------------- PR Review: https://git.openjdk.org/jdk/pull/31772#pullrequestreview-4662022696 PR Review Comment: https://git.openjdk.org/jdk/pull/31772#discussion_r3550896919 PR Review Comment: https://git.openjdk.org/jdk/pull/31772#discussion_r3550742817 PR Review Comment: https://git.openjdk.org/jdk/pull/31772#discussion_r3550754947 PR Review Comment: https://git.openjdk.org/jdk/pull/31772#discussion_r3550815397
