-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8359808: JavaRuntimeURLConnection should only connect to non-directory resources #25871
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
base: master
Are you sure you want to change the base?
Changes from all commits
ce23206
555bac6
3bafc25
930aa2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright (c) 2014, 2024, Oracle and/or its affiliates. All rights reserved. | ||
* Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
|
@@ -31,113 +31,85 @@ | |
import java.net.MalformedURLException; | ||
import java.net.URL; | ||
|
||
import jdk.internal.jimage.ImageLocation; | ||
import jdk.internal.jimage.ImageReader; | ||
import jdk.internal.jimage.ImageReader.Node; | ||
import jdk.internal.jimage.ImageReaderFactory; | ||
|
||
import jdk.internal.loader.Resource; | ||
import sun.net.www.ParseUtil; | ||
import sun.net.www.URLConnection; | ||
|
||
/** | ||
* URLConnection implementation that can be used to connect to resources | ||
* contained in the runtime image. | ||
* contained in the runtime image. See section "New URI scheme for naming stored | ||
* modules, classes, and resources" in <a href="https://openjdk.org/jeps/220"> | ||
* JEP 220</a>. | ||
*/ | ||
public class JavaRuntimeURLConnection extends URLConnection { | ||
|
||
// ImageReader to access resources in jimage | ||
private static final ImageReader reader = ImageReaderFactory.getImageReader(); | ||
// ImageReader to access resources in jimage. | ||
private static final ImageReader READER = ImageReaderFactory.getImageReader(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it simpler if you drop "(never null)". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just stressing that (unlike the old code) there's actually no need for a null check. |
||
|
||
// the module and resource name in the URL | ||
// The module and resource name in the URL (i.e. "jrt:/[$MODULE[/$PATH]]"). | ||
// | ||
// The module name is not percent-decoded, and can be empty. | ||
private final String module; | ||
private final String name; | ||
// The resource name permits UTF-8 percent encoding of non-ASCII characters. | ||
private final String path; | ||
|
||
// the Resource when connected | ||
private volatile Resource resource; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Resource API was never needed here and adds uncalled methods such as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect it was needed when the protocol handler was initially created. It should have probably been cleaned up before it was brought to main line. |
||
// The resource node (when connected). | ||
private volatile Node resourceNode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean to keep this as a volatile field? The only access right seems to be in the synchronized getResourceNode method but maybe it was accessed without the lock in a previous version? |
||
|
||
JavaRuntimeURLConnection(URL url) throws IOException { | ||
super(url); | ||
String path = url.getPath(); | ||
if (path.isEmpty() || path.charAt(0) != '/') | ||
String urlPath = url.getPath(); | ||
if (urlPath.isEmpty() || urlPath.charAt(0) != '/') { | ||
throw new MalformedURLException(url + " missing path or /"); | ||
if (path.length() == 1) { | ||
this.module = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
this.name = null; | ||
} | ||
int pathSep = urlPath.indexOf('/', 1); | ||
if (pathSep == -1) { | ||
// No trailing resource path. This can never "connect" or return a | ||
// resource (see JEP 220 for details). | ||
this.module = urlPath.substring(1); | ||
this.path = null; | ||
} else { | ||
int pos = path.indexOf('/', 1); | ||
if (pos == -1) { | ||
this.module = path.substring(1); | ||
this.name = null; | ||
} else { | ||
this.module = path.substring(1, pos); | ||
this.name = ParseUtil.decode(path.substring(pos+1)); | ||
} | ||
this.module = urlPath.substring(1, pathSep); | ||
this.path = percentDecode(urlPath.substring(pathSep + 1)); | ||
} | ||
} | ||
|
||
/** | ||
* Finds a resource in a module, returning {@code null} if the resource | ||
* is not found. | ||
* Finds and caches the resource node associated with this URL and marks the | ||
* connection as "connected". | ||
*/ | ||
private static Resource findResource(String module, String name) { | ||
if (reader != null) { | ||
URL url = toJrtURL(module, name); | ||
ImageLocation location = reader.findLocation(module, name); | ||
if (location != null) { | ||
return new Resource() { | ||
@Override | ||
public String getName() { | ||
return name; | ||
} | ||
@Override | ||
public URL getURL() { | ||
return url; | ||
} | ||
@Override | ||
public URL getCodeSourceURL() { | ||
return toJrtURL(module); | ||
} | ||
@Override | ||
public InputStream getInputStream() throws IOException { | ||
byte[] resource = reader.getResource(location); | ||
return new ByteArrayInputStream(resource); | ||
} | ||
@Override | ||
public int getContentLength() { | ||
long size = location.getUncompressedSize(); | ||
return (size > Integer.MAX_VALUE) ? -1 : (int) size; | ||
} | ||
}; | ||
private synchronized Node getResourceNode() throws IOException { | ||
if (resourceNode == null) { | ||
if (path == null) { | ||
throw new IOException("cannot connect to jrt:/" + module); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can make this |
||
} | ||
Node node = READER.findNode("/modules/" + module + "/" + path); | ||
if (node == null || !node.isResource()) { | ||
throw new IOException(module + "/" + path + " not found"); | ||
} | ||
this.resourceNode = node; | ||
super.connected = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly, many of the JDK 1.0 era APIs expose protected fields for subclasses, in this case URLConnection. One thing that is a bit icky is for a getXXX method to have the side effect to set this field. Maybe this is connectNode or something to suggest it connects the URLConnection? |
||
} | ||
return null; | ||
return resourceNode; | ||
} | ||
|
||
@Override | ||
public synchronized void connect() throws IOException { | ||
if (!connected) { | ||
if (name == null) { | ||
String s = (module == null) ? "" : module; | ||
throw new IOException("cannot connect to jrt:/" + s); | ||
} | ||
resource = findResource(module, name); | ||
if (resource == null) | ||
throw new IOException(module + "/" + name + " not found"); | ||
connected = true; | ||
} | ||
public void connect() throws IOException { | ||
getResourceNode(); | ||
} | ||
|
||
@Override | ||
public InputStream getInputStream() throws IOException { | ||
connect(); | ||
return resource.getInputStream(); | ||
return new ByteArrayInputStream(READER.getResource(getResourceNode())); | ||
} | ||
|
||
@Override | ||
public long getContentLengthLong() { | ||
try { | ||
connect(); | ||
return resource.getContentLength(); | ||
return getResourceNode().size(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like ExplodedImage.PathNode.size() throws UncheckedIOException. That might need mapping back to IOException to get the -1 return consistently. |
||
} catch (IOException ioe) { | ||
return -1L; | ||
} | ||
|
@@ -149,27 +121,16 @@ public int getContentLength() { | |
return len > Integer.MAX_VALUE ? -1 : (int)len; | ||
} | ||
|
||
/** | ||
* Returns a jrt URL for the given module and resource name. | ||
*/ | ||
@SuppressWarnings("deprecation") | ||
private static URL toJrtURL(String module, String name) { | ||
try { | ||
return new URL("jrt:/" + module + "/" + name); | ||
} catch (MalformedURLException e) { | ||
throw new InternalError(e); | ||
// Perform percent decoding of the resource name/path from the URL. | ||
private static String percentDecode(String path) throws MalformedURLException { | ||
if (path.indexOf('%') == -1) { | ||
// Nothing to decode (overwhelmingly common case). | ||
return path; | ||
Comment on lines
+126
to
+128
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this make a performance difference, checking for '%' is the first thing in ParseUtil.decode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The jrt protocol handler only exists because a URL streams a corresponding URLStreamHandler. It's not clear if the protocol handler is actually used. So I don't expect it is performance critical. |
||
} | ||
} | ||
|
||
/** | ||
* Returns a jrt URL for the given module. | ||
*/ | ||
@SuppressWarnings("deprecation") | ||
private static URL toJrtURL(String module) { | ||
try { | ||
return new URL("jrt:/" + module); | ||
} catch (MalformedURLException e) { | ||
throw new InternalError(e); | ||
return ParseUtil.decode(path); | ||
} catch (IllegalArgumentException e) { | ||
throw new MalformedURLException(e.getMessage()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old code treated this as fatal, throwing InternalError. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MalformedURLException seems better here. |
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright (c) 2014, Oracle and/or its affiliates. All rights reserved. | ||
* Copyright (c) 2014, 2025, Oracle and/or its affiliates. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
|
@@ -28,7 +28,6 @@ | |
*/ | ||
|
||
import java.io.IOException; | ||
import java.io.InputStream; | ||
import java.net.URL; | ||
import java.net.URLConnection; | ||
|
||
|
@@ -41,8 +40,28 @@ public class Basic { | |
@DataProvider(name = "urls") | ||
public Object[][] urls() { | ||
Object[][] data = { | ||
{ "jrt:/java.base/java/lang/Object.class", true }, | ||
{ "jrt:/java.desktop/java/lang/Object.class", false }, | ||
{"jrt:/java.base/java/lang/Object.class", true}, | ||
// Valid resource with and without percent-encoding. | ||
{"jrt:/java.base/java/lang/Runtime$Version.class", true}, | ||
{"jrt:/java.base/java%2Flang%2FRuntime%24Version.class", true}, | ||
// Unnecessary percent encoding (just Object again). | ||
{"jrt:/java.base/%6a%61%76%61%2f%6c%61%6e%67%2f%4f%62%6a%65%63%74%2e%63%6c%61%73%73", true}, | ||
// Query parameters and fragments are silently ignored. | ||
{"jrt:/java.base/java/lang/Object.class?yes=no", true}, | ||
{"jrt:/java.base/java/lang/Object.class#anchor", true}, | ||
|
||
// Missing resource (no such class). | ||
{"jrt:/java.base/java/lang/NoSuchClass.class", false}, | ||
// Missing resource (wrong module). | ||
{"jrt:/java.desktop/java/lang/Object.class", false}, | ||
// Entries in jimage which don't reference resources. | ||
{"jrt:/modules/java.base/java/lang", false}, | ||
{"jrt:/packages/java.lang", false}, | ||
// Invalid (incomplete/corrupt) URIs. | ||
{"jrt:/java.base", false}, | ||
{"jrt:/java.base/", false}, | ||
// Cannot escape anything in the module name. | ||
{"jrt:/java%2Ebase/java/lang/Object.class", false}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add "jrt:/" to the list? |
||
}; | ||
return data; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examination of code and other users shows that this could never be null. Other users also name it "READER".