On Tue, 7 Oct 2025 08:07:39 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 with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Daniel's suggestion - don't repeat/copy code from super()
> - merge latest from master branch
> - missed pushing the change
> - 8367561: fix inputstream leak
> - 8367561: introduce tests
src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
line 125:
> 123: } else {
> 124: try (var _ = new FileInputStream(file.getPath())) {
> 125: }
*Nit:* You can consider shortening this to `new
FileInputStream(file.getPath()).close()`.
src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java
line 258:
> 256: is = new BufferedInputStream(new
> FileInputStream(file.getPath()));
> 257: }
> 258: return is;
90% of this work – i.e., getting the file listing – is already done in
`connect()`. `getInputStream()` only concatenates `directoryListing`. Moving
this last 10% logic to `connect()`, making it populate a
private Supplier<InputStream> inputStreamSupplier;
field, can simplify the code. Have you considered this route?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2409922581
PR Review Comment: https://git.openjdk.org/jdk/pull/27633#discussion_r2410326632
