On Fri, 2 Jul 2021 10:25:29 GMT, Jaikiran Pai <j...@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 incrementally with one additional > commit since the last revision: > > implement review suggestions: > - reduce the toString() calls by creating a helper > - fs.getPath("/") instead of fs.getRootDirectories().iterator().next() >I skimmed the test. A helper method that maps a Set to a modifiable Set would >avoid the 10 usages of toString() in the setup, just a suggestion. Done. I introduced a helper in the updated version of the PR. > Also I was puzzled by fs.getRootDirectories().iterator().next() and why this > isn't fs.getPath("/"). When I started with a reproducer and then this test, I used the code that the reporter had attached in the JBS issue. That one had used "fs.getRootDirectories().iterator().next()" to show this issue. I just happened to carry that over into the test. I've updated the PR to now use fs.getPath("/") and also verified that this version of the test continues to fail without the fix proposed in this PR and passes with the changes. ------------- PR: https://git.openjdk.java.net/jdk/pull/4604
