-
Enhancement
-
Resolution: Unresolved
-
P2
-
9
It has been a source of confusion where the input validation shall be implemented for intrinsics and what validations are present exactly. For a developer implementing an intrinsic, it is not clear to which extent the input is validated at the Java wrapper; neither it is clear for the developer of the Java wrapper to which extent the input is validated at the intrinsic. In several occasions this "who validates what" information is communicated using comments, yet it is a matter of time that that particular comment, which is completely detached and far away from its subject, gets misaligned with the code. This story, limited with String-related intrinsics, aims to bring clarity to this dilemma:
- Always have a Java wrapper for an intrinsic, and make the actual method (mapped to the intrinsic under the hood) private
- Always validate all input at the Java wrapper
- Always validate all input at the intrinsic, but place them behind a VM flag, which is disabled by default
### Implementation details
The concrete goal of this work is to, for `java.lang.String*` classes,
1. Move `@IntrinsicCandidate`-annotated public methods (in Java code) to a private one, and wrap them with a public "front door" method
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. Wrap all input validation checks in the intrinsic code (add if missing) in a `if (VerifyIntrinsicChecks) { ... }` block
Following preliminary work needs to be carried out as well:
1. Add `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.
### Implementation notes
- `@IntrinsicCandidate`-annotated constructors should be left untouched.
### Noteworthy related tickets
-JDK-8158168 reports missing bounds checks for some String intrinsics
-JDK-8159129 reports missing checks for compressByte/inflateByte
-JDK-8356380 reports missing bounds checks (at the Java wrapper) for `JavaLangAccess::countPositives`
### Noteworthy related discussions
http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022936.html
> Also, independently, I'm having a hard time figuring out how
> to prove that, between the Java code and the HotSpot
> intrinsic (and interpreter/C1/C2), the range checking logic
> is consistently applied.
>
> My best advice for this is, always, put the range checking logic
> in one place, in Java code. This makes me profoundly suspicious
> of *any* public method that is also marked @HSIC, if it takes any
> array arguments. If the public method is an intrinsic, it means we
> are trusting C++ IR-assembly code to securely check Java array
> bounds. That is unnatural, and (as history proves repeatedly)
> subject to errors.
>
> Instead, we should have a Java routine which checks bounds,
> and a *private* intrinsic which is provably called *only after*
> the Java checker is called.
https://github.com/openjdk/jdk/pull/24982#discussion_r2087493446
> An intrinsic is a private "back door" in the VM, and must not be
> accessed (except perhaps in carefully documented unusual
> circumstances) outside of its own source file. There should
> be a non-private "front door" in the same source file which
> applies all relevant checks before calling the intrinsic.
>
> Such checks need to be at least as strong as the built-in
> JVM range checks, null checks, and type checks that would
> be performed, if the intrinsic were implemented in 100%
> pure Java bytecodes.
>
> In performance evaluations, such checks may appear to be
> redundant. But they must not be removed. Instead, whatever
> other checks are redundant with them (there must be some!)
> should be brought into the same C2 compilation task as the
> "front door" checks. This requires making the two sets of
> checks (if there are two) look similar enough that C2 (or another
> JIT) can recognize their redundance. Bringing the checks into the
> same task might require a ForceInline annotation (which is
> allowed only inside the JDK). Recognizing the checks as
> redundant might require a C2 bug fix or RFE.
- Always have a Java wrapper for an intrinsic, and make the actual method (mapped to the intrinsic under the hood) private
- Always validate all input at the Java wrapper
- Always validate all input at the intrinsic, but place them behind a VM flag, which is disabled by default
### Implementation details
The concrete goal of this work is to, for `java.lang.String*` classes,
1. Move `@IntrinsicCandidate`-annotated public methods (in Java code) to a private one, and wrap them with a public "front door" method
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. Wrap all input validation checks in the intrinsic code (add if missing) in a `if (VerifyIntrinsicChecks) { ... }` block
Following preliminary work needs to be carried out as well:
1. Add `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.
### Implementation notes
- `@IntrinsicCandidate`-annotated constructors should be left untouched.
### Noteworthy related tickets
-
-
-
### Noteworthy related discussions
http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022936.html
> Also, independently, I'm having a hard time figuring out how
> to prove that, between the Java code and the HotSpot
> intrinsic (and interpreter/C1/C2), the range checking logic
> is consistently applied.
>
> My best advice for this is, always, put the range checking logic
> in one place, in Java code. This makes me profoundly suspicious
> of *any* public method that is also marked @HSIC, if it takes any
> array arguments. If the public method is an intrinsic, it means we
> are trusting C++ IR-assembly code to securely check Java array
> bounds. That is unnatural, and (as history proves repeatedly)
> subject to errors.
>
> Instead, we should have a Java routine which checks bounds,
> and a *private* intrinsic which is provably called *only after*
> the Java checker is called.
https://github.com/openjdk/jdk/pull/24982#discussion_r2087493446
> An intrinsic is a private "back door" in the VM, and must not be
> accessed (except perhaps in carefully documented unusual
> circumstances) outside of its own source file. There should
> be a non-private "front door" in the same source file which
> applies all relevant checks before calling the intrinsic.
>
> Such checks need to be at least as strong as the built-in
> JVM range checks, null checks, and type checks that would
> be performed, if the intrinsic were implemented in 100%
> pure Java bytecodes.
>
> In performance evaluations, such checks may appear to be
> redundant. But they must not be removed. Instead, whatever
> other checks are redundant with them (there must be some!)
> should be brought into the same C2 compilation task as the
> "front door" checks. This requires making the two sets of
> checks (if there are two) look similar enough that C2 (or another
> JIT) can recognize their redundance. Bringing the checks into the
> same task might require a ForceInline annotation (which is
> allowed only inside the JDK). Recognizing the checks as
> redundant might require a C2 bug fix or RFE.
- duplicates
-
JDK-8356380 Check bounds for JavaLangAccess::countPositives
-
- Closed
-
- relates to
-
JDK-8155608 String intrinsic range checks are not strict enough
-
- Resolved
-
-
JDK-8159129 TestStringIntrinsicRangeChecks fails w/ No exception thrown for compressByte/inflateByte
-
- Resolved
-
-
JDK-8142303 C2 compilation fails with "bad AD file"
-
- Closed
-
-
JDK-8158168 Missing bounds checks for some String intrinsics
-
- Closed
-