8352724: Verify bounds for primitive array reads in JVMCI #24200

pecimuth · 2025-03-24T14:31:54Z

This PR adds a bounds check for primitive array reads in JVMCI. When a JVMCI compiler attempts to read after the last array element (from the padding of the allocated object), JVMCI should throw an exception instead of returning a garbage value. The check added in this PR handles both primitive and object reads.

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-8352724: Verify bounds for primitive array reads in JVMCI (Enhancement - P4)

Reviewers

Doug Simon (@dougxc - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24200

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-24T14:32:55Z

Hi @pecimuth, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user pecimuth" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2025-03-24T14:33:20Z

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

8352724: Verify bounds for primitive array reads in JVMCI

Reviewed-by: dnsimon

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 375 new commits pushed to the master branch:

43b1947: 8344883: Force clients to explicitly pass mem_tag value, even if it is mtNone
d748bb5: 8343157: Examine large files for character encoding/decoding
313c34a: 8354513: Bug in j.u.l.Handler deadlock test allows null pointer during race condition
... and 372 more: https://git.openjdk.org/jdk/compare/de580090cd9ada313a878975b9f183045d293684...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.

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 (@dougxc) 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).

openjdk · 2025-03-24T14:34:11Z

@pecimuth The following labels will be automatically applied to this pull request:

graal
hotspot-compiler

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

pecimuth · 2025-03-24T18:31:17Z

/covered

bridgekeeper · 2025-03-24T18:32:33Z

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

dougxc

LGTM and trival.

openjdk · 2025-04-14T11:26:01Z

⚠️ @pecimuth the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8352724
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

dougxc

Actually, can you please add some extra tests to TestConstantReflectionProvider.java for out-of-bounds reads.

mlbridge · 2025-04-14T12:56:15Z

Webrevs

dougxc · 2025-04-14T14:39:35Z

.../jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestConstantReflectionProvider.java

+        for (ConstantValue cv : readConstants(ArrayConstants.class)) {
+            if (cv.boxed != null && cv.boxed.getClass().isArray()) {
+                JavaKind kind = metaAccess.lookupJavaType(cv.value).getComponentType().getJavaKind();
+                long offset = metaAccess.getArrayBaseOffset(kind) + (long) metaAccess.getArrayIndexScale(kind) * Array.getLength(cv.boxed);


If I understand correctly, this tests a read of an element one past the end of the array.
Can you please also add a test for a read that is partially out-of-bounds:

long offset = 1 + metaAccess.getArrayBaseOffset(kind) + (long) metaAccess.getArrayIndexScale(kind) * (Array.getLength(cv.boxed) - 1);

I added a test for a long read from array[array.index - 1] because adding + 1 would make the read unaligned (which is also not allowed). Please check it out.

dougxc

Thanks for the new tests.

pecimuth · 2025-04-14T16:43:51Z

/integrate

openjdk · 2025-04-14T16:44:36Z

@pecimuth
Your change (at version 3661b21) is now ready to be sponsored by a Committer.

dougxc · 2025-04-14T18:30:21Z

/sponsor

openjdk · 2025-04-14T18:31:30Z

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

46a6fc8: 8353483: Open source some JProgressBar tests
d684867: 8346230: [perf] scalability issue for the specjvm2008::xml.transform workload
43b1947: 8344883: Force clients to explicitly pass mem_tag value, even if it is mtNone
... and 374 more: https://git.openjdk.org/jdk/compare/de580090cd9ada313a878975b9f183045d293684...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-14T18:31:59Z

@dougxc @pecimuth Pushed as commit de0e648.

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

Verify bounds for primitive array reads.

1bfeb51

bridgekeeper bot added the oca Needs verification of OCA signatory status label Mar 24, 2025

openjdk bot added graal graal-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels Mar 24, 2025

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Mar 24, 2025

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Apr 14, 2025

openjdk bot added the rfr Pull request is ready for review label Apr 14, 2025

dougxc approved these changes Apr 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 14, 2025

dougxc suggested changes Apr 14, 2025

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Apr 14, 2025

Test reads after last array element in JVMCI.

7475f46

pecimuth requested a review from dougxc April 14, 2025 14:32

dougxc reviewed Apr 14, 2025

View reviewed changes

Test array reads that are partially out of bounds.

3661b21

pecimuth requested a review from dougxc April 14, 2025 16:35

dougxc approved these changes Apr 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 14, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 14, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 14, 2025

openjdk bot closed this Apr 14, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 14, 2025

8352724: Verify bounds for primitive array reads in JVMCI #24200

8352724: Verify bounds for primitive array reads in JVMCI #24200

Uh oh!

Conversation

pecimuth commented Mar 24, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 24, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Mar 24, 2025

Uh oh!

pecimuth commented Mar 24, 2025

Uh oh!

bridgekeeper bot commented Mar 24, 2025

Uh oh!

dougxc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Apr 14, 2025

Uh oh!

dougxc left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

dougxc Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pecimuth Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

dougxc left a comment

Choose a reason for hiding this comment

Uh oh!

pecimuth commented Apr 14, 2025

Uh oh!

openjdk bot commented Apr 14, 2025

Uh oh!

dougxc commented Apr 14, 2025

Uh oh!

openjdk bot commented Apr 14, 2025

Uh oh!

openjdk bot commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

pecimuth commented Mar 24, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 24, 2025 •

edited

Loading

dougxc left a comment •

edited

Loading

mlbridge bot commented Apr 14, 2025 •

edited

Loading

dougxc Apr 14, 2025 •

edited

Loading