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. This is lovely work. I've left a few suggestions which you may wish to take action on. src/hotspot/share/opto/library_call.cpp line 946: > 944: Node* count, > 945: bool char_count, > 946: bool halt_on_oob) { Adding halt_on_oob is a pro move. It makes it very clear that something very bad is afoot. Now I see why A/B testing was harder to do in the jtreg tests, especially for the "throwing" case. Just a note for the future (no change requested): The InternalError exception is sometimes used for these kinds of "crash and burn" conditiosn. (See the code in java.lang.invoke.) Users can try to catch IE, but they are advised not to, and it will therefore crash the thread (at least), as an unhandled throw. The advantage of using IE is that a jtreg harness can catch it much more easily (as opposed to a hard VM halt). ------------- Marked as reviewed by jrose (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/25998#pullrequestreview-3122211152 PR Review Comment: https://git.openjdk.org/jdk/pull/25998#discussion_r2277794171
