8359808: JavaRuntimeURLConnection should only connect to non-directory resources #25871

david-beaumont · 2025-06-18T12:53:56Z

Simplifying JavaRuntimeURLConnection to avoid accidentally returning non-resource data to users.

This change has the following distinct parts:

Refactor code to use Node instead of directly accessing low level ImageLocation type.
Remove unnecessary use of "Resource" interface and related URL generation code (completely unreachable).
Adding comments explaining why there's a non-obvious distinction in how module and resource names are treated with respect to URL percent encoding.
Small constructor logic simplification (module name cannot be null anymore)
Small simplification around 'READER' use, since it is impossible for that to ever be null (other users of ImageReaderFactory already assume it could never be null, and code path analysis agrees).
Adding tests for the non-resource cases.
Adding extra test data to check the behaviour with respect to things like percent escaping (previously untested).
Adding TODO comments for things I could do in this PR or later (reviewer opinions welcome).

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-8359808: JavaRuntimeURLConnection should only connect to non-directory resources (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25871

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-06-18T12:54:47Z

👋 Welcome back david-beaumont! 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 · 2025-06-18T12:55:30Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-06-18T12:56:05Z

@david-beaumont 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.

mlbridge · 2025-06-18T12:59:03Z

Webrevs

01: Full - Incremental (3bafc256)
00: Full (555bac6e)

david-beaumont

Preloading a few explanatory comments.

david-beaumont · 2025-06-18T12:54:49Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

@@ -45,99 +44,79 @@
 */
 public class JavaRuntimeURLConnection extends URLConnection {

-    // ImageReader to access resources in jimage
-    private static final ImageReader reader = ImageReaderFactory.getImageReader();


Examination of code and other users shows that this could never be null. Other users also name it "READER".

david-beaumont · 2025-06-18T12:55:49Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

    private final String name;

-    // the Resource when connected
-    private volatile Resource resource;


The Resource API was never needed here and adds uncalled methods such as getURL() which are then implemented with code that could never be invoked.

david-beaumont · 2025-06-18T12:56:38Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

            throw new MalformedURLException(url + " missing path or /");
-        if (path.length() == 1) {
-            this.module = null;


There's no reason for the module==null case, since the only thing caring that this might be null just converts null to the empty string anyway.

david-beaumont · 2025-06-18T12:58:29Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

+                throw new IOException(module + "/" + name + " not found");
+            }
+            this.resource = node;
+            super.connected = true;


I know I don't need to use super here, but it documents the fact that this is not a field of this subclass, without readers having to go check. Mutable protected fields are weird and error prone, so I felt calling it out a bit was worth it. Happy to replace with a comment if people feel that's better though.

david-beaumont · 2025-06-18T13:00:54Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

    }

    @Override
    public long getContentLengthLong() {
        try {
-            connect();
-            return resource.getContentLength();
+            return getResourceNode().size();


Having getResourceNode() return the (lazily fetched) node avoids the reader needing to know/reason about how "connect()" has the side-effect of making the "resource" field non-null.

AlanBateman · 2025-06-18T13:11:00Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

+    // URL's path part, and there's no requirement for there to be distinct rules
+    // about percent encoding. However, we choose to treat the module name is if
+    // it were a URL authority (since Java package/module names are historically
+    // strongly associated with internet domain names).


"However, we choose to treat the module name is if URL authority" - I don't think we should be put in the comment as it is confusing to suggest the URL authority component. The URL scheme documented in JEP 220 always put the module and resource name in the URL path component. It's just historical that it allowed for encoding of the resource name, wasn't given enough thought in JDK 9 when this url stream handler was added.

Happy to accept rewording here. I do want to pull out that there is a conceptual reason for treating module names like domain authorities though, or just make the code treat the whole path the same.
Having unexplained weirdness like this just ends up being a drain on future maintainers otherwise.

Happy to accept rewording here. I do want to pull out that there is a conceptual reason for treating module names like domain authorities though, or just make the code treat the whole path the same. Having unexplained weirdness like this just ends up being a drain on future maintainers otherwise.

I don't disagree on the weirdness but I don't want to mislead readers that this has anything to do with the URL authority component (the jrt scheme does not have this component). However to explain the weirdness requires digging into history, probably the jake repo where the changes for JEP 220 were accumulated before the JEP was integrated.

The alternative is to permit module names to use percent encoding, since package names (and thus module names) can include non-ASCII characters, and doing so would simplify the code, but it also allows a level of obfuscateability to the URLs which we might not desire (unless we reject any over-encoding of ASCII).

I updated the comment and some TODOs. I'll likely raise a separate PR for it since I suspect we need to end up:

allowing percent encoding everywhere (to support non-ASCII Unicode).

disallowing over-encoding (e.g. of '/' or '$', or just any ASCII) to prevent obfuscation.
But as it's a behaviour change, I'd rather separate it out.

AlanBateman

The changes looks mostly okay, just minor comments on comments, TODOs and naming.

AlanBateman · 2025-06-19T15:34:16Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

+    // URL's path part, and there's no requirement for there to be distinct rules
+    // about percent encoding, and it is likely that any differences between how
+    // module names and resource names are treated is unintentional. The rules
+    // about percent encoding may well be tightened up in the future.


I think it would be better to drop this paragraph from the comment, it just begs too many questions.

AlanBateman · 2025-06-19T15:36:13Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

-
-    // the module and resource name in the URL
+    // ImageReader to access resources in jimage (never null).
+    private static final ImageReader READER = ImageReaderFactory.getImageReader();


I think it simpler if you drop "(never null)".

AlanBateman · 2025-06-19T15:37:21Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

-        String path = url.getPath();
-        if (path.isEmpty() || path.charAt(0) != '/')
+        // TODO: Allow percent encoding in module names.
+        // TODO: Consider rejecting URLs with fragments, queries or authority.


I'd prefer not include all the TODO messages.

AlanBateman · 2025-06-19T15:39:09Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

+    // ImageReader to access resources in jimage (never null).
+    private static final ImageReader READER = ImageReaderFactory.getImageReader();
+
+    // The module and resource name in the URL ("jrt:/<module-name>/<resource-name>").


In JEP 220 we use jrt:/[$MODULE[/$PATH]] to make it clear that the resource is optionally.

AlanBateman · 2025-06-19T15:41:51Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

+        if (pathSep == -1) {
+            // No trailing resource path. This can never "connect" or return a
+            // resource, but might be useful as a representation to pass around.
+            // The module name *can* be empty here (e.g. "jrt:/") but not null.


If the URL scheme from JEP 220 goes into the first comment then it will be clear that the resource is optionally. This will allow the comment here to be reduced.

AlanBateman · 2025-06-19T15:44:48Z

src/java.base/share/classes/sun/net/www/protocol/jrt/JavaRuntimeURLConnection.java

-    // the Resource when connected
-    private volatile Resource resource;
+    // The resource node (when connected).
+    private volatile Node resource;


Maybe better to rename to node, resourceNode, or imageNode here.

david-beaumont added 2 commits June 18, 2025 00:15

Removing dead/unused code, simplifying connection logic and tidying up.

ce23206

Copyright update.

555bac6

openjdk bot added the rfr Pull request is ready for review label Jun 18, 2025

openjdk bot added the net [email protected] label Jun 18, 2025

david-beaumont commented Jun 18, 2025

View reviewed changes

david-beaumont changed the title ~~8356645: JavaRuntimeURLConnection should only connect to non-directory resources~~ 8359808: JavaRuntimeURLConnection should only connect to non-directory resources Jun 18, 2025

AlanBateman reviewed Jun 18, 2025

View reviewed changes

Reworking comment and adjusting TODOs.

3bafc25

AlanBateman reviewed Jun 19, 2025

View reviewed changes

8359808: JavaRuntimeURLConnection should only connect to non-directory resources #25871

Are you sure you want to change the base?

8359808: JavaRuntimeURLConnection should only connect to non-directory resources #25871

Uh oh!

Conversation

david-beaumont commented Jun 18, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Jun 18, 2025

Uh oh!

openjdk bot commented Jun 18, 2025

Uh oh!

openjdk bot commented Jun 18, 2025

Uh oh!

mlbridge bot commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

david-beaumont left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-beaumont Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-beaumont commented Jun 18, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 18, 2025 •

edited

Loading

david-beaumont Jun 18, 2025 •

edited

Loading

AlanBateman Jun 18, 2025 •

edited

Loading

david-beaumont Jun 18, 2025 •

edited

Loading