8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB #21324

kimbarrett · 2024-10-03T12:50:55Z

Please review this change to TypeRawPtr::add_offset to prevent a compiler from
inferring things based on prior pointer arithmetic not invoking UB. As noted in
the bug report, clang is actually doing this.

To accomplish this, changed to integral arithmetic. Also added over/underflow
checks.

Also made a couple of minor touchups. Replaced an implicit conversion to bool
with an explicit compare to nullptr (per style guide). Removed a no longer
needed dummy return after a (now) noreturn function.

Testing: mach5 tier1-7
That testing was with calls to "fatal" for the over/underflow cases and the
sum==0 case. There were no hits. I'm not sure how to construct a test that
would hit those.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB (Bug - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) Review applies to cc1f2ac8
Dean Long (@dean-long - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21324/head:pull/21324
$ git checkout pull/21324

Update a local copy of the PR:
$ git checkout pull/21324
$ git pull https://git.openjdk.org/jdk.git pull/21324/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21324

View PR using the GUI difftool:
$ git pr show -t 21324

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21324.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-03T12:51:46Z

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-03T12:53:37Z

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB

Reviewed-by: dlong, kvn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 99 new commits pushed to the master branch:

a45abf1: 8341860: ProblemList applications/ctw/modules/java_base_2.java on linux-x64
593c27e: 8341535: sun/awt/font/TestDevTransform.java fails with RuntimeException: Different rendering
3180aaa: 8341832: Incorrect continuation address of synthetic SIGSEGV for APX in product builds
3ab519f: 8341424: GHA: Collect hs_errs from build time failures
a24525b: 8339320: Optimize ClassFile Utf8EntryImpl#inflate
fcc9c8d: 8341854: Incorrect clearing of ZF in fast_unlock_lightweight on x86
38c1d65: 8337980: Javac allows invocation of an inherited instance method from a static method
950e3a7: 8341625: Improve ZipFile validation of the END header
e704c05: 8340547: Starting many threads can delay safepoints
c30ad01: 8325495: C2: implement optimization for series of Add of unique value
... and 89 more: https://git.openjdk.org/jdk/compare/7d524d7e378430afb3a262e8fe544bd1be22748c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-03T12:54:26Z

@kimbarrett The following label will be automatically applied to this pull request:

hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-10-03T12:56:47Z

Webrevs

vnkozlov

Looks reasonable. Just one nit comment.

vnkozlov · 2024-10-03T16:38:34Z

src/hotspot/share/opto/type.cpp

@@ -3133,7 +3133,7 @@ const TypeRawPtr *TypeRawPtr::make( enum PTR ptr ) {
 }

 const TypeRawPtr *TypeRawPtr::make( address bits ) {
-  assert( bits, "Use TypePtr for null" );
+  assert( bits != nullptr, "Use TypePtr for null" );


Please, remove spaces after open and before close ().

I'm not fond of those spaces, but they follow the style used throughout this file.

Although it looks like only 1/3 of the asserts in this file have extra whitespace, including the one being touched
here. So sure, I can remove the extraneous whitespace from this function, since touching it anyway.

vnkozlov

Good.

vnkozlov · 2024-10-04T16:03:36Z

Side note: please enable GHA testing for your repo.

TobiHartmann · 2024-10-07T05:43:47Z

What about using intptr_t for TypeRawPtr::_bits instead?

kimbarrett · 2024-10-07T15:13:47Z

What about using intptr_t for TypeRawPtr::_bits instead?

That has more fannout, into code I'm not familiar with. The proposed change fixes the immediate "miscompilation".
A change of the type could be done as a further enhancement, if that makes sense to do. I'd rather leave that to
someone from the compiler team. If that approach is what's wanted to fix the immediate problem, then I'm going
to want to hand this issue off. Also, uintptr_t might be more appropriate than intptr_t.

dean-long · 2024-10-07T21:27:58Z

src/hotspot/share/opto/type.cpp

@@ -3223,14 +3223,22 @@ const TypePtr* TypeRawPtr::add_offset(intptr_t offset) const {
  case TypePtr::NotNull:
    return this;
  case TypePtr::Null:
+    return make( (address)offset );


Shouldn't this assert that _bits == 0? Looking at the code, however, I can't find anywhere that we actually create a TypeRawPtr with TypePtr::Null. We could probably remove this case and let it fall through to the default ShouldNotReachHere().

Initialization of TypePtr::NULL_PTR here:

jdk/src/hotspot/share/opto/type.cpp

Line 538 in 4d50cbb

TypePtr::NULL_PTR= TypePtr::make(AnyPtr, TypePtr::Null, 0);

I saw that too, but it creates a TypePtr, not a TypeRawPtr.

Oh, you are right. And TypeRawPtr::make asserts the PTR is neither Constant nor Null. Which makes
both switch cases under modification here supposedly unreachable. That would explain why I never hit
either after running lots of tests. All of the change proposed here can be eliminated, and instead change
both cases to fall through to the default ShouldNotReachHere(). (And that would be another way to
remove the -Wzero-as-null-pointer-constant warning that was how I got here in the first place. :) )

There's TypeRawPtr::make(enum PTR ptr) which doesn't allow Constant or Null, but we are using TypeRawPtr::make(address bits) here.
We may need to keep the Constant case. I wouldn't be surprised if there was a way to trigger that path using Unsafe.

Yeah, keeping it makes sense. I've removed the TypePtr::Null case, allowing that one to default to
ShuoldNotReachHere().

TobiHartmann · 2024-10-08T15:17:44Z

What about using intptr_t for TypeRawPtr::_bits instead?

That has more fannout, into code I'm not familiar with. The proposed change fixes the immediate "miscompilation". A change of the type could be done as a further enhancement, if that makes sense to do. I'd rather leave that to someone from the compiler team. If that approach is what's wanted to fix the immediate problem, then I'm going to want to hand this issue off. Also, uintptr_t might be more appropriate than intptr_t.

Okay, that's fine with me.

dean-long

Looks good.

kimbarrett · 2024-10-11T21:10:32Z

Thanks for reviews, @vnkozlov and @dean-long

kimbarrett · 2024-10-11T21:10:38Z

/integrate

openjdk · 2024-10-11T21:11:13Z

Going to push as commit 0a57fe1.
Since your change was applied there have been 134 commits pushed to the master branch:

1f6bd0c: 8341972: java/awt/dnd/DnDRemoveFocusOwnerCrashTest.java timed out after JDK-8341257
7276a1b: 8341784: Refactor TypeVect to use a BasicType instead of a const Type*
6133866: 8341070: javac fails with an exception when compiling import module under source level 8
519544c: 8341909: G1: Add region index to region printer output
7c0dbf8: 8341820: Check return value of hcreate_r
ae41daf: 8341880: RISC-V: riscv_vector.h native build fails with gcc13 after JDK-8320500
ace44e5: 8341806: Gcc version detection failure on Alinux3
472db92: 8341900: Optimize DirectCodeBuilder writeBody
24eb360: 8341906: Optimize ClassFile writing BufBuffer
cd4981c: 8341257: Open source few DND tests - Set1
... and 124 more: https://git.openjdk.org/jdk/compare/7d524d7e378430afb3a262e8fe544bd1be22748c...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-11T21:11:23Z

@kimbarrett Pushed as commit 0a57fe1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

fix

4883371

openjdk bot added the rfr Pull request is ready for review label Oct 3, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 3, 2024

vnkozlov reviewed Oct 3, 2024

View reviewed changes

remove surrounding whitespace

cc1f2ac

vnkozlov approved these changes Oct 4, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 4, 2024

dean-long reviewed Oct 7, 2024

View reviewed changes

remove unreachable TypePtr::Null case

c3dc62e

openjdk bot removed the ready Pull request is ready to be integrated label Oct 9, 2024

dean-long approved these changes Oct 9, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 9, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 11, 2024

openjdk bot closed this Oct 11, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 11, 2024

kimbarrett deleted the fix-add-offset branch October 11, 2024 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB #21324

8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB #21324

kimbarrett commented Oct 3, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 3, 2024

openjdk bot commented Oct 3, 2024 •

edited

Loading

openjdk bot commented Oct 3, 2024

mlbridge bot commented Oct 3, 2024 •

edited

Loading

vnkozlov left a comment

vnkozlov Oct 3, 2024

kimbarrett Oct 4, 2024

kimbarrett Oct 4, 2024

vnkozlov left a comment

vnkozlov commented Oct 4, 2024

TobiHartmann commented Oct 7, 2024

kimbarrett commented Oct 7, 2024

dean-long Oct 7, 2024

kimbarrett Oct 7, 2024

dean-long Oct 7, 2024

kimbarrett Oct 8, 2024

dean-long Oct 8, 2024

kimbarrett Oct 9, 2024

TobiHartmann commented Oct 8, 2024

dean-long left a comment

kimbarrett commented Oct 11, 2024

kimbarrett commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB #21324

8341178: TypeRawPtr::add_offset may be "miscompiled" due to UB #21324

Conversation

kimbarrett commented Oct 3, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 3, 2024

openjdk bot commented Oct 3, 2024 • edited Loading

openjdk bot commented Oct 3, 2024

mlbridge bot commented Oct 3, 2024 • edited Loading

Webrevs

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

vnkozlov commented Oct 4, 2024

TobiHartmann commented Oct 7, 2024

kimbarrett commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TobiHartmann commented Oct 8, 2024

dean-long left a comment

Choose a reason for hiding this comment

kimbarrett commented Oct 11, 2024

kimbarrett commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

kimbarrett commented Oct 3, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 3, 2024 •

edited

Loading

mlbridge bot commented Oct 3, 2024 •

edited

Loading