On Tue, 12 Aug 2025 08:17:58 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 one additional > commit since the last revision: > > Remove `@apiNote` in `encodeISOArray()` Javadoc > > Those who are touching to these methods should well be > aware of the details elaborated in the `@apiNote`, no > need to put it on a display. src/java.base/share/classes/sun/nio/cs/ISO_8859_1.java line 159: > 157: private static int encodeISOArray(char[] sa, int sp, > 158: byte[] da, int dp, int len) { > 159: if ((sp | dp | len) < 0 || I think this change is OK; I understand it removes uncertainty about inlining of the checking helper method. In general, though, I think we should be using the Preconditions methods, instead of hand-rolling checks from random-looking Java operators. (Sorry, but they will look random to the person reading this years later!) (I think Objects::rNN is overkill; I do believe that the implicit null checks are easy to reason about. I guess tastes vary here. O::rNN is marked force-inline, so that should not be a problem to throw it into the checking logic, as an explicit null check.) Anyway, I would like to a see a comment here, in the code, saying why the Preconditions methods are not used at this point. If there was an inlining problem, let's acknowledge it. It's OK to make String code be a special case (with hand-rolled checks, carefully tuned). Let's document how we did that. One point: I don't think we expect these bulk string operations to occur in a containing hot loop, so the intrinsic value of Precondition methods (to contribute to RCE optimizations) is not required. Still, on the whole, it is usually better to use a named method than a tricky Java operator expression – much as we all love those! ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2277764353
