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

8282395: URL.openConnection can throw IOOBE #8155

Closed
wants to merge 6 commits into from

Conversation

tkiriyama
Copy link
Contributor

@tkiriyama tkiriyama commented Apr 8, 2022

I fixed sun.net.www.ParseUtil.decode().

ParseUtil.decode() always tries to decode after parsing '%', so if '%' is located at the end of the String, IndexOutOfBoundsException is thrown. Also, if '%' is shown after decodable string and following string is not decodable (e.g: "%25%s%G1"), ParseUtil.decode() throws IllegalArgumentException.

But URL standard says below (https://url.spec.whatwg.org/#percent-decode).

Otherwise, if byte is 0x25 (%) and the next two bytes after byte in input are not in the ranges 
0x30 (0) to 0x39 (9), 0x41 (A) to 0x46 (F), and 0x61 (a) to 0x66 (f), all inclusive, append byte to output.

So, there should be used isEscaped() to judge to decode.

Would you please review this fix?


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

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8155

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 8, 2022

👋 Welcome back tkiriyama! 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 Apr 8, 2022
@openjdk
Copy link

openjdk bot commented Apr 8, 2022

@tkiriyama The following label will be automatically applied to this pull request:

  • net

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

@openjdk openjdk bot added the net net-dev@openjdk.org label Apr 8, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 8, 2022

Webrevs

@AlanBateman
Copy link
Contributor

Are there examples using URL/URLconnection (rather than ParseUtil directly) to demonstrate the issue?

@dfuch
Copy link
Member

dfuch commented Apr 8, 2022

MalformedURLException should probably be thrown instead of simply accepting a malformed % encoded pair.

@tkiriyama
Copy link
Contributor Author

Are there examples using URL/URLconnection (rather than ParseUtil directly) to demonstrate the issue?

I’m sorry for the late reply. Thre is the example in JDK-8282395.
This is a simple example code.

import java.net.URL;

public class TestURL {
  public static void main(String args[]) throws Exception {
    URL url = new URL("ftp://.:%@");
    url.openConnection();
  }
}

The following exception is thrown.

>java TestURL
Exception in thread "main" java.lang.IndexOutOfBoundsException: Range [1, 3) out of bounds for length 1
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:100)
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckFromToIndex(Preconditions.java:112)
        at java.base/jdk.internal.util.Preconditions.checkFromToIndex(Preconditions.java:349)
        at java.base/java.util.Objects.checkFromToIndex(Objects.java:411)
        at java.base/java.lang.Integer.parseInt(Integer.java:708)
        at java.base/sun.net.www.ParseUtil.unescape(ParseUtil.java:166)
        at java.base/sun.net.www.ParseUtil.decode(ParseUtil.java:202)
        at java.base/sun.net.www.protocol.ftp.FtpURLConnection.<init>(FtpURLConnection.java:203)
        at java.base/sun.net.www.protocol.ftp.Handler.openConnection(Handler.java:61)
        at java.base/sun.net.www.protocol.ftp.Handler.openConnection(Handler.java:56)
        at java.base/java.net.URL.openConnection(URL.java:1094)
        at TestURL.main(TestURL.java:6)

@tkiriyama
Copy link
Contributor Author

MalformedURLException should probably be thrown instead of simply accepting a malformed % encoded pair.

Thank you very much for your comment.
I think we should accept any codes like "%25%s%G1" according to URL standard.

Is there a reason why an exception should be thrown?

@dfuch
Copy link
Member

dfuch commented Apr 14, 2022

I think we should accept any codes like "%25%s%G1" according to URL standard.

Is there a reason why an exception should be thrown?

Yes. It is not a valid escape sequence according to the spec.

jshell> URI.create("ftp://host:5678/%25%s%G1");
|  Exception java.lang.IllegalArgumentException: Malformed escape pair at index 19: ftp://host:5678/%25%s%G1
|        at URI.create (URI.java:906)
|        at (#1:1)
|  Caused by: java.net.URISyntaxException: Malformed escape pair at index 19: ftp://host:5678/%25%s%G1
|        at URI$Parser.fail (URI.java:2973)
|        at URI$Parser.scanEscape (URI.java:3101)
|        at URI$Parser.scan (URI.java:3124)
|        at URI$Parser.checkChars (URI.java:3142)
|        at URI$Parser.parseHierarchical (URI.java:3226)
|        at URI$Parser.parse (URI.java:3174)
|        at URI.<init> (URI.java:623)
|        at URI.create (URI.java:904)
|        ...

@tkiriyama
Copy link
Contributor Author

It is not a valid escape sequence according to the spec.

I see. JavaAPI document says that Escaped octets, that is, triplets consisting the percent character ('%') followed by two hexadecimal digits. Also, RFC 2396 says that '%' must be escaped as "%25" in order to be used as data within a URI.
The exception thrown by URI.create() may be appropriate. I consider whether to fix the exception thrown by URL.openConnection().

@bridgekeeper
Copy link

bridgekeeper bot commented May 17, 2022

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

@tkiriyama
Copy link
Contributor Author

I am very sorry for taking long time to fix.
I fixed java.net.URL#openConnection() to throw MalformedURLException when ParseUtil#decode() throws IllegalArgumentException because Javadoc says that openConnection() throws IOException if if an I/O exception occurs.
Also I fixed ParseUtil#decode() not to throw IndexOutOfBoundsException when a malformed escape pair is passed.

@tkiriyama
Copy link
Contributor Author

@dfuch
Sorry to bother you, but I want you to review this fix again.

Comment on lines +203 to +204
} catch (NumberFormatException | IndexOutOfBoundsException e) {
throw new IllegalArgumentException("Malformed escape pair: " + s);
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest turning the assert at line 200 into a proper bound check and throw IllegalArgumentException if n - i >= 2

Copy link
Member

Choose a reason for hiding this comment

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

I meant if n - i is not >= 2 of course.

Comment on lines 1095 to 1101
URLConnection url = null;
try {
url = handler.openConnection(this);
} catch (IllegalArgumentException e) {
throw new MalformedURLException(e.getMessage());
}
return url;
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the changes in java.net.URL. It's not the proper place to do the conversion. If a conversion is needed, it should take place in the handler.
One possible experiment could be to change ParseUtil::decode to declare throws MalformedURLException and see what no longer compiles. If compilation passes - then maybe that's the way to go. If it doesn't - then we could analyze what to do on a case-by-case basis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some methods call ParseUtil::decode and of these 10 are not declared to be thrown MalformedURLException.
Modifying ParseUtil :: decode has not a little impact. Is modifying java.net.URL not proper?

The following classes have the methods which are not declared to be thrown MalformedURLException.

java.net.URLClassLoader
sun.net.www.protocol.ftp.FtpURLConnection
sun.security.provider.PolicyFile
jdk.internal.loader.FileURLMapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfuch
Could you tell me your opinion based on this information, please?

Copy link
Member

@dfuch dfuch Jul 6, 2022

Choose a reason for hiding this comment

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

Let me try to summarize: there are several things that we could do:

  1. nothing: continue throwing IIOBE
  2. throw IAE instead of IIOBE, and let IAE trickle up the calling stack to the callers code
  3. throw MUE instead of IIOBE, and then update all the places where ParseUtils::decode is called to either let the MUE trickle up the stack if possible, or deal with it if not.
  4. throw IAE instead of IIOBE and somewhere up the stack catch that and transform it into MUE.
    4.1. do the catch & transform in URL::openConnection when calling URLStreamHandler::openConnection
    4.2. do the catch & transform in every protocol handler implementation that use ParseUtils::decode

Now let's see pro & cons:

  1. is not ideal because it looks like a bug (and it is)
  2. is expedient. There seems to be some precedent for that since ParseUtils::decode already throws IAE if it can't parse the port. However IAE is not specified to be thrown by openConnection. So maybe we should update the specification of URL::openConnection and URLStreamHandler::openConnection to allow for IAE. This is however a bit strange to use IAE because in this case the URL is indeed malformed, so MUE would be more appropriate.
  3. has the advantage of being clear, and to force all callers of ParseUtils to deal with the situation. However, ParseUtils seems to be widely used, so each places where it's called needs to be examined, to see what would be the more appropriate solution in each case
  4. The advantage here is that callers of ParseUtils don't need to be changed if IAE is already appropriate for them. But those for which MUE would be more appropriate would benefit from having IAE transformed in MUE, which can be achieved by 4.1 or 4.2
    4.1 the issue here is that URLStreamHandler::openConnection is not specified to throw IAE, yet we catch IAE that might be thrown by custom URLStreamHandler implementations not present in the JDK, where the IAE that they throw might have nothing to do with a Malformed URL. So we risk introducing regressions for applications that provide their own custom URLStreamHandlers implementations, and that might not expect the wrapping of the IAE they throw into MUE. I believe that if we did that it would need to be specified (in URL::openConnection) and it would require a CSR.
    4.2 we need to examine all protocol handler implementations present in the JDK and do the wrapping there. URLStreamHandler::openConnection is specified to throw IOException, so throwing MUE is within the bounds. However - if we don't have ParseUtils::decode throw MUE, we're not guaranteed not to miss some places.

I'd say from a consistency point of view, 3. is probably best. Then failing that, from a compatibility vs consistency point of view 4.2 might be our best shot. 2. might be envisaged, but maybe at the expense of logging a follow up issue to clean that up and later decide between 3. or 4.2.
I would tend to rule out 4.1 due to the possible compatibility issue, and also because if URLStreamHandler::openConnection wanted to throw MUE it could, so there is no reason for doing 4.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your valuable comments. I think 3 is a good idea. I will try fix, but it takes some time to see the impact on many other methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfuch

I'm sorry, it took time for me to think about how to fix.
First, I thought of option 3, but it seems difficult.
In many of the methods in error, the javadoc defines the exceptions that can be thrown.
When option 3, we will wrap exceptions in many methods because it is difficult to fix specifications of all methods and CSR.
However, if we are going to wrap exceptions, I think it's better and less fix to do it in a protocol handler, it means option 4.2.

Let me hear what you think.

Copy link
Member

Choose a reason for hiding this comment

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

4.2 might be fine especially if we can do it at the point were ParseUtils is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfuch
Thank you very much. Your advice was very helpful in fix.
I fixed sun.net.www.protocol.ftp.Hanler.openConnection to throw MalformedURLException.
Hanler classes in other protcol should not be fixed because they don't use ParseUtil.decode method.

Comment on lines +203 to +204
} catch (NumberFormatException | IndexOutOfBoundsException e) {
throw new IllegalArgumentException("Malformed escape pair: " + s);
Copy link
Member

Choose a reason for hiding this comment

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

I meant if n - i is not >= 2 of course.

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 16, 2022

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

try {
connection = new FtpURLConnection(u, p);
} catch (IllegalArgumentException e) {
throw new MalformedURLException(e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest keeping the IllegalArgumentException as the root cause of the MUE for better diagnosability. Otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The constructor "MalformedURLException (Throwable cause)" doens'nt exist. Isn't there any other method?

Copy link
Member

Choose a reason for hiding this comment

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

You can use Throwable::initCause

Copy link
Member

Choose a reason for hiding this comment

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

var mfue = new MalformedURLException(e.getMessage());
mfue.initCause(e);
throw mfue;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your advice, I could fix it.

@dfuch
Copy link
Member

dfuch commented Sep 30, 2022

I'm going to run some tests in our CI and will come back here with the result.

@@ -197,11 +197,14 @@ public static String decode(String s) {
bb.clear();
int ui = i;
for (;;) {
assert (n - i >= 2);
if (n - i >= 2) {
Copy link
Member

Choose a reason for hiding this comment

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

OK - the test should be inverted - sorry for not noticing that earlier. It should be n - i < 2. It's causing massive failures in the CI. Can you please run at least the tests under jdk/sun/net and jdk/java/net after making the change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry for my bad mistake.
I fixed it and run the tests under jdk/sun/net and jdk/java/net. I confirmed all tests passed in Linux.

@dfuch
Copy link
Member

dfuch commented Oct 5, 2022

Thanks! I will rerun the tests and if everything comes back green I'll approve.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

Tests were green: approved.

@openjdk
Copy link

openjdk bot commented Oct 5, 2022

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

8282395: URL.openConnection can throw IOOBE

Reviewed-by: dfuchs

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

  • e775acf: 8293986: Incorrect double-checked locking in com.sun.beans.introspect.ClassInfo
  • 9d116ec: 8294262: AArch64: compiler/vectorapi/TestReverseByteTransforms.java test failed on SVE machine
  • 4b17d28: 8294261: AArch64: Use pReg instead of pRegGov when possible
  • 891156a: 8295003: Do not mention applets in the "java.awt.color" package
  • e6c33e6: 8295014: Remove unnecessary explicit casts to void* in CHeapObjBase
  • 1bfcc27: 8294931: JFR: Simplify SettingInfo
  • eb90c4f: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited
  • 4df4a1f: 8287832: jdk/jfr/event/runtime/TestActiveSettingEvent.java failed with "Expected two batches of Active Setting events"
  • 35d17a0: 8293864: Kitchensink24HStress.java fails with SIGSEGV in JfrCheckpointManager::lease
  • c5f462e: 8294956: GHA: qemu-debootstrap is deprecated, use the regular one
  • ... and 1601 more: https://git.openjdk.org/jdk/compare/a82417fa190a132313f6734a75f1998858c164fd...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 (@dfuch) 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 Oct 5, 2022
@tkiriyama
Copy link
Contributor Author

@dfuch
Thank you very much for your review and some helpful comments.

I think two reviews are required to be integrated. Could anyone please review this pull request?

@dfuch
Copy link
Member

dfuch commented Oct 7, 2022

For core libraries one reviewer is usually enough. The two reviewers rule applies mostly to hotspot components I believe. You may wait for a second reviewer if you wish to, though, or if the first reviewer advises you to do so. Also it's good practice to wait a bit before integrating to see if anyone else will chime him. Due to the various time zones it's usually recommended to wait at least 24 hours after the RFR notification has gone out on the mailing list, even if you have a reviewer after 5mins. Unless you're fixing a build break of course - in which case reviewers may advise you to integrate ASAP ;-)

@dfuch
Copy link
Member

dfuch commented Oct 10, 2022

PS: if you integrate this PR I will sponsor it.

@tkiriyama
Copy link
Contributor Author

@dfuch
Thank you, Your advice has been really helpful.
I had already gotten your valuable review, so I would be glad if you could sponsor this PR.

@tkiriyama
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 11, 2022
@openjdk
Copy link

openjdk bot commented Oct 11, 2022

@tkiriyama
Your change (at version 2064a47) is now ready to be sponsored by a Committer.

@dfuch
Copy link
Member

dfuch commented Oct 11, 2022

/sponsor

@dfuch
Copy link
Member

dfuch commented Oct 11, 2022

Done! Thanks for your perseverance!

@openjdk
Copy link

openjdk bot commented Oct 11, 2022

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

  • fe70487: 8294958: java/net/httpclient/ConnectTimeout tests are slow
  • 97f1321: 8294356: IGV: scheduled graphs contain duplicated elements
  • 5e05e42: 8294901: remove pre-VS2017 checks in Windows related coding
  • e775acf: 8293986: Incorrect double-checked locking in com.sun.beans.introspect.ClassInfo
  • 9d116ec: 8294262: AArch64: compiler/vectorapi/TestReverseByteTransforms.java test failed on SVE machine
  • 4b17d28: 8294261: AArch64: Use pReg instead of pRegGov when possible
  • 891156a: 8295003: Do not mention applets in the "java.awt.color" package
  • e6c33e6: 8295014: Remove unnecessary explicit casts to void* in CHeapObjBase
  • 1bfcc27: 8294931: JFR: Simplify SettingInfo
  • eb90c4f: 8294377: Prepare to stop auto-inheriting documentation for subclasses of exceptions whose documentation is inherited
  • ... and 1604 more: https://git.openjdk.org/jdk/compare/a82417fa190a132313f6734a75f1998858c164fd...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 11, 2022
@openjdk openjdk bot closed this Oct 11, 2022
@openjdk 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 Oct 11, 2022
@openjdk
Copy link

openjdk bot commented Oct 11, 2022

@dfuch @tkiriyama Pushed as commit 4435d56.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
3 participants