Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v6]
On Fri, 30 Jul 2021 17:22:59 GMT, Lance Andersen wrote: >> Hi, >> >> As discussed in the >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html >> thread, this is the revised patch to address the use of '.' and '..' within >> Zip FS >> >> Zip FS needs to use "." and ".." as links to the current and parent >> directories and cannot reliably support entries that have "." and ".." as >> name elements. This patch updates Zip Fs to reject ZIP files that have >> entries in the CEN that can't be used for files in a file system. >> >> >> Mach5 tiers 1 through 3 have been run without any errors encountered . >> >> Best, >> Lance > > Lance Andersen has updated the pull request incrementally with one additional > commit since the last revision: > > Add missing space in if statement Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v6]
> Hi, > > As discussed in the > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html > thread, this is the revised patch to address the use of '.' and '..' within > Zip FS > > Zip FS needs to use "." and ".." as links to the current and parent > directories and cannot reliably support entries that have "." and ".." as > name elements. This patch updates Zip Fs to reject ZIP files that have > entries in the CEN that can't be used for files in a file system. > > > Mach5 tiers 1 through 3 have been run without any errors encountered . > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add missing space in if statement - Changes: - all: https://git.openjdk.java.net/jdk/pull/4900/files - new: https://git.openjdk.java.net/jdk/pull/4900/files/0bf065ea..292e698f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v5]
On Fri, 30 Jul 2021 15:43:52 GMT, Lance Andersen wrote:
>> Hi,
>>
>> As discussed in the
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
>> thread, this is the revised patch to address the use of '.' and '..' within
>> Zip FS
>>
>> Zip FS needs to use "." and ".." as links to the current and parent
>> directories and cannot reliably support entries that have "." and ".." as
>> name elements. This patch updates Zip Fs to reject ZIP files that have
>> entries in the CEN that can't be used for files in a file system.
>>
>>
>> Mach5 tiers 1 through 3 have been run without any errors encountered .
>>
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update note added to Zip FS module-info
Looks good to me.
test/jdk/jdk/nio/zipfs/HasDotDotTest.java line 116:
> 114: @Test(dataProvider = "checkForDotOrDotDotPaths")
> 115: public void hasDotOrDotDotTest(String path) throws IOException {
> 116: if(DEBUG) {
Nit: space after `if`
-
Marked as reviewed by naoto (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
On Fri, 30 Jul 2021 11:24:37 GMT, Alan Bateman wrote: >> src/jdk.zipfs/share/classes/module-info.java line 49: >> >>> 47: * >>> 48: * @implNote The Zip File System will throw a ZipException when opening >>> an >>> 49: * existing Zip file that contains Zip entries with "." or ".." in its >>> name elements. >> >> Hello Lance, reading this sentence adds a bit of confusion since it uses the >> word "contains". Had I not known the implemenation details, this sentence >> would have made me think zip file with name elements of the form `foo.bar` >> or `hello..` would also be rejected since these name elements "contain" `.` >> or `..` >> >> Do you think we should change the word to something like "The Zip File >> System will throw a ZipException when opening an existing Zip file that has >> "." or ".." named entries"? > > I think the `@implNote` tag can be dropped here. For the text then maybe it > could be tweaked to something like "The ZIP file system provider does not > support opening an existing ZIP that contains entries with ...". Updated the note removing `@impleNote` thought about pre-pending **Note:** to make the wording stick out but held off for now. Updated the Release note accordingly Jaikiran, I think the use of name elements is fine as we use similar phrasing in `Path::getName()` - PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v5]
> Hi, > > As discussed in the > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html > thread, this is the revised patch to address the use of '.' and '..' within > Zip FS > > Zip FS needs to use "." and ".." as links to the current and parent > directories and cannot reliably support entries that have "." and ".." as > name elements. This patch updates Zip Fs to reject ZIP files that have > entries in the CEN that can't be used for files in a file system. > > > Mach5 tiers 1 through 3 have been run without any errors encountered . > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Update note added to Zip FS module-info - Changes: - all: https://git.openjdk.java.net/jdk/pull/4900/files - new: https://git.openjdk.java.net/jdk/pull/4900/files/67c726af..0bf065ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
On Fri, 30 Jul 2021 01:43:56 GMT, Jaikiran Pai wrote: >> Lance Andersen 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 six additional >> commits since the last revision: >> >> - Add Impl Note to Zip FS module-info >> - Merge >> - Add missing Copyright header and address minor comments >> - Address missing linefeed after package name >> - Address overzelous intellij import update >> - Patch to address JDK-8251329 > > src/jdk.zipfs/share/classes/module-info.java line 49: > >> 47: * >> 48: * @implNote The Zip File System will throw a ZipException when opening >> an >> 49: * existing Zip file that contains Zip entries with "." or ".." in its >> name elements. > > Hello Lance, reading this sentence adds a bit of confusion since it uses the > word "contains". Had I not known the implemenation details, this sentence > would have made me think zip file with name elements of the form `foo.bar` or > `hello..` would also be rejected since these name elements "contain" `.` or > `..` > > Do you think we should change the word to something like "The Zip File System > will throw a ZipException when opening an existing Zip file that has "." or > ".." named entries"? I think the `@implNote` tag can be dropped here. For the text then maybe it could be tweaked to something like "The ZIP file system provider does not support opening an existing ZIP that contains entries with ...". - PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
On Thu, 29 Jul 2021 18:21:07 GMT, Lance Andersen wrote: >> Hi, >> >> As discussed in the >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html >> thread, this is the revised patch to address the use of '.' and '..' within >> Zip FS >> >> Zip FS needs to use "." and ".." as links to the current and parent >> directories and cannot reliably support entries that have "." and ".." as >> name elements. This patch updates Zip Fs to reject ZIP files that have >> entries in the CEN that can't be used for files in a file system. >> >> >> Mach5 tiers 1 through 3 have been run without any errors encountered . >> >> Best, >> Lance > > Lance Andersen 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 six additional > commits since the last revision: > > - Add Impl Note to Zip FS module-info > - Merge > - Add missing Copyright header and address minor comments > - Address missing linefeed after package name > - Address overzelous intellij import update > - Patch to address JDK-8251329 src/jdk.zipfs/share/classes/module-info.java line 49: > 47: * > 48: * @implNote The Zip File System will throw a ZipException when opening an > 49: * existing Zip file that contains Zip entries with "." or ".." in its > name elements. Hello Lance, reading this sentence adds a bit of confusion since it uses the word "contains". Had I not known the implemenation details, this sentence would have made me think zip file with name elements of the form `foo.bar` or `hello..` would also be rejected since these name elements "contain" `.` or `..` Do you think we should change the word to something like "The Zip File System will throw a ZipException when opening an existing Zip file that has "." or ".." named entries"? - PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
> Hi, > > As discussed in the > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html > thread, this is the revised patch to address the use of '.' and '..' within > Zip FS > > Zip FS needs to use "." and ".." as links to the current and parent > directories and cannot reliably support entries that have "." and ".." as > name elements. This patch updates Zip Fs to reject ZIP files that have > entries in the CEN that can't be used for files in a file system. > > > Mach5 tiers 1 through 3 have been run without any errors encountered . > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Remove uneeded variable - Changes: - all: https://git.openjdk.java.net/jdk/pull/4900/files - new: https://git.openjdk.java.net/jdk/pull/4900/files/22731ac3..67c726af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
On Thu, 29 Jul 2021 18:21:07 GMT, Lance Andersen wrote: >> Hi, >> >> As discussed in the >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html >> thread, this is the revised patch to address the use of '.' and '..' within >> Zip FS >> >> Zip FS needs to use "." and ".." as links to the current and parent >> directories and cannot reliably support entries that have "." and ".." as >> name elements. This patch updates Zip Fs to reject ZIP files that have >> entries in the CEN that can't be used for files in a file system. >> >> >> Mach5 tiers 1 through 3 have been run without any errors encountered . >> >> Best, >> Lance > > Lance Andersen 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 six additional > commits since the last revision: > > - Add Impl Note to Zip FS module-info > - Merge > - Add missing Copyright header and address minor comments > - Address missing linefeed after package name > - Address overzelous intellij import update > - Patch to address JDK-8251329 src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1622: > 1620: index++; > 1621: } > 1622: return dotOrDotDotFound; Could simply return `false` here, and eliminate the local variable `dotOrDotDotFound`. - PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
> Hi, > > As discussed in the > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html > thread, this is the revised patch to address the use of '.' and '..' within > Zip FS > > Zip FS needs to use "." and ".." as links to the current and parent > directories and cannot reliably support entries that have "." and ".." as > name elements. This patch updates Zip Fs to reject ZIP files that have > entries in the CEN that can't be used for files in a file system. > > > Mach5 tiers 1 through 3 have been run without any errors encountered . > > Best, > Lance Lance Andersen 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 six additional commits since the last revision: - Add Impl Note to Zip FS module-info - Merge - Add missing Copyright header and address minor comments - Address missing linefeed after package name - Address overzelous intellij import update - Patch to address JDK-8251329 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4900/files - new: https://git.openjdk.java.net/jdk/pull/4900/files/2265ffe1..22731ac3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=01-02 Stats: 3049 lines in 76 files changed: 1357 ins; 359 del; 1333 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
On Mon, 26 Jul 2021 10:16:47 GMT, Lance Andersen wrote:
>> Hi,
>>
>> As discussed in the
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
>> thread, this is the revised patch to address the use of '.' and '..' within
>> Zip FS
>>
>> Zip FS needs to use "." and ".." as links to the current and parent
>> directories and cannot reliably support entries that have "." and ".." as
>> name elements. This patch updates Zip Fs to reject ZIP files that have
>> entries in the CEN that can't be used for files in a file system.
>>
>>
>> Mach5 tiers 1 through 3 have been run without any errors encountered .
>>
>> Best,
>> Lance
>
> Lance Andersen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add missing Copyright header and address minor comments
> zipfs.getPath("./Hello.txt"),
> zipfs.getPath("../../../Hello.txt"),
> zipfs.getPath("../Hello.txt")};
>
>
> In other words, the `ZipFileSystem` doesn't end up creating a zip file which
> is then rejected by `ZipFileSystem` when creating a new filesystem using
> `Files.newFileSystem`. That's a good thing.
Right, it's always existing behavior and matches the behavior of the platform
file system.
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Mon, 26 Jul 2021 09:52:09 GMT, Jaikiran Pai wrote:
> This change looks fine to me. I was unsure how the writing/creating entries
> with `.` or `..` with `ZipFileSystem` would behave in context of this change,
> so I gave this a try locally with the changes from this PR:
>
> ```
> try (FileSystem zipfs = FileSystems.newFileSystem(ZIPFILE)) {
> final Path[] paths = new Path[]{
> zipfs.getPath("./Hello.txt"),
> zipfs.getPath("../../../Hello.txt"),
> zipfs.getPath("../Hello.txt")};
> for (int i = 0; i < paths.length; i++) {
> try (OutputStream os = Files.newOutputStream(paths[i])) {
> os.write(("Hello " + i).getBytes(StandardCharsets.UTF_8));
> }
> }
> }
> ```
>
> This code runs fine and it ends up creating (just one) CEN entry for
> `Hello.txt`:
>
> ```
> JTwork/scratch/zipfsDotDotTest.zip
> Length DateTimeName
> - -- -
> 7 07-26-2021 15:07 Hello.txt
> ```
>
> In other words, the `ZipFileSystem` doesn't end up creating a zip file which
> is then rejected by `ZipFileSystem` when creating a new filesystem using
> `Files.newFileSystem`. That's a good thing.
With the exception of creating the Inodes table, Zip FS always calls
ZipPath::getResolvedPath for access to Zip entries. So there is no issues with
the creation and access of entries in the case above
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
On Mon, 26 Jul 2021 07:30:12 GMT, Alan Bateman wrote:
>> Lance Andersen has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add missing Copyright header and address minor comments
>
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1573:
>
>> 1571: }
>> 1572: IndexNode inode = new IndexNode(cen, pos, nlen);
>> 1573: if(hasDotOrDotDot(inode.name)) {
>
> Minor nit, missing space in "if(".
fixed
> src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1602:
>
>> 1600: // Inode.name always includes "/" in path[0]
>> 1601: if (path.length == 1) {
>> 1602: return false;
>
> It may be useful to add "assert path[0] == '/';" at the start of this method.
I added it per your suggestion, though IndexNode(byte[] cen, int pos, int nlen)
which is only used by initCen() will already guarantee the leading "/" is there
> test/jdk/jdk/nio/zipfs/HasDotDotTest.java line 1:
>
>> 1: import org.testng.annotations.DataProvider;
>
> Missing copyright header.
Geez, how did I miss that. Added
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
> Hi, > > As discussed in the > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html > thread, this is the revised patch to address the use of '.' and '..' within > Zip FS > > Zip FS needs to use "." and ".." as links to the current and parent > directories and cannot reliably support entries that have "." and ".." as > name elements. This patch updates Zip Fs to reject ZIP files that have > entries in the CEN that can't be used for files in a file system. > > > Mach5 tiers 1 through 3 have been run without any errors encountered . > > Best, > Lance Lance Andersen has updated the pull request incrementally with one additional commit since the last revision: Add missing Copyright header and address minor comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4900/files - new: https://git.openjdk.java.net/jdk/pull/4900/files/68af64c4..2265ffe1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4900&range=00-01 Stats: 25 lines in 2 files changed: 24 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4900.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4900/head:pull/4900 PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Sun, 25 Jul 2021 21:56:10 GMT, Lance Andersen wrote:
> Hi,
>
> As discussed in the
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
> thread, this is the revised patch to address the use of '.' and '..' within
> Zip FS
>
> Zip FS needs to use "." and ".." as links to the current and parent
> directories and cannot reliably support entries that have "." and ".." as
> name elements. This patch updates Zip Fs to reject ZIP files that have
> entries in the CEN that can't be used for files in a file system.
>
>
> Mach5 tiers 1 through 3 have been run without any errors encountered .
>
> Best,
> Lance
This change looks fine to me. I was unsure how the writing/creating entries
with `.` or `..` with `ZipFileSystem` would behave in context of this change,
so I gave this a try locally with the changes from this PR:
try (FileSystem zipfs = FileSystems.newFileSystem(ZIPFILE)) {
final Path[] paths = new Path[]{
zipfs.getPath("./Hello.txt"),
zipfs.getPath("../../../Hello.txt"),
zipfs.getPath("../Hello.txt")};
for (int i = 0; i < paths.length; i++) {
try (OutputStream os = Files.newOutputStream(paths[i])) {
os.write(("Hello " + i).getBytes(StandardCharsets.UTF_8));
}
}
}
This code runs fine and it ends up creating (just one) CEN entry for
`Hello.txt`:
JTwork/scratch/zipfsDotDotTest.zip
Length DateTimeName
- -- -
7 07-26-2021 15:07 Hello.txt
In other words, the `ZipFileSystem` doesn't end up creating a zip file which is
then rejected by `ZipFileSystem` when creating a new filesystem using
`Files.newFileSystem`. That's a good thing.
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Sun, 25 Jul 2021 21:56:10 GMT, Lance Andersen wrote:
> Hi,
>
> As discussed in the
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-July/079621.html
> thread, this is the revised patch to address the use of '.' and '..' within
> Zip FS
>
> Zip FS needs to use "." and ".." as links to the current and parent
> directories and cannot reliably support entries that have "." and ".." as
> name elements. This patch updates Zip Fs to reject ZIP files that have
> entries in the CEN that can't be used for files in a file system.
>
>
> Mach5 tiers 1 through 3 have been run without any errors encountered .
>
> Best,
> Lance
This is behavior change (to reject zip/JAR files) so a CSR will be required.
I've also added a label to the JBS issue to remind us to add a release note.
I think it would be good for @jaikiran to review too.
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1573:
> 1571: }
> 1572: IndexNode inode = new IndexNode(cen, pos, nlen);
> 1573: if(hasDotOrDotDot(inode.name)) {
Minor nit, missing space in "if(".
src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1602:
> 1600: // Inode.name always includes "/" in path[0]
> 1601: if (path.length == 1) {
> 1602: return false;
It may be useful to add "assert path[0] == '/';" at the start of this method.
test/jdk/jdk/nio/zipfs/HasDotDotTest.java line 1:
> 1: import org.testng.annotations.DataProvider;
Missing copyright header.
-
PR: https://git.openjdk.java.net/jdk/pull/4900
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 23 Jul 2021 10:36:51 GMT, Alan Bateman wrote: > I wasn't suggesting there is a patch attached to that issue. Rather I was > just pointing out that JDK-8251329 was being worked on already before this PR > was created. Ok, that makes sense. Thank you for the details. - PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 23 Jul 2021 08:47:46 GMT, Jaikiran Pai wrote: > 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?). I wasn't suggesting there is a patch attached to that issue. Rather I was just pointing out that JDK-8251329 was being worked on already before this PR was created. Lance's initial patch for JDK-8251329 changed ZipPath::getResolvedPath when creating the inode entry but that approach leads to anomalies. Overall I think the discussion has been useful but I don't think it's feasible to have a file system view over entries that look like links to the current or parent directory. - PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai 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
Closing this PR based on the above discussion.
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai 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
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:08:43 GMT, Lance Andersen wrote:
>> 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.
I discussed this issue with @LanceAndersen and I think we are in agreement that
a zip file system needs to use "." and ".." as links to the current and parent
directories and cannot reliably support entries that have "." and ".." as name
elements. The other options on the table (including Lance's origin prototype
fix and the proposal in this PR) lead to anomalies. So I think we should close
this PR and restart JDK-8251329. @jaikiran Are you okay this this? 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 that have entries in
the CEN that can't be used for files in a file system.
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On 02/07/2021 18:16, Lance Andersen wrote: : That being said, if we want to follow Alan’s suggestion and throw an Exception, I am OK with that as well. Either way, we currently cannot access the file via Zip FS due to the call to ZipPath::getResolvedPath() for all access and the path is only normalized when the Inodes are created. Alan, do you have a specific preference? Not yet but if you tolerate these hostile entries then it means that all access with relative paths containing a "." or ".." element will be ambiguous. It will break relativize, normalize, and maybe other path operations. I assume it will break copy/move operations when the source is in a zip file and the target is in a non-zip file. I suspect it would also need to encode file names when open zip file for read/write access because the native file system is used for caching. -Alan
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Jul 2, 2021, at 12:13 PM, Lance Andersen
mailto:lance.ander...@oracle.com>> wrote:
On Jul 2, 2021, at 8:08 AM, Jaikiran Pai
mailto:jai.forums2...@gmail.com>> wrote:
Hello Lance,
On 02/07/21 4:42 pm, Lance Andersen wrote:
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"
Thank you for noticing this issue in my change and bringing this up. I have a
question around this use case. Please consider a small variation to your
example as below:
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));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
}
Notice that I now have a zip/jar which has 2 differently named entries
"../Hello.txt" and "/Hello.txt". This creates the archive without any issues
and those 2 entries are noted in its listing. Now assuming someone walks over
this jar using the ZipFileSystem, starting at root ("/"), what should be the
expected output? The path(s) returned will end up being "/" and "/Hello.txt"
but which resource is expected to be served in this case? The one which has
"Hello World" in it or the one which has "Bye bye"? This example can be
extended a bit more by introducing a "./Hello.txt", in the jar, with yet
another different content in that entry. Is there some specification for this
that I can check and adapt the test case accordingly?
Consider Hello.zip created via:
try (var os = Files.newOutputStream(Path.of("Hello.zip"));
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello".getBytes(StandardCharsets.UTF_8));
zos.putNextEntry(new ZipEntry("Hello.txt"));
zos.write("Another Hello".getBytes(StandardCharsets.UTF_8));
}
Winzip does not handle this case well.
InfoZip will :
-
unzip ../Hello.zip
Archive: ../Hello.zip
warning: skipped "../" path component(s) in ../Hello.txt
inflating: Hello.txt
replace Hello.txt? [y]es, [n]o, [A]ll, [N]one, [r]ename: r
new name: foo.txt
inflating: foo.txt
% ls
Hello.txt foo.txt
% cat Hello.txt
Hello
% cat foo.txt
Another Hello
%
This scenario is not well covered in the Zip spec, so we can do what we think
is best.
Personally, I am OK with resolving the path. The odds of having multiple
relative paths to the same file would be a mistake in creating the Zip.
That being said, if we want to follow Alan’s suggestion and throw an Exception,
I am OK with that as well.
Either way, we currently cannot access the file via Zip FS due to the call to
ZipPath::getResolvedPath() for all access and the path is only normalized when
the Inodes are created.
Alan, do you have a specific preference?
One more datapoint
Consider:
try (FileSystem zipfs =
FileSystems.newFileSystem(Path.of("ZipFSHello.zip"),
Map.of("create", "true"))) {
Files.writeString(zipfs.getPath("HelloZipfs.txt"), "Hello");
Files.writeString(zipfs.getPath("../HelloZipfs.txt"), "A ZipFS Hello");
}
The above will result in a Zip with a single entry of “HelloZipfs.txt” and a
value of “A ZipFS Hello”
As mentioned in my previous comment, this is due to the use of
ZipPath::getResolvedPath() which brings me back to the suggestion of doing the
same when we create the Inodes given this is an a virtual FS.
HTH
Best
Lance
-Jaikiran
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Jul 2, 2021, at 8:08 AM, Jaikiran Pai
mailto:jai.forums2...@gmail.com>> wrote:
Hello Lance,
On 02/07/21 4:42 pm, Lance Andersen wrote:
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"
Thank you for noticing this issue in my change and bringing this up. I have a
question around this use case. Please consider a small variation to your
example as below:
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));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
}
Notice that I now have a zip/jar which has 2 differently named entries
"../Hello.txt" and "/Hello.txt". This creates the archive without any issues
and those 2 entries are noted in its listing. Now assuming someone walks over
this jar using the ZipFileSystem, starting at root ("/"), what should be the
expected output? The path(s) returned will end up being "/" and "/Hello.txt"
but which resource is expected to be served in this case? The one which has
"Hello World" in it or the one which has "Bye bye"? This example can be
extended a bit more by introducing a "./Hello.txt", in the jar, with yet
another different content in that entry. Is there some specification for this
that I can check and adapt the test case accordingly?
Consider Hello.zip created via:
try (var os = Files.newOutputStream(Path.of("Hello.zip"));
ZipOutputStream zos = new ZipOutputStream(os)) {
zos.putNextEntry(new ZipEntry("../Hello.txt"));
zos.write("Hello".getBytes(StandardCharsets.UTF_8));
zos.putNextEntry(new ZipEntry("Hello.txt"));
zos.write("Another Hello".getBytes(StandardCharsets.UTF_8));
}
Winzip does not handle this case well.
InfoZip will :
-
unzip ../Hello.zip
Archive: ../Hello.zip
warning: skipped "../" path component(s) in ../Hello.txt
inflating: Hello.txt
replace Hello.txt? [y]es, [n]o, [A]ll, [N]one, [r]ename: r
new name: foo.txt
inflating: foo.txt
% ls
Hello.txt foo.txt
% cat Hello.txt
Hello
% cat foo.txt
Another Hello
%
This scenario is not well covered in the Zip spec, so we can do what we think
is best.
Personally, I am OK with resolving the path. The odds of having multiple
relative paths to the same file would be a mistake in creating the Zip.
That being said, if we want to follow Alan’s suggestion and throw an Exception,
I am OK with that as well.
Either way, we currently cannot access the file via Zip FS due to the call to
ZipPath::getResolvedPath() for all access and the path is only normalized when
the Inodes are created.
Alan, do you have a specific preference?
-Jaikiran
[cid:E1C4E2F0-ECD0-4C9D-ADB4-B16CA7BCB7FC@home]
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On 02/07/2021 13:08, Jaikiran Pai wrote:
Thank you for noticing this issue in my change and bringing this up. I
have a question around this use case. Please consider a small
variation to your example as below:
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));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
}
Notice that I now have a zip/jar which has 2 differently named entries
"../Hello.txt" and "/Hello.txt". This creates the archive without any
issues and those 2 entries are noted in its listing. Now assuming
someone walks over this jar using the ZipFileSystem, starting at root
("/"), what should be the expected output? The path(s) returned will
end up being "/" and "/Hello.txt" but which resource is expected to be
served in this case? The one which has "Hello World" in it or the one
which has "Bye bye"? This example can be extended a bit more by
introducing a "./Hello.txt", in the jar, with yet another different
content in that entry. Is there some specification for this that I can
check and adapt the test case accordingly?
This is an area where treating a zip file as a virtual file system
doesn't quite work. We may have to look at zipfs ignoring entries that
contain "." or ".." name elements or have it throw an exception if they
are encountered. I think the main thing is that the APIs and access
behaves consistently.
-Alan
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
Hello Lance,
On 02/07/21 4:42 pm, Lance Andersen wrote:
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"
Thank you for noticing this issue in my change and bringing this up. I
have a question around this use case. Please consider a small variation
to your example as below:
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));
zos.closeEntry();
zos.putNextEntry(new ZipEntry("/Hello.txt"));
zos.write("Bye bye".getBytes(StandardCharsets.UTF_8));
zos.closeEntry();
}
Notice that I now have a zip/jar which has 2 differently named entries
"../Hello.txt" and "/Hello.txt". This creates the archive without any
issues and those 2 entries are noted in its listing. Now assuming
someone walks over this jar using the ZipFileSystem, starting at root
("/"), what should be the expected output? The path(s) returned will end
up being "/" and "/Hello.txt" but which resource is expected to be
served in this case? The one which has "Hello World" in it or the one
which has "Bye bye"? This example can be extended a bit more by
introducing a "./Hello.txt", in the jar, with yet another different
content in that entry. Is there some specification for this that I can
check and adapt the test case accordingly?
-Jaikiran
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
On Fri, 2 Jul 2021 11:06:40 GMT, Jaikiran Pai 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
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v4]
> 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
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4604/files
- new: https://git.openjdk.java.net/jdk/pull/4604/files/8e77d289..abae52bb
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4604&range=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4604&range=02-03
Stats: 10322 lines in 336 files changed: 6220 ins; 2802 del; 1300 mod
Patch: https://git.openjdk.java.net/jdk/pull/4604.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4604/head:pull/4604
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
On Fri, 2 Jul 2021 10:25:29 GMT, Jaikiran Pai 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
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v3]
> 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()
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/4604/files
- new: https://git.openjdk.java.net/jdk/pull/4604/files/3971ad6f..8e77d289
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4604&range=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4604&range=01-02
Stats: 18 lines in 1 file changed: 6 ins; 0 del; 12 mod
Patch: https://git.openjdk.java.net/jdk/pull/4604.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/4604/head:pull/4604
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
On Thu, 1 Jul 2021 13:25:32 GMT, Jaikiran Pai 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 suggestion - move isSelfOrParent to ZipPath class
The implementation change look good.
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. Also I was puzzled by fs.getRootDirectories().iterator().next() and
why this isn't fs.getPath("/").
-
PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
On Thu, 1 Jul 2021 13:05:26 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> implement review suggestion - move isSelfOrParent to ZipPath class > > src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1971: > >> 1969: } >> 1970: return false; >> 1971: } > > It might be a bit clearer if ZipPath define isSelfOrParent(), that would > avoid needing to expose the internal representation. Done. I've updated the PR to implement this suggestion. - PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside [v2]
> 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 suggestion - move isSelfOrParent to ZipPath class - Changes: - all: https://git.openjdk.java.net/jdk/pull/4604/files - new: https://git.openjdk.java.net/jdk/pull/4604/files/344da6cb..3971ad6f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4604&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4604&range=00-01 Stats: 20 lines in 2 files changed: 8 ins; 9 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4604.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4604/head:pull/4604 PR: https://git.openjdk.java.net/jdk/pull/4604
Re: RFR: 8251329: (zipfs) Files.walkFileTree walks infinitely if zip has dir named "." inside
On Sun, 27 Jun 2021 13:13:42 GMT, Jaikiran Pai 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. src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 1971: > 1969: } > 1970: return false; > 1971: } It might be a bit clearer if ZipPath define isSelfOrParent(), that would avoid needing to expose the internal representation. - PR: https://git.openjdk.java.net/jdk/pull/4604
