On Thu, 26 Jun 2025 10:48:37 GMT, Volkan Yazici <vyaz...@openjdk.org> wrote:
> Validate input in `java.lang.StringCoding` intrinsic Java wrappers, improve > their documentation, enhance the checks in the associated IR or assembly > code, and adapt them to cause VM crash on invalid input. > > ## Implementation notes > > The goal of the associated umbrella issue > [JDK-8156534](https://bugs.openjdk.org/browse/JDK-8156534) is to, for > `java.lang.String*` classes, > > 1. Move `@IntrinsicCandidate`-annotated `public` methods<sup>1</sup> (in Java > code) to `private` ones, and wrap them with a `public` ["front door" > method](https://github.com/openjdk/jdk/pull/24982#discussion_r2087493446) > 2. Since we moved the `@IntrinsicCandidate` annotation to a new method, > intrinsic mappings – i.e., associated `do_intrinsic()` calls in > `vmIntrinsics.hpp` – need to be updated too > 3. Add necessary input validation (range, null, etc.) checks to the newly > created public front door method > 4. Place all input validation checks in the intrinsic code (add if missing!) > behind a `VerifyIntrinsicChecks` VM flag > > Following preliminary work needs to be carried out as well: > > 1. Add a new `VerifyIntrinsicChecks` VM flag > 2. Update `generate_string_range_check` to produce a `HaltNode`. That is, > crash the VM if `VerifyIntrinsicChecks` is set and a Java wrapper fails to > spot an invalid input. > > <sup>1</sup> `@IntrinsicCandidate`-annotated constructors are not subject to > this change, since they are a special case. > > ## Functional and performance tests > > - `tier1` (which includes `test/hotspot/jtreg/compiler/intrinsics/string`) > passes on several platforms. Further tiers will be executed after integrating > reviewer feedback. > > - Performance impact is still actively monitored using > `test/micro/org/openjdk/bench/java/lang/String{En,De}code.java`, among other > tests. If you have suggestions on benchmarks, please share in the comments. > > ## Verification of the VM crash > > I've tested the VM crash scenario as follows: > > 1. Created the following test program: > > public class StrIntri { > public static void main(String[] args) { > Exception lastException = null; > for (int i = 0; i < 1_000_000; i++) { > try { > > jdk.internal.access.SharedSecrets.getJavaLangAccess().countPositives(new > byte[]{1,2,3}, 2, 5); > } catch (Exception exception) { > lastException = exception; > } > } > if (lastException != null) { > lastException.printStackTrace(); > } else { > System.out.println("completed"); > } > } > } > ... Thanks a lot for looking into this Volkan! I left a couple of minor comments. I also noticed that you haven't yet added the benchmark results to the description: do you want to run them again after the reviews? src/hotspot/cpu/x86/macroAssembler_x86.cpp line 6014: > 6012: } > 6013: > 6014: // Encode given `char[]`/`byte[]` to `byte[]` in ISO_8859_1 or ASCII Thanks for updating the comments! Do we need the markdown back quotes here (and in other `macroAssembler-*` file comments)? src/hotspot/share/classfile/vmIntrinsics.hpp line 417: > 415: > \ > 416: do_class(java_lang_StringCoding, "java/lang/StringCoding") > \ > 417: do_intrinsic(_countPositives, java_lang_StringCoding, > countPositives_name, countPositives_signature, F_S) \ It is a matter of taste but it might be better not to change the whitespaces (it might make searching for changes (and possibly backports) harder. The rest of the file is not too consistent anyway). src/hotspot/share/opto/c2_globals.hpp line 666: > 664: > \ > 665: develop(bool, VerifyIntrinsicChecks, false, > \ > 666: "Verify that Java level checks in intrinsics work as > expected") \ To make it clearer I think we might want to move "in intrinsic" after "Verify" or at the end. ------------- PR Review: https://git.openjdk.org/jdk/pull/25998#pullrequestreview-3015376813 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2204312397 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2204505403 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2204256430
