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 C++ methods, 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"); > } > } > } > > 2. Comp... src/java.base/share/classes/java/lang/StringCoding.java line 93: > 91: public static int countPositives(byte[] ba, int off, int len) { > 92: Objects.requireNonNull(ba, "ba"); > 93: Objects.checkFromIndexSize(off, len, ba.length); I recall core libraries intentionally avoided this because of performance problems. Is it possible for us to say trust the `len` argument to be non-negative? That allows us to simplify this to `Objects.checkIndex(off, ba.length - len)`. See this usage in perf-sensitive FFM API: https://github.com/openjdk/jdk/blob/149882416a956dec728a964c150b826dd589908f/src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java#L401 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2169545990
