Skip to content
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

8299677: Formatter.format might take a long time to format an integer or floating-point #1077

Closed
wants to merge 411 commits into from

Conversation

phohensee
Copy link
Member

@phohensee phohensee commented Jan 17, 2023

Simple almost clean backport of a potential DOS attack vector fix. Copyright date conflict plus changed

Flags.contains(f, Flags.ZERO_PAD)

to

f.contains(Flags.ZERO_PAD)

Passes new and old Formatter tests.


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-8299677: Formatter.format might take a long time to format an integer or floating-point (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1077

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 17, 2023

👋 Welcome back phh! 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 33412c102ce799ff2de3512df77e6e07d76acd36 8299677: Formatter.format might take a long time to format an integer or floating-point Jan 17, 2023
@openjdk
Copy link

openjdk bot commented Jan 17, 2023

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 17, 2023
@mlbridge
Copy link

mlbridge bot commented Jan 17, 2023

Webrevs

@phohensee
Copy link
Member Author

The pre-submit test failures occur because the new test, Padding.java, fails compilation by jtreg because Padding.java imports org.junit.jupiter. The latter is available in jtreg version 7.2, which I run on my test machines, but is not available in the version of jtreg used to run the pre-submit tests. The jtreg version used to run the pre-submit tests should be upgraded because otherwise backporting new tests will difficult/impossible without rewrites.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 24, 2023

@phohensee 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!

@phohensee
Copy link
Member Author

/open

@openjdk
Copy link

openjdk bot commented Feb 24, 2023

@phohensee This pull request is already open

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 24, 2023

@phohensee 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 21, 2023

@phohensee 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 21, 2023
@phohensee
Copy link
Member Author

/open

@openjdk openjdk bot reopened this Apr 22, 2023
@openjdk
Copy link

openjdk bot commented Apr 22, 2023

@phohensee This pull request is now open

@RealCLanger
Copy link
Contributor

The test seems to fail in GHA...

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Choose a reason for hiding this comment

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

LGTM.

@openjdk
Copy link

openjdk bot commented Apr 26, 2023

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

8299677: Formatter.format might take a long time to format an integer or floating-point

Reviewed-by: mdoerr

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 openjdk bot added the ready Pull request is ready to be integrated label Apr 26, 2023
@TheRealMDoerr
Copy link
Contributor

TheRealMDoerr commented Apr 26, 2023

The test seems to fail in GHA...

Indeed. Is this a localization issue?
expected: <1.2> but was: <1,2>
The test has passed with -Duser.language=en-US
Some tests use Locale.setDefault. Maybe we should add that upstream (the test is failing upstream on the same machine, too).

@bridgekeeper
Copy link

bridgekeeper bot commented May 24, 2023

@phohensee 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!

@phohensee
Copy link
Member Author

keep open.

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 13, 2023

@phohensee 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!

GoeLin and others added 5 commits August 28, 2023 20:49
…a and HeapByteBufferTest.java

Backport-of: 7c1ebcc4ce74bb06f7c911e59a86bcfb5c5da844
…ith exit code 97

Backport-of: 1f438a8a702034c2f10c0008e72395f526b15ef5
Backport-of: 16a4f02f2d4f5574af3b20f2f0c788d15dd503ac
Reviewed-by: mdoerr
Backport-of: 3b85f84f026973a2abdbce8d9baf1329c8a4ebf8
Reviewed-by: mdoerr
Backport-of: e7795851d2e02389e63950fef939084b18ec4bfb
shipilev and others added 13 commits August 28, 2023 20:49
…va fails

Backport-of: 812f475bc4ea84225e8bbb0b5a677eed0af864dd
Backport-of: 92d8326e4037605897d7c4eb4b3edb63a2fc11b0
Backport-of: 0901548833a0125f15fede64bc2e7dbe84fed42d
Reviewed-by: phh
Backport-of: 538f9557b87f750264231f04bfbc91d15f8af8c0
…libfreetype-dev

Reviewed-by: stuefe
Backport-of: 69d900d2ce97e5479020cff9a63c471d07e39989
…nput/output error" in java.lang.ProcessHandleImpl$Info.info0

Backport-of: d24b7b7026cf85f1aecf44f60819762872cfd5c1
Reviewed-by: stuefe
Backport-of: 9e4fc568a6f1a93c84a84d6cc5220c6eb4e546a5
…g: did not see the expected RSS reduction

Reviewed-by: mdoerr
Backport-of: ad34be1f329edc8e7155983835cc70d733c014b8
Reviewed-by: stuefe
Backport-of: 20e94784c9f7c30e95550c72aedb5e986a153114
… are corrupted on Japanese Windows

Reviewed-by: kevinw, cjplummer, phh
…it orientation

Backport-of: d00a767047ec41e233e711dbc5fe7b8818e72f28
Backport-of: a3d67231a71fbe37c509fcedd54c679b4644c0d9
@shipilev
Copy link
Member

shipilev commented Sep 5, 2023

Just pushed jtreg 7.3.1 update to 17u-dev. Try to merge from master and see if test works now, @phohensee.

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label Sep 5, 2023
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 5, 2023
@TheRealMDoerr
Copy link
Contributor

TheRealMDoerr commented Sep 6, 2023

The merge has messed up this PR. Can it be repaired or should we use a new one?
Oh, wait... The "Files changed" tab still looks good.

@phohensee
Copy link
Member Author

Indeed it does. :) Thanks for the review!

@phohensee
Copy link
Member Author

Martin, may I ask for a review of openjdk/jdk11u-dev#1667 as well? It's a clean backport from this one.

@phohensee
Copy link
Member Author

/integrate

Approved.

@openjdk
Copy link

openjdk bot commented Sep 7, 2023

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

  • caaf942: Merge
  • dc00ae4: 8299658: C1 compilation crashes in LinearScan::resolve_exception_edge
  • a44f8b0: 8314960: Add Certigna Root CA - 2

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 7, 2023
@openjdk openjdk bot closed this Sep 7, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 7, 2023
@openjdk
Copy link

openjdk bot commented Sep 7, 2023

@phohensee Pushed as commit 6b192d3.

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

@phohensee phohensee deleted the backport-8299677 branch September 7, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.