RFR: 8361842: Move input validation checks to Java for java.lang.StringCoding intrinsics [v9]
Volkan Yazici
vyazici at openjdk.org
Mon Jul 21 12:52:43 UTC 2025
On Mon, 21 Jul 2025 12:33:30 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 23 additional commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into strIntrinCheck
> - Make `StringCoding` encoding intrinsics lenient
> - Fix `encodeISOArray` bounds checks and Javadoc
> - Disable `TestVerifyIntrinsicChecks` for GraalVM
> - Relax target array capacity check for intrinsic Java wrappers
>
> It's not possible to determine the required capacity of the target
> array in constant time, as Unicode code points may occupy either one
> or two `char` values. As a result, existing implementations typically
> invoke encoding methods in a loop, handling each unmappable character
> on a case-by-case basis. For an example, see
> `sun.nio.cs.DoubleByte.Encoder::encode`.
> - Fix out-of-bounds in `sun.nio.cs.SingleByte.Encoder::encodeArrayLoop`
> - Replace casting with `as_Region()` in `generate_string_range_check`
> - Duplicate affected tests with `-XX:+VerifyIntrinsicChecks` variants
> - Fix compiler error in `generate_string_range_check`
> - Add test verifying the effectiveness of `VerifyIntrinsicChecks`
> - ... and 13 more: https://git.openjdk.org/jdk/compare/514f29e4...f69374fb
Needed to replace all `Preconditions` invocations throwing `AIOOB` on failure with a more lenient approach that returns 0 on out-of-bounds, because,
1. this matches the compiler intrinsic behavior
2. there are several (i.e., ~7) `sun.nio.cs` classes that depend on this lenient behavior. I needed to either fix(?) these 7 classes or make the intrinsic wrappers more lenient
-------------
PR Comment: https://git.openjdk.org/jdk/pull/25998#issuecomment-3096649239
More information about the core-libs-dev
mailing list