On Tue, 15 Jul 2025 19:21:28 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... > > Volkan Yazici has updated the pull request incrementally with three > additional commits since the last revision: > > - Minimize the number of touched lines in `vmIntrinsics.hpp` > - Remove Markdown-styling in comments > - Improve wording of the `VerifyIntrinsicChecks` flag src/java.base/share/classes/java/lang/StringCoding.java line 36: > 34: import static java.util.Objects.requireNonNull; > 35: import static jdk.internal.util.Preconditions.AIOOBE_FORMATTER; > 36: import static jdk.internal.util.Preconditions.checkFromIndexSize; Avoid import static of methods, it makes the code harder to read and understand where the methods come from. Just import Preconditions. src/java.base/share/classes/java/lang/StringCoding.java line 162: > 160: if (c > '\u00FF') > 161: break; > 162: da[dp++] = (byte) c; Revert: Omit the space after (byte); there many more usages of "(byte)c" and this file does not need to drift from the prevailing style in StringUTF16.java AbstractStringBuilder, etc. Here and at line 195: src/java.base/share/classes/java/lang/StringCoding.java line 189: > 187: > 188: @IntrinsicCandidate > 189: private static int encodeAsciiArray0(char[] sa, int sp, byte[] da, > int dp, int len) { I rather prefer the readability of the src and dest parameters being on separate lines. (but not "{" by itself Suggestion: private static int encodeAsciiArray0(char[] sa, int sp, byte[] da, int dp, int len) { src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 43: > 41: import static java.util.Objects.requireNonNull; > 42: import static jdk.internal.util.Preconditions.AIOOBE_FORMATTER; > 43: import static jdk.internal.util.Preconditions.checkFromIndexSize; Avoid static imports of methods, it makes it harder to read and know where the methods are especially when crossing package boundaries. src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 177: > 175: if (c > '\u00FF') > 176: break; > 177: da[dp++] = (byte) c; Revert extra space. src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 201: > 199: int len = (dlen < slen) ? dlen : slen; > 200: try { > 201: int ret = len <= 0 ? 0 : encodeISOArray(sa, sp, da, dp, > len); Moving the length correction into `encodeISOArray` would help other callers too. (There's only 1 at the moment). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208470957 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208491363 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208494207 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208503099 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208504829 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2208510343
