-
Notifications
You must be signed in to change notification settings - Fork 160
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back ktakakuri! A progress list of the required criteria for merging this PR into |
This backport pull request has now been updated with issue from the original commit. |
@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! |
|
There was a problem hiding this 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.
There was a problem hiding this 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.
❗ This change is not yet ready to be integrated. |
@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! |
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). |
/approval request Fix Request 8u |
@ktakakuri |
@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 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 |
@ktakakuri This pull request is now open |
I think this one should depend on #434, not the other way around, so VS2010 builds pass. |
I think it would be good to merge this backport after #434 is merged. |
@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! |
This pull request is pending approval of the Fix Request. |
@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! |
This pull request is pending approval of the Fix Request. |
@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! |
This pull request is pending approval of the Fix Request. |
@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
@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 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! |
This pull request is pending approval of the Fix Request. |
@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! |
This pull request is pending approval of the Fix Request. |
@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! |
This pull request is pending approval of the Fix Request. |
@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! |
This pull request is pending approval of the Fix Request. |
@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 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 |
@ktakakuri This pull request is now open |
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:
Note that atomull() was renamed to atojulong() in JDK9 as part of the enhancement JDK-8153073, which supports suffixes in memory size options.
Testing
When a given value is too large to fit in a julong, the proper exception is raised with this patch.
without patch
with patch
Thank you.
Progress
Issue
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