RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]

[Jaikiran Pai]

On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this proposed fix for https://bugs.openjdk.java.net/browse/JDK-8251329?
>> 
>> As noted in that issue, if a zip filesystem created on top of a jar containing a "./" entry is "walked" using the `Files.walkFileTree`, it leads to a infinite never ending iteration (which ultimately fails with Java heap space OOM).
>> 
>> Alan notes in that issue that:
>> 
>>> This is more likely an issue with the zipfs DirectoryStream implementation. A DirectoryStream is specified to not include elements that for the special links to the current or parent directory. It should be rare. 
>> 
>> This indeed turned out to be an issue in the `jdk.nio.zipfs.ZipDirectoryStream#iterator()` implementation where it calls the `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` implementation.  The implementation, unlike what the javadoc of `java.nio.file.DirectoryStream` states, was including the "." and ".." paths in its returned iterator:
>> 
>>> The elements returned by the iterator are in no specific order. Some file
>>  systems maintain special links to the directory itself and the directory's
>>   parent directory. Entries representing these links are not returned by the
>>   iterator.
>> 
>> 
>> The proposed fix in this patch checks the paths for "." and "..", similar to what the `sun.nio.fs.UnixDirectoryStream` does in its `isSelfOrParent` and skips those paths from being added into the returned iterator. The `jdk.nio.zipfs.ZipFileSystem#iteratorOf(...)` (where this change has been done) is currently only used by `jdk.nio.zipfs.ZipDirectoryStream#iterator()` and has package-private visibility, so this change shouldn't impact any other usage/expectations.
>> 
>> A new jtreg test has been added to reproduce this issue and verify the fix. Local testing of the `test/jdk/jdk/nio/` (including this new test) went fine without any issues after this change. I will be triggering a `tier1` test locally in a while.
>
> 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 four additional commits since the last revision:
> 
>  - Merge latest from upstream master branch to bring in fixes that might help fix the unrelated tier1 failures in Github Actions job runs
>  - implement review suggestions:
>     - reduce the toString() calls by creating a helper
>     - fs.getPath("/") instead of fs.getRootDirectories().iterator().next()
>  - implement review suggestion - move isSelfOrParent to ZipPath class
>  - 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside

Hello Alan,

> So I think we should close this PR and restart JDK-8251329. @jaikiran Are you okay this this?

Yes, this sounds fine to me. I don't have any objection in closing this PR and I'll do it shortly.

> JDK-8251329 is still assigned to JDK-8251329 and has a patch with the changes that we think are the right way for newFileSystem to reject ZIP files 

This part I didn't understand. Did you mean to refer some other JBS issue? Because from what I see in https://bugs.openjdk.java.net/browse/JDK-8251329 there's no patch attached to it (unless of course it's restricted to specific JBS user roles?).

-------------

PR: https://git.openjdk.java.net/jdk/pull/4604