RFR: 8374582: [REDO] Move input validation checks to Java for java.lang.StringCoding intrinsics

Volkan Yazici vyazici at openjdk.org
Tue Jan 20 18:54:09 UTC 2026 

On Mon, 12 Jan 2026 10:29:39 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

> ## Issue
> 
> This is a redo of [JDK-8361842](https://bugs.openjdk.org/browse/JDK-8361842) which was backed out by [JDK-8374210](https://bugs.openjdk.org/browse/JDK-8374210) due to C2-related regressions. The original change moved input validation checks for java.lang.StringCoding from the intrinsic to Java code (leaving the intrinsic check only with the `VerifyIntrinsicChecks` flag). Refer to the [original PR](https://github.com/openjdk/jdk/pull/25998) for details.
> 
> This additional issue happens because, in some cases, for instance when the Java checking code is not inlined and we give an out-of-range constant as input, we fold the data path but not the control path and we crash in the backend.
> 
> ## Causes
> 
> The cause of this is that the out-of-range constant (e.g. -1) floats into the intrinsic and there (assuming the input is valid) we add a constraint to its type to positive integers (e.g. to compute the array address) which makes it top.
> 
> ## Fix
> 
> A possible fix is to introduce an opaque node (OpaqueGuardNode) similar to what we do in `must_be_not_null` for values that we know cannot be null:
> https://github.com/openjdk/jdk/blob/ce721665cd61d9a319c667d50d9917c359d6c104/src/hotspot/share/opto/graphKit.cpp#L1484
> This will temporarily add the range check to ensure that C2 figures that out-of-range values cannot reach the intrinsic. Then, during macro expansion, we replace the opaque node with the corresponding constant (true/false) in product builds such that the actually unneeded guards are folded and do not end up in the emitted code.
> 
> # Testing
> 
> * Tier 1-3+
> * 2 JTReg tests added
>   * `TestRangeCheck.java` as regression test for the reported issue
>   * `TestOpaqueGuardNodes.java` to check that opaque guard nodes are added when parsing and removed at macro expansion

Marked as reviewed by vyazici (Committer).

Verified that 3c466d372b7 is a clean revert of 7e18de137c3 delivered in [JDK-8374210].

[JDK-8374210]: https://bugs.openjdk.org/browse/JDK-8374210

src/hotspot/share/opto/c2_globals.hpp line 680:

> 678:   develop(bool, VerifyIntrinsicChecks, false,                               \
> 679:           "Verify in intrinsic that Java level checks work as expected")    \
> 680:                                                                             \

I suggest removing the `VerifyIntrinsicChecks` flag. Given `OpaqueGuard` already verifies the value when `#ifdef ASSERT`, does `VerifyIntrinsicChecks` serve any purpose anymore?

src/hotspot/share/opto/library_call.hpp line 170:

> 168:                                     Node* length, bool char_count,
> 169:                                     bool halt_on_oob = false,
> 170:                                     bool is_opaque = false);

Do we really need to introduce two new toggles: `halt_on_oob` and `is_opaque`? At all call-sites either one of the following is used:

1. `halt_on_oob=true, is_opaque=!VerifyIntrinsicChecks`
2. defaults (i.e., `halt_on_oob=is_opaque=false`)

Can we instead only settle one, e.g., `halt_on_oob=VerifyIntrinsicChecks`?

src/hotspot/share/opto/loopopts.cpp line 1:

> 1: /*

What is the reason that the new `OpaqueGuard` is not taken into account in `PhaseIdealLoop::clone_iff`?

src/hotspot/share/opto/macro.cpp line 2565:

> 2563:         // Tests with OpaqueGuard nodes are implicitly known to be true or false. Replace the node with appropriate value. In debug builds,
> 2564:         // we leave the test in the graph to have an additional sanity check at runtime. If the test fails (i.e. a bug),
> 2565:         // we will execute a Halt node.

*Nit:* Can we adhere to the max. 120 (or even better, 80!) characters per line limit of the file?

src/hotspot/share/opto/macro.cpp line 2569:

> 2567:         _igvn.replace_node(n, n->in(1));
> 2568: #else
> 2569:         _igvn.replace_node(n, _igvn.intcon(0));

Curious: why do we invoke `intcon(0)` for `OpaqueGuard`, whereas it was `intcon(1)` for `OpaqueNotNull` slightly above?

src/hotspot/share/opto/opaquenode.hpp line 160:

> 158: // we keep the actual checks as additional verification code (i.e. removing OpaqueGuardNode and use the BoolNode
> 159: // inputs instead).
> 160: class OpaqueGuardNode : public Node {

With the `OpaqueGuardNode::is_positive` flag gone, `OpaqueGuardNode` looks pretty much identical to `OpaqueNotNullNode`. Is there a code reuse opportunity we can take advantage of?

test/hotspot/jtreg/compiler/intrinsics/TestVerifyIntrinsicChecks.java line 1:

> 1: /*

Since the `VerifyIntrinsicChecks` flag is gone, AFAICT, all following changes can be reverted:


git rm test/hotspot/jtreg/compiler/intrinsics/TestVerifyIntrinsicChecks.java
git checkout upstream/HEAD -- \
test/hotspot/jtreg/compiler/intrinsics/string/TestCountPositives.java \
test/hotspot/jtreg/compiler/intrinsics/string/TestEncodeIntrinsics.java \
test/hotspot/jtreg/compiler/intrinsics/string/TestHasNegatives.java \
test/hotspot/jtreg/compiler/patches/java.base/java/lang/Helper.java

test/hotspot/jtreg/compiler/intrinsics/string/TestRangeCheck.java line 32:

> 30:  *      -XX:CompileCommand=inline,java.lang.StringCoding::*
> 31:  *      -XX:CompileCommand=exclude,jdk.internal.util.Preconditions::checkFromIndexSize
> 32:  *      -XX:CompileCommand=compileonly,compiler.intrinsics.string.TestRangeCheck::test

Is this necessary? (This wasn't used in `TestStringConstruction`.)

test/hotspot/jtreg/compiler/intrinsics/string/TestRangeCheck.java line 58:

> 56:             // cut off the dead code. As a result, -1 is fed as input into the
> 57:             // StringCoding::countPositives0 intrinsic which is replaced by TOP and causes a
> 58:             // failure in the matcher.

I'd appreciate it if we can be more elaborate for less C2-illiterate people like myself. 😇

Suggestion:

            // Calling `StringCoding::countPositives`, which is a "front door"
            // to the `StringCoding::countPositives0` intrinsic.
            // `countPositives` validates its input using
            // `Preconditions::checkFromIndexSize`, which also maps to an
            // intrinsic. When `checkFromIndexSize` is not inlined, C2 does not
            // know about the explicit range checks, and does not cut off the
            // dead code. As a result, an invalid value (e.g., `-1`) can be fed
            // as input into the `countPositives0` intrinsic, got replaced
            // by TOP, and cause a failure in the matcher.

-------------

PR Review: https://git.openjdk.org/jdk/pull/29164#pullrequestreview-3681112226
PR Comment: https://git.openjdk.org/jdk/pull/29164#issuecomment-3738455817
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2689568427
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2687948444
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2685859575
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2685838328
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2705884654
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2705885810
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2704760982
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2689735070
PR Review Comment: https://git.openjdk.org/jdk/pull/29164#discussion_r2689780537

More information about the security-dev mailing list