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

8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly #516

Closed
wants to merge 13 commits into from

Conversation

igraves
Copy link
Member

@igraves igraves commented Oct 5, 2020

The java.util.Formatter format specifies support for field widths, argument indexes, or precision lengths of a field that relate to the variadic arguments supplied to the formatter. These numbers are specified by integers, sometimes negative. For argument index, it's specified in the documentation that the highest allowed argument is limited by the largest possible index of an array (ie the largest possible variadic index), but for the other two it's not defined. Moreover, what happens when a number field in a string is too large or too small to be represented by a 32-bit integer type is not defined.

This fix adds documentation to specify what error behavior occurs during these cases. Additionally it adds an additional exception type to throw when an invalid argument index is observed.

A CSR will be required for this PR.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/516/head:pull/516
$ git checkout pull/516

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 5, 2020

👋 Welcome back igraves! 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 added the rfr Pull request is ready for review label Oct 5, 2020
@openjdk
Copy link

openjdk bot commented Oct 5, 2020

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

  • core-libs
  • i18n

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.

@openjdk openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Oct 5, 2020
@igraves
Copy link
Member Author

igraves commented Oct 5, 2020

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Oct 5, 2020
@openjdk
Copy link

openjdk bot commented Oct 5, 2020

@igraves this pull request will not be integrated until the CSR request JDK-8253875 for issue JDK-8253459 has been approved.

@mlbridge
Copy link

mlbridge bot commented Oct 5, 2020

Webrevs

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Choose a reason for hiding this comment

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

Is the new exception type useful? yes, it matches the previous pattern.
But it does not (and none of the IllegalFormatException subclasses) produce a readable message with the offending value. So the developer will not see anything useful.
The fine grained exceptions provide little value.

@igraves
Copy link
Member Author

igraves commented Oct 6, 2020

I've been on the fence about this, personally. The Formatter uses pretty fine-grained exception types for error conditions. I'd be okay discontinuing this practice here, but am not sure what to replace this with. Perhaps we enable IllegalFormatException to be, itself, public and instantiable ?

@mlbridge
Copy link

mlbridge bot commented Oct 15, 2020

Mailing list message from Jason Mehrens on i18n-dev:

Ian,

Should IllegalFormatArgumentIndexException.java provide an override of getMessage? It appears that is done in the following:

https://github.com/openjdk/jdk/blob/9b07ef33b6e07afae1a8bfa034200eb3aab1f94f/src/java.base/share/classes/java/util/IllegalFormatWidthException.java
https://github.com/openjdk/jdk/blob/9b07ef33b6e07afae1a8bfa034200eb3aab1f94f/src/java.base/share/classes/java/util/IllegalFormatPrecisionException.java

Otherwise the index won't be rendered when toString or printStackTrace is invoked.

Jason

________________________________________
From: core-libs-dev <core-libs-dev-retn at openjdk.java.net> on behalf of Ian Graves <igraves at openjdk.java.net>
Sent: Wednesday, October 14, 2020 1:39 PM
To: core-libs-dev at openjdk.java.net; i18n-dev at openjdk.java.net
Subject: Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v4]

The `java.util.Formatter` format specifies support for field widths, argument indexes, or precision lengths of a field
that relate to the variadic arguments supplied to the formatter. These numbers are specified by integers, sometimes
negative. For argument index, it's specified in the documentation that the highest allowed argument is limited by the
largest possible index of an array (ie the largest possible variadic index), but for the other two it's not defined.
Moreover, what happens when a number field in a string is too large or too small to be represented by a 32-bit integer
type is not defined. This fix adds documentation to specify what error behavior occurs during these cases.
Additionally it adds an additional exception type to throw when an invalid argument index is observed. A CSR will be
required for this PR.

Ian Graves has updated the pull request incrementally with one additional commit since the last revision:

Tweaking verbiage

-------------

Changes:
- all: https://git.openjdk.java.net/jdk/pull/516/files
- new: https://git.openjdk.java.net/jdk/pull/516/files/391449a8..9b07ef33

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=516&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=516&range=02-03

Stats: 18 lines in 2 files changed: 0 ins; 5 del; 13 mod
Patch: https://git.openjdk.java.net/jdk/pull/516.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/516/head:pull/516

PR: https://git.openjdk.java.net/jdk/pull/516

Copy link

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

*
* <p> Values of <i>width</i> must be in the range one to
* {@link Integer#MAX_VALUE}, inclusive, otherwise
* {@link IllegalFormatWidthException} will be thrown
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

* {@link IllegalFormatWidthException} will be thrown
* Note that widths can appear to have a negative value, but the negative sign
* is a <i>flag</i>. For example in the format string {@code "%-20s"} the
* <i>width</i> is <i>20</i> and the <i>flag</i> is "-".</p>
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@@ -2783,8 +2799,11 @@ private int index(String s, int start, int end) {
try {
// skip the trailing '$'
index = Integer.parseInt(s, start, end - 1, 10);
if(index <= 0) {
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

* Unchecked exception thrown when the argument index is not within the valid
* range of supported argument index values. If an index value isn't
* representable by an {@code int} type, then the value
* {@code Integer.MIN_VALUE} will be used in the exception.
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.


/**
* Gets the value of the illegal index.
* Returns {@code Integer.MIN_VALUE} if the illegal index is not
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@@ -28,7 +28,9 @@
/**
* Unchecked exception thrown when the precision is a negative value other than
* {@code -1}, the conversion does not support a precision, or the value is
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@@ -27,7 +27,9 @@

/**
* Unchecked exception thrown when the format width is a negative value other
* than {@code -1} or is otherwise unsupported.
* than {@code -1} or is otherwise unsupported. If a given format width is not
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

}

/**
* Gets the value of the illegal index.
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@@ -2783,8 +2799,11 @@ private int index(String s, int start, int end) {
try {
// skip the trailing '$'
index = Integer.parseInt(s, start, end - 1, 10);
if(index <= 0) {
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

* is a <i>flag</i>. For example in the format string {@code "%-20s"} the
* <i>width</i> is <i>20</i> and the <i>flag</i> is "-".</p>
*
* <p> Values of <i>index</i> must be in the range one to
Copy link

@Marcono1234 Marcono1234 Oct 24, 2020

Choose a reason for hiding this comment

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

Hi @Marcono1234, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. 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 Marcono1234 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@openjdk
Copy link

openjdk bot commented Nov 13, 2020

⚠️ @igraves This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@igraves
Copy link
Member Author

igraves commented Nov 13, 2020

Updates (including cleaning up some git weirdness with rebasing) include adherence to the new CSR draft proposal. This makes the new exception type package-private.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 13, 2020
return;
}
}
throw new RuntimeException("Expected IllegalFormatException for zero argument index.");
Copy link
Contributor

Choose a reason for hiding this comment

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

The wording of errors around exceptions can be misinterpreted. Did an expected exception occur or not?
If all you saw was the exception without the line of code, would it be unambiguous?

@@ -2783,8 +2799,11 @@ private int index(String s, int start, int end) {
try {
// skip the trailing '$'
index = Integer.parseInt(s, start, end - 1, 10);
if(index <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space after 'if' please.

@@ -0,0 +1,98 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically, using the TestNG framework is preferable for new tests.
In addition to a convenient set of Asserts that check and print expected vs actual and message
it includes patterns for testing for expected exceptions.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, these tests are amenable to TestNG's assertThrows method. That might be all you need; we generally aren't too concerned about the exact content and formatting of the exception's message. However, if you want to make additional assertions over the thrown exception, it can be obtained using expectThrows instead of assertThrows.

throw new RuntimeException("Expected IllegalFormatException for non-representable precision.");
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a newline at end-of-file.

@Override
public String getMessage() {
return String.format("Illegal format argument index = %d", getIndex());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The exception with a very large negative number isn't going to easy to recognize.
Can the exception message say (for the Integer.MIN_VALUE) that the index is not valid index.
Its probably too much to ask have an indication where in the format string the offending index occurs.

int index = getIndex();

if (index == Integer.MIN_VALUE) {
return "Format argument index: (unrepresentable as int)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps "(not representable as int)" is more readable.

Copy link
Member Author

Choose a reason for hiding this comment

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

After reading it a few times, I agree.

@openjdk
Copy link

openjdk bot commented Nov 18, 2020

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

8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly

Reviewed-by: rriggs, smarks

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

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 (@RogerRiggs, @stuart-marks) 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 openjdk bot added the ready Pull request is ready to be integrated label Nov 18, 2020
IllegalFormatException e = expectThrows(IllegalFormatException.class, () -> {
String r = String.format("%2147483648$s", "A", "B");
});
//assertEquals(e.getMessage(), "Illegal format argument index = " + Integer.MIN_VALUE);
Copy link
Member

Choose a reason for hiding this comment

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

Extraneous comment?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it would seem!

@igraves
Copy link
Member Author

igraves commented Nov 19, 2020

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Nov 19, 2020
@openjdk
Copy link

openjdk bot commented Nov 19, 2020

@igraves
Your change (at version 78fc817) is now ready to be sponsored by a Committer.

@stuart-marks
Copy link
Member

/sponsor

@openjdk openjdk bot closed this Nov 19, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 19, 2020
@openjdk
Copy link

openjdk bot commented Nov 19, 2020

@stuart-marks @igraves Since your change was applied there have been 87 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 080c707.

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

openjdk-notifier bot referenced this pull request Nov 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org integrated Pull request has been integrated
4 participants