On Sat, 4 Oct 2025 16:32:16 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:
>
> missed pushing the change
src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
line 145:
> 143: public String getHeaderField(String name) {
> 144: initializeHeaders();
> 145: return super.getHeaderField(name);
Several header related method implementations in this class have been updated
to avoid the `super.XXXX()` calls. The `super.XXX()` implementation was calling
`getInputStream()` and ignoring/not using the returned `InputStream`. That call
to `getInputStream()` was a way to initiate a readability check and if the File
wasn't readable then it would return a different result compared to when it was
readable. The use of `getInputStream()` has been replaced by a call to
`isReadable()` which does the same checks without leaving around a
`InputStream` instance. Apart from that change, the rest of the code that
resided in the `super.XXX()` implementation has been literally copy/pasted in
these methods to avoid any kind of behavioural change.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2404079257
