8371604: [lworld] SIGSEGV in InlineKlass::is_payload_marked_as_null #1752

Arraying · 2025-11-20T12:24:10Z

Hi all,

This patch fixes several int overflows relating to flat array element loads/stores.
I've included some gtests that address a subset of these changes. We should test flat arrays larger than 16GB in size, which is not something we can feasibly do in our tier1 jtreg tests. I've filed JDK-8372261 as a follow-up RFE to investigate this.

Testing: tiers 1-4, LocalityCheck.java run locally 100 times.

Progress

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

Issue

JDK-8371604: [lworld] SIGSEGV in InlineKlass::is_payload_marked_as_null (Bug - P2)

Reviewers

Coleen Phillimore (@coleenp - Committer)

Contributors

Frederic Parain <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1752

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1752.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-11-20T12:25:51Z

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

openjdk · 2025-11-20T12:27:06Z

@Arraying 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:

8371604: [lworld] SIGSEGV in InlineKlass::is_payload_marked_as_null

Co-authored-by: Frederic Parain <[email protected]>
Reviewed-by: coleenp

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 143 new commits pushed to the lworld branch:

a483e8c: 8209554: [lworld] ClassCastException thrown for JCK test instead of expected IllegalArgumentException
c73220a: 8372345: [lworld] Problem list JDK-8372341
29569fb: 8368801: [lworld] AOT warning message causes LoggingDeadlock2 test failure
... and 140 more: https://git.openjdk.org/valhalla/compare/a680c717f3204d2f878944b724c4a78560e4f913...lworld

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coleenp) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Arraying · 2025-11-20T15:48:16Z

The Skara bot seems to be malfunctioning.
/touch

openjdk · 2025-11-20T15:49:26Z

@Arraying The pull request is being re-evaluated and the inactivity timeout has been reset.

mlbridge · 2025-11-20T15:52:42Z

Webrevs

fparain · 2025-11-20T18:02:48Z

There's another occurance of this int overflow with the _offset field in DelayedFieldAccess (c1_GraphBuilder.hpp).

Arraying · 2025-11-21T09:24:21Z

There's another occurance of this int overflow with the _offset field in DelayedFieldAccess (c1_GraphBuilder.hpp).

Good catch, thanks. This _offset is used in JVMCI as an int, so I'll update it there as well.

fparain · 2025-11-21T14:07:32Z

Correction, the possible overflow is in DelayedLoadIndexed, not DelayedFieldAccess, thank you Paul for figuring this out.

Arraying · 2025-11-21T15:58:15Z

Thanks also to Tobias and Coleen for helping troubleshoot.
/contributor add fparain

openjdk · 2025-11-21T15:58:32Z

@Arraying
Contributor Frederic Parain <[email protected]> successfully added.

coleenp

This looks good. Did you have a jvmci change still?

coleenp · 2025-11-21T15:56:34Z

src/hotspot/share/oops/flatArrayOop.inline.hpp

  assert(is_within_bounds(index), "index out of bounds");

-  address addr = (address) base();
-  addr += (index << Klass::layout_helper_log2_element_size(lh));


Because index is an int, this could go negative right?

Arraying · 2025-11-21T16:07:27Z

This looks good. Did you have a jvmci change still?

Nope, I shouldn't need to change ciField anymore as far as I can tell, and Fred agrees. But thanks for being vigilant!

Arraying · 2025-11-21T16:59:44Z

I added a regression test, it doesn't actually take very long to run (I was mostly running slowdebug so I was under the impression it was much slower than it actually is) and the memory could be constrained by jtreg. Convenient.

I still think that even with this test, JDK-8372261 applies and should be investigated further.

coleenp

The test looks good! I'm glad you could add it.

Arraying added 4 commits November 19, 2025 15:17

Use ptrdiff_t in value_at_addr.

e45e106

Fix another overflow.

fafd5ce

Include test case.

ff7be30

Fix up tests.

a7dbd59

openjdk bot changed the title ~~8371604~~ 8371604: [lworld] SIGSEGV in InlineKlass::is_payload_marked_as_null Nov 20, 2025

Arraying marked this pull request as ready for review November 20, 2025 14:42

openjdk bot added the rfr Pull request is ready for review label Nov 20, 2025

C1 changes

49320b9

coleenp approved these changes Nov 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 21, 2025

Add regression test.

8b0289f

coleenp approved these changes Nov 21, 2025

View reviewed changes

8371604: [lworld] SIGSEGV in InlineKlass::is_payload_marked_as_null #1752

Are you sure you want to change the base?

8371604: [lworld] SIGSEGV in InlineKlass::is_payload_marked_as_null #1752

Conversation

Arraying commented Nov 20, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Nov 20, 2025

Uh oh!

openjdk bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Arraying commented Nov 20, 2025

Uh oh!

openjdk bot commented Nov 20, 2025

Uh oh!

mlbridge bot commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

fparain commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Arraying commented Nov 21, 2025

Uh oh!

fparain commented Nov 21, 2025

Uh oh!

Arraying commented Nov 21, 2025

Uh oh!

openjdk bot commented Nov 21, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Arraying Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Arraying commented Nov 21, 2025

Uh oh!

Arraying commented Nov 21, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Arraying commented Nov 20, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Nov 20, 2025 •

edited

Loading

mlbridge bot commented Nov 20, 2025 •

edited

Loading

fparain commented Nov 20, 2025 •

edited

Loading