On Fri, 26 Jun 2026 13:33:37 GMT, Frederic Parain <[email protected]> wrote:
> This patch adds missing argument validation to copyOfSpecialArray() and > copyOfRangeSpecialArray(). > The implementation in JVM_CopyOfSpecialArray is also updated to address a > number of issues (inconsistencies, code duplication, buggy index > computation). Those changes should solve JDK-8387251. > > Tested with Mach5, tier1-4. > > --------- > - [X] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). This looks good. I have a couple of questions and would like short comments on some of the parts. src/hotspot/share/prims/jvm.cpp line 437: > 435: arrayHandle oh(THREAD, org); > 436: ObjArrayKlass* ak = ObjArrayKlass::cast(org->klass()); > 437: InlineKlass* vk = InlineKlass::cast(ak->element_klass()); Can the declaration of 'vk' be put down in the else block where it's used? I see the caller already checks this also. src/hotspot/share/prims/jvm.cpp line 438: > 436: ObjArrayKlass* ak = ObjArrayKlass::cast(org->klass()); > 437: InlineKlass* vk = InlineKlass::cast(ak->element_klass()); > 438: int len = to - from; // length of the new array Is there a check somewhere that from < to, so len is not negative? I see caller checks. Can you add a comment like // Caller checks from < to? src/hotspot/share/prims/jvm.cpp line 462: > 460: ak->copy_array(oh(), from, dh(), 0, copy_len, CHECK_NULL); > 461: for (int i = copy_len; i < len; i++) { > 462: ((objArrayOop)dh())->obj_at_put(i, nullptr); Do you need the cast? Can you not do dh->obj_at_put() ? I think the handle will do this for you. Can you add comment like // Null out values past the length of the copy or something like that. src/hotspot/share/prims/jvm.cpp line 465: > 463: } > 464: } else { > 465: for (int i = 0; i < len; i++) { This is for the case where copy_len = 0? Do we want to assert also here that the new array shouldn't be null free array? test/hotspot/jtreg/runtime/valhalla/inlinetypes/TestArrayFactories.java line 412: > 410: throw new RuntimeException("Missing > IllegalArgumentException"); > 411: } catch (IllegalArgumentException e) {} > 412: } Is there a positive test where a non null restricted array gets nulls for indexes outside the range of from and to? ie, the 'else' branch line 465? ------------- PR Review: https://git.openjdk.org/valhalla/pull/2586#pullrequestreview-4593994109 PR Review Comment: https://git.openjdk.org/valhalla/pull/2586#discussion_r3493571252 PR Review Comment: https://git.openjdk.org/valhalla/pull/2586#discussion_r3493552535 PR Review Comment: https://git.openjdk.org/valhalla/pull/2586#discussion_r3493601215 PR Review Comment: https://git.openjdk.org/valhalla/pull/2586#discussion_r3493646397 PR Review Comment: https://git.openjdk.org/valhalla/pull/2586#discussion_r3493678092
