On Fri, 2 Jul 2021 11:06:40 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 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 Hi Jaikiran, Consider: try (var os = Files.newOutputStream(ZIPFILE); ZipOutputStream zos = new ZipOutputStream(os)) { zos.putNextEntry(new ZipEntry("../Hello.txt")); zos.write("Hello World".getBytes(StandardCharsets.UTF_8)); } With your proposed fix, you will only return "/" when you walk the the Zip file and we should also return "/Hello.txt" If. you resolve the path when the Inode entry is created: ` name(new ZipPath(null, normalize(name), true).getResolvedPath());` That should address the issue and also allow you to access the entry. ------------- PR: https://git.openjdk.java.net/jdk/pull/4604
