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
Marked as reviewed by dfuchs (Reviewer).
-------------
PR Review: https://git.openjdk.org/jdk/pull/27633#pullrequestreview-3314936881
