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

JDK-8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException #2662

Closed
wants to merge 1 commit into from

Conversation

@candrews
Copy link
Contributor

@candrews candrews commented Feb 20, 2021

java.net.URLClassLoader.getResource can throw an undocumented IllegalArgumentException.

According to the javadoc for the getResource and findResource methods, neither should be throwing IllegalArgumentException - they should return null if the resource can't be resolved.

Quoting the javadoc for URLClassLoader.html#findResource

Returns:
a URL for the resource, or null if the resource could not be found, or if the loader is closed.

And quoting the javadoc for ClassLoader.html#getResource

Returns:
URL object for reading the resource; null if the resource could not be found, a URL could not be constructed to locate the resource, the resource is in a package that is not opened unconditionally, or access to the resource is denied by the security manager.

Neither mentions throwing IllegalArgumentException and both are clear that when URL can't be constructed, null should be returned.

Here's a stack trace:

java.lang.IllegalArgumentException: name
        at java.base/jdk.internal.loader.URLClassPath$Loader.findResource(URLClassPath.java:600)
        at java.base/jdk.internal.loader.URLClassPath.findResource(URLClassPath.java:291)
        at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:655)
        at java.base/java.net.URLClassLoader$2.run(URLClassLoader.java:653)
        at java.base/java.security.AccessController.doPrivileged(Native Method)
        at java.base/java.net.URLClassLoader.findResource(URLClassLoader.java:652)

Looking at URLClassPath.findResource

        URL findResource(final String name, boolean check) {
            URL url;
            try {
                url = new URL(base, ParseUtil.encodePath(name, false));
            } catch (MalformedURLException e) {
                throw new IllegalArgumentException("name");
            }

Instead of throwing IllegalArgumentException, that line should simply return null.

A similar issue exists at URLClassPath.getResource

        URL findResource(final String name, boolean check) {
            URL url;
            try {
                url = new URL(base, ParseUtil.encodePath(name, false));
            } catch (MalformedURLException e) {
                throw new IllegalArgumentException("name");
            }

Instead of throwing IllegalArgumentException, that line should simply return null.


Progress

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

Issue

  • JDK-8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException

Reviewers

Download

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

@bridgekeeper bridgekeeper bot added the oca label Feb 20, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 20, 2021

Hi @candrews, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

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 candrews" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@candrews
Copy link
Contributor Author

@candrews candrews commented Feb 20, 2021

Submitted a bug report at https://bugreport.java.com/
Received internal review ID : 9069151

@openjdk
Copy link

@openjdk openjdk bot commented Feb 20, 2021

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

  • core-libs

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.

@candrews
Copy link
Contributor Author

@candrews candrews commented Feb 22, 2021

This one liner will reproduce this issue (you can easily use jshell to run it and see the problem):

new java.net.URLClassLoader(new java.net.URL[]{new java.net.URL("https://repo1.maven.org/anything-that-ends-with-slash/")}).findResource("c:/windows");

The key parts to reproduce the issue are:

  1. The URLClassLoader URL must not start with file: or jar: and must end in /. See
  2. The string passed to findResource must not be a valid URL component. Windows paths (which include a : making them invalid URL components) are a common and easy way to see this error.

@candrews candrews changed the title URLClassPath: do not throw exception 8262277: URLClassPath: do not throw exception Feb 24, 2021
@candrews candrews changed the title 8262277: URLClassPath: do not throw exception 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException Feb 24, 2021
@candrews
Copy link
Contributor Author

@candrews candrews commented Feb 26, 2021

/signed

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 26, 2021

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 3, 2021

Webrevs

Copy link
Contributor

@AlanBateman AlanBateman left a comment

This seems to be a long standing bug, maybe goes all the way back to JDK 1.2. Are you planning to add a regression test?

@bchristi-git
Copy link
Member

@bchristi-git bchristi-git commented Mar 3, 2021

Hi, Craig
The commented-out lines should be removed from the change.

As Alan said, a regression test will be needed. At minimum, it should test a method that returns a URL, as well as a method that returns an Enumeration (which can also lead to an IAE, as I noted in the bug report).

Also, though the compatibility risk is low, it would be good to include a CSR for this change to long-standing behavior.

@bchristi-git
Copy link
Member

@bchristi-git bchristi-git commented Mar 3, 2021

/csr needed

@openjdk openjdk bot added the csr label Mar 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

@bchristi-git has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@candrews please create a CSR request and add link to it in JDK-8262277. This pull request cannot be integrated until the CSR request is approved.

@bchristi-git
Copy link
Member

@bchristi-git bchristi-git commented Mar 3, 2021

Also, the copyright year should be updated: 2019 -> 2021

@candrews candrews changed the title 8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException Mar 4, 2021
@candrews
Copy link
Contributor Author

@candrews candrews commented Mar 4, 2021

The commented-out lines should be removed from the change.

Done! 👍

As Alan said, a regression test will be needed. At minimum, it should test a method that returns a URL, as well as a method that returns an Enumeration (which can also lead to an IAE, as I noted in the bug report).

I've added a test.

Also, the copyright year should be updated: 2019 -> 2021

Done! 👍

@bchristi-git has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@candrews please create a CSR request and add link to it in JDK-8262277. This pull request cannot be integrated until the CSR request is approved.

I'm trying to figure out how to create a CSR and I'm not having much luck. According to the CSR FAQs:

Q: How do I create a CSR ?
A: Do not directly create a CSR from the Create Menu. JIRA will let you do this right up until the moment you try to save it and find your typing was in vain.
Instead you should go to the target bug, select "More", and from the drop down menu select "Create CSR". This is required to properly associate the CSR with the main bug, just as is done for backports.

However, I don't have an account on https://bugs.openjdk.java.net so I can't do as instructed.

How do I create the CSR?

@candrews candrews force-pushed the patch-1 branch 2 times, most recently from 3f6a974 to e456f88 Mar 4, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 4, 2021

Mailing list message from Brent Christian on core-libs-dev:

On 3/4/21 1:16 PM, Craig Andrews wrote:

@bchristi-git has indicated that a [compatibility and specification](https://wiki.openjdk.java.net/display/csr/Main) (CSR) request is needed for this pull request.
@candrews please create a CSR request and add link to it in [JDK-8262277](https://bugs.openjdk.java.net/browse/JDK-8262277). This pull request cannot be integrated until the CSR request is approved.

I'm trying to figure out how to create a CSR and I'm not having much luck. According to the [CSR FAQs](https://wiki.openjdk.java.net/display/csr/CSR+FAQs):

Q: How do I create a CSR ?
A: Do not directly create a CSR from the Create Menu. JIRA will let you do this right up until the moment you try to save it and find your typing was in vain.
Instead you should go to the target bug, select "More", and from the drop down menu select "Create CSR". This is required to properly associate the CSR with the main bug, just as is done for backports.

However, I don't have an account on https://bugs.openjdk.java.net so I can't do as instructed.

How do I create the CSR?

OK, yeah - I can create the CSR. If you can craft some content for the
Summary, Problem, Solution, and maybe Compatibility Risk Description
(the risk itself is Low), I can take it from there.

A recent CSR of a somewhat similar
fix-the-implementation-to-match-the-spec flavor might be:
https://bugs.openjdk.java.net/browse/JDK-8255997
(though the change there is to *start* throwing an exception :D ).
Anyway, perhaps that might helpful as a reference.

-Brent

* @bug 8262277
* @summary Test to see if URLClassLoader.getResource and URLClassLoader.getResources
* throw IllegalArgumentException
*/
Copy link
Contributor

@AlanBateman AlanBateman Mar 5, 2021

I'd prefer if the test checked that findResource returned null and that findResources returned an empty enumeration. I think we should be able to find a better name for the test too.
Do you really want the author tag in the test? I know they exist in some tests but they are impossible to remove, even when tests/code are significantly changed by someone else.

Copy link
Contributor Author

@candrews candrews Mar 5, 2021

I'd prefer if the test checked that findResource returned null and that findResources returned an empty enumeration.

I'll update the test accordingly.

think we should be able to find a better name for the test too.
Do you really want the author tag in the test? I know they exist in some tests but they are impossible to remove, even when tests/code are significantly changed by someone else.

I can rename the test class to be something descriptive and remove the @author tag. I was following other tests which is why I did it this way.

Copy link
Contributor Author

@candrews candrews Mar 5, 2021

I've made the changes you requested.

@candrews
Copy link
Contributor Author

@candrews candrews commented Mar 5, 2021

Summary

Modify implementation of java.net.URLClassLoader such that findResource and java.net.URLClassLoader.html#findResources do no longer throw undocumented IllegalArgumentException.

Problem

The javadocs for URLClassLoader.html#findResource and java.net.URLClassLoader.html#findResources do not indicate that an IllegalArgumentException can be thrown. The implementation does throw such exceptions under specific conditions which, due to being undocumented, is unexpected behavior. The unexpected exception can cause incorrect behavior, particularly on Windows systems due to the Windows file system path format increasingly the likelihood of causing the unexpected exception. An example of such a problem was discovered in Spring Framework necessitating a workaround..

Solution

Modify the implementation of java.net.URLClassLoader to not throw IllegalArgumentException when finding resources.

Compatibility Risk

Low

Compatibility Risk Description

It is possible that some code is catching the undocumented IllegalArgumentException and treating it in a specific way.

Copy link
Contributor

@AlanBateman AlanBateman left a comment

The fix looks good. Brent has created a CSR. It seems unlikely that anyone is depending the long standing/broken behavior but it seems release note worthy.

Copy link
Member

@bchristi-git bchristi-git left a comment

Looks good, thanks!

@openjdk openjdk bot removed the csr label Mar 9, 2021
@candrews
Copy link
Contributor Author

@candrews candrews commented Mar 15, 2021

What's the next step in the process for this PR?

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 15, 2021

You need to fix this error:

Title mismatch between PR and JBS for issue JDK-8262277

by changing the title of this PR to match the JBS title. Then you should be able to integrate it.

@candrews candrews changed the title JDK-8262277: java.net.URLClassLoader.getResource throws undocumented IllegalArgumentException JDK-8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException Mar 15, 2021
@candrews
Copy link
Contributor Author

@candrews candrews commented Mar 15, 2021

You need to fix this error:

Title mismatch between PR and JBS for issue JDK-8262277

by changing the title of this PR to match the JBS title. Then you should be able to integrate it.

Done - how's it look now?

@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

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

8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException

Reviewed-by: alanb, bchristi, psadhukhan

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

  • d6b5e18: 8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers
  • 80cdf78: 8263544: Unused argument in ConstantPoolCacheEntry::set_field()
  • c0176c4: 8263552: Use String.valueOf() for char-to-String conversions
  • fac39fe: 8263508: Remove dead code in MethodHandleImpl
  • 7b4aefe: 8263530: sun.awt.X11.ListHelper.removeAll() should use clear()
  • 32c7fcc: 8263490: [macos] Crash occurs on JPasswordField with activated InputMethod
  • 8afec70: 8260931: Implement JEP 382: New macOS Rendering Pipeline
  • 0638303: 8263497: Clean up sun.security.krb5.PrincipalName::toByteArray
  • ba22e6f: 8263446: Avoid unary minus over unsigned type in ObjectSynchronizer::dec_in_use_list_ceiling
  • b371f90: 8263504: Some OutputMachOpcodes fields are uninitialized
  • ... and 276 more: https://git.openjdk.java.net/jdk/compare/2b00367e1154feb2c05b84a11d62fb5750e46acf...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 (@AlanBateman, @bchristi-git, @prsadhuk) 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 label Mar 15, 2021
@candrews
Copy link
Contributor Author

@candrews candrews commented Mar 15, 2021

/contributor add @candrews

@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@candrews Could not parse @candrews as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Mar 15, 2021

You don't need to add yourself as a contributor. The only thing you need to do is integrate. Then it is ready to be sponsored.

@candrews
Copy link
Contributor Author

@candrews candrews commented Mar 15, 2021

/integrate

@openjdk openjdk bot added the sponsor label Mar 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@candrews
Your change (at version 944956c) is now ready to be sponsored by a Committer.

@bchristi-git
Copy link
Member

@bchristi-git bchristi-git commented Mar 15, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Mar 15, 2021

@bchristi-git @candrews Since your change was applied there have been 292 commits pushed to the master branch:

  • 4f1cda4: 8263387: G1GarbageCollection JFR event gets gc phase, not gc type
  • 5ab5244: 8263514: Minor issue in JavacFileManager.SortFiles.REVERSE
  • 771b146: 8245025: MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 46d78f0: 6539707: (fc) MappedByteBuffer.force() method throws an IOException in a very simple test
  • 189289d: 8262326: MaxMetaspaceSize does not have to be aligned to metaspace commit alignment
  • d825198: 8263556: remove @modules java.base from tests
  • d6b5e18: 8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers
  • 80cdf78: 8263544: Unused argument in ConstantPoolCacheEntry::set_field()
  • c0176c4: 8263552: Use String.valueOf() for char-to-String conversions
  • fac39fe: 8263508: Remove dead code in MethodHandleImpl
  • ... and 282 more: https://git.openjdk.java.net/jdk/compare/2b00367e1154feb2c05b84a11d62fb5750e46acf...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0c718ab.

💡 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
5 participants