RFR: 8359808: JavaRuntimeURLConnection should only connect to non-directory resources [v2]

[Alan Bateman]

On Wed, 18 Jun 2025 16:48:45 GMT, David Beaumont <duke at openjdk.org> wrote:

>> Simplifying JavaRuntimeURLConnection to avoid accidentally returning non-resource data to users.
>> 
>> This change has the following distinct parts:
>> 1. Refactor code to use Node instead of directly accessing low level ImageLocation type.
>> 2. Remove unnecessary use of "Resource" interface and related URL generation code (completely unreachable).
>> 3. Adding comments explaining why there's a non-obvious distinction in how module and resource names are treated with respect to URL percent encoding.
>> 4. Small constructor logic simplification (module name cannot be null anymore)
>> 5. 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).
>> 6. Adding tests for the non-resource cases.
>> 7. Adding extra test data to check the behaviour with respect to things like percent escaping (previously untested).
>> 8. Adding TODO comments for things I could do in this PR or later (reviewer opinions welcome).
>
> David Beaumont has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Reworking comment and adjusting TODOs.

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

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

> 46: 
> 47:     // ImageReader to access resources in jimage (never null).
> 48:     private static final ImageReader READER = ImageReaderFactory.getImageReader();

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

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

> 48:     private static final ImageReader READER = ImageReaderFactory.getImageReader();
> 49: 
> 50:     // 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.

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

> 54:     // about percent encoding, and it is likely that any differences between how
> 55:     // module names and resource names are treated is unintentional. The rules
> 56:     // 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.

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

> 62: 
> 63:     // The resource node (when connected).
> 64:     private volatile Node resource;

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

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

> 67:         super(url);
> 68:         // TODO: Allow percent encoding in module names.
> 69:         // TODO: Consider rejecting URLs with fragments, queries or authority.

I'd prefer not include all the TODO messages.

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

> 76:             // No trailing resource path. This can never "connect" or return a
> 77:             // resource, but might be useful as a representation to pass around.
> 78:             // 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.

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

PR Review: https://git.openjdk.org/jdk/pull/25871#pullrequestreview-2943467201
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157287702
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157291827
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157284956
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157300971
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157289343
PR Review Comment: https://git.openjdk.org/jdk/pull/25871#discussion_r2157296409