Skip to content

8159695: Arguments::atojulong() fails to detect overflows #428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ktakakuri
Copy link
Contributor

@ktakakuri ktakakuri commented Jan 25, 2024

Hi all, this is a backport of JDK-8159695: Arguments::atojulong() fails to detect overflows.
The bug reported is reproducible in JDK8, so this patch should be applied.
The original patch does not apply cleanly, and the following modifications are needed:

  • Lines related to hexadecimal numbers are skipped because support for hexadecimal numbers as a VM option, corresponding to JDK-8042885, was not introduced until JDK9.
  • The accompanying test to check the functionality of the atojulong() function is dropped because it is a GTest, which is not available in JDK8.

Note that atomull() was renamed to atojulong() in JDK9 as part of the enhancement JDK-8153073, which supports suffixes in memory size options.
Testing

  • Manually check the behaviour
    When a given value is too large to fit in a julong, the proper exception is raised with this patch.

without patch

java -XX:GCDrainStackTargetSize=999999999999999999999999999999 -XX:+PrintFlagsFinal -version | grep StackTarget

    uintx GCDrainStackTargetSize                   := 18446744073709551615                    {product}

with patch

java -XX:GCDrainStackTargetSize=999999999999999999999999999999 -XX:+PrintFlagsFinal -version
Improperly specified VM option 'GCDrainStackTargetSize=999999999999999999999999999999'
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.
  • hotspot_tier1

Thank you.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • JDK-8159695 needs maintainer approval
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8159695: Arguments::atojulong() fails to detect overflows (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/428/head:pull/428
$ git checkout pull/428

Update a local copy of the PR:
$ git checkout pull/428
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/428/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 428

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/428.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 25, 2024

👋 Welcome back ktakakuri! 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 openjdk bot changed the title Backport 992b6c464a9655438a808cf2e1979e04356123be 8159695: Arguments::atojulong() fails to detect overflows Jan 25, 2024
@openjdk
Copy link

openjdk bot commented Jan 25, 2024

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Jan 25, 2024
@mlbridge
Copy link

mlbridge bot commented Jan 25, 2024

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 22, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@openjdk
Copy link

openjdk bot commented Feb 29, 2024

⚠️ @ktakakuri This change is now ready for you to apply for maintainer approval. This can be done directly in each associated issue or by using the /approval command.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strtoull is apparently not available or not properly declared for windows-x86.

Copy link
Member

@phohensee phohensee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows-x86 GHA build will failure for other reasons (see e.g. #430), so approving this.

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 11, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

As phohensee has pointed out ,strtoull needs to be declared for Windows-x86. A corresponding fix exists in JDK9, but it's in a different context. I am currently working on backporting this fix in issue #434. Once issue #434 is integrated, the build failure will be resolved and GHA result is fine (see #434).

@ktakakuri
Copy link
Contributor Author

/approval request Fix Request 8u
This bug is reproducible in JDK8, and backporting this patch is needed.
The patch does not apply cleanly, with GTest and hexadecimal-related lines being dropped.
Low risk as it only relates to edge cases of the string-julong conversion.
For building in Windows x86, the appropriate macro definition is needed and the correspoding backport has been submitted.
Tesing: GHA, hotspot_tier1, and manually check the behaviour.

@openjdk
Copy link

openjdk bot commented Apr 16, 2024

@ktakakuri
8159695: The approval request has been created successfully.

@openjdk openjdk bot added the approval label Apr 16, 2024
@bridgekeeper
Copy link

bridgekeeper bot commented May 14, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 11, 2024

@ktakakuri This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Jun 11, 2024
@ktakakuri
Copy link
Contributor Author

/open

@openjdk openjdk bot reopened this Jun 11, 2024
@openjdk
Copy link

openjdk bot commented Jun 11, 2024

@ktakakuri This pull request is now open

@gnu-andrew
Copy link
Member

I think this one should depend on #434, not the other way around, so VS2010 builds pass.

@ktakakuri
Copy link
Contributor Author

I think it would be good to merge this backport after #434 is merged.
The fix is no problem as is.

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 22, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 2, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 1, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 30, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

2 similar comments
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 31, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 31, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 28, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 31, 2024

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 3, 2025

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@ktakakuri
Copy link
Contributor Author

This pull request is pending approval of the Fix Request.
I comment to not close.

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 12, 2025

@ktakakuri This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 9, 2025

@ktakakuri This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Apr 9, 2025
@ktakakuri
Copy link
Contributor Author

/open

@openjdk openjdk bot reopened this May 20, 2025
@openjdk
Copy link

openjdk bot commented May 20, 2025

@ktakakuri This pull request is now open

@openjdk openjdk bot removed the approval label Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

3 participants