On Fri, 13 Dec 2024 13:31:01 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Can I please get a review of this change which proposes to address the issue >> reported in https://bugs.openjdk.org/browse/JDK-8345506? >> >> The `jar` tool has several operations which take `--file` as a parameter. >> The value for that option is a JAR file path. The `jar` operation is then >> run against that JAR file. The `--file` parameter is optional and when it >> isn't provided, the `jar` tool expects the JAR file content to be streamed >> through STDIN of the `jar` process. >> >> The issue here is that the `--validate` option has a bug in the >> implementation where when the `--file` option is absent, it tries to read >> from the STDIN into a temporary file that the implementation just created. >> To do so it uses `Files.copy(...)` which throws an exception if the >> destination file exists (which it does in this case because that temporary >> destination file was created just a few lines above). >> >> The fix in this commit address this issue by using an alternate way to >> transfer the JAR content into the temporary file. >> >> A new jtreg test has been introduced to reproduce the issue and verify the >> fix. I couldn't locate any other existing test which was exercising the code >> path which deals with `jar` operations against the STDIN of the `jar` >> process. So the new jtreg test has test for other operations and not just >> `--validate` operation. >> >> The new test and existing tests in tier1, tier2 and tier3 continue to pass >> with this change. > > src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 435: > >> 433: file = createTemporaryFile("tmpJar", ".jar"); >> 434: try (InputStream in = new >> FileInputStream(FileDescriptor.in)) { >> 435: Files.copy(in, file.toPath()); > > Did you try adding the REPLACE_EXISTING option to Files.copy, I assume that > will fix it. Hello Alan, I had thought about that, but then I looked at the implementation of `Files.copy(...)` with `REPLACE_EXISTING`. If that option is specified, the `Files.copy(...)` implementation first deletes the existing file: // attempt to delete an existing file if (replaceExisting) { deleteIfExists(target); } before it initiates the copying. It didn't feel right to be explicitly creating a file (before the call to Files.copy) and then having it deleted due to the use of `REPLACE_EXISTING`. So I went ahead with this alternate approach. Would you still prefer that we use `REPLACE_EXISTING` here? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22734#discussion_r1883954830
