RFR: 8361842: Move input validation checks to Java for String-related intrinsics [v2]

[Roger Riggs]

On Tue, 15 Jul 2025 19:21:28 GMT, Volkan Yazici <vyazici at 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