On Wed, 8 Oct 2025 10:26:24 GMT, Jaikiran Pai <[email protected]> wrote:
>> Can I please get a review of this change which proposes to fix a file
>> descriptor leak in `sun.net.www.protocol.file.FileURLConnection`?
>>
>> The bug affects all JDK versions since Java 8 through mainline JDK.
>> Furthermore, the issue isn't in any way related to "jar:" URLs or the JAR
>> resources caching involved in the `JarURLConnection`.
>>
>> `sun.net.www.protocol.file.FileURLConnection` is a `java.net.URLConnection`.
>> For this issue, the APIs on `URLConnection` that are of interest are
>> `connect()`, header related APIs (the getHeaderXXXX(), getLastModified() and
>> similar APIs) and `getInputStream()`.
>>
>> The existing implementation in `FileURLConnection` in its `connect()`
>> implementation does readability checks on the underlying `File` instance. If
>> the `File` is a directory then the readability check is done by verifying
>> that a `File.list()` call does not return null. On the other hand, if the
>> File is not a directory, then connect() constructs a temporary
>> `java.io.FileInputStream` for the File and lets the FileInputStream's
>> constructor implementation do the necessary readability checks. In either
>> case, if the readability checks fail, then an IOException is thrown from
>> this method and the FileURLConnection stays unconnected.
>>
>> One important detail of the implementation in `FileURLConnection.connect()`
>> is that the `FileInputStream` that it creates for the regular-file
>> readability check, it keeps it open when it returns from `connect()`.
>> `connect()` itself doesn't return any value, so this `FileInputStream` that
>> was created for readability check remains open without the application
>> having access to it, unless the application calls
>> `(File)URLConnection.getInputStream()`. The implementation of
>> `FileURLConnection.getInputStream()` returns this previously constructed
>> `InputStream` if `connect()` had previously suceeded with its readability
>> checks. The application, as usual, is then expected to `close()` that
>> `InputStream` that was returned from `URLConnection.getInputStream()`. This
>> is the "normal" case, where the application at some point calls the
>> `URLConnection.getInputStream()` and closes that stream.
>>
>> As noted previously, `URLConnection` has a few other APIs, for example the
>> APIs that provide header values. An example:
>>
>>
>> Path regularFile = Path.of("hello.txt");
>> URLConnection conn = regularFile.toUri().toURL().openConnection();
>> // either of the following header related APIs
>> long val = conn.getLastModified();
>> String val = conn.getHeaderField...
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Daniel's suggestion - add reachability fence
test/jdk/sun/net/www/protocol/file/FileURLConnStreamLeakTest.java line 1:
> 1: /*
Converting this entire class to a single `@ParameterizedTest` method (i.e., no
instance field, no `@{Before,After}Each`) can avoid significant duplication.
@ParameterizedTest("connTesters")
void test(Consumer<URLConnection> connTester) {
Path file = Files.createTempFile(Path.of("."), "8367561-", ".txt");
try {
Files.writeString(file, String.valueOf(System.currentTimeMillis()));
URLConnection conn = file.toUri().toURL().openConnection();
assertNotNull(conn, "URLConnection for " + file + " is null");
assertEquals(FILE_URLCONNECTION_CLASSNAME, conn.getClass().getName(),
"unexpected URLConnection type");
connTester.accept(conn);
Reference.reachabilityFence(conn);
} finally {
Files.deleteIfExists(file);
}
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2414359823
