8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException #22734

jaikiran · 2024-12-13T13:17:39Z

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.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Warning

⚠️ Found leading lowercase letter in issue title for 8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException

Issue

JDK-8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException (Bug - P4)

Reviewers

Lance Andersen (@LanceAndersen - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22734/head:pull/22734
$ git checkout pull/22734

Update a local copy of the PR:
$ git checkout pull/22734
$ git pull https://git.openjdk.org/jdk.git pull/22734/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22734

View PR using the GUI difftool:
$ git pr show -t 22734

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22734.diff

Using Webrev

Link to Webrev Comment

…ception

bridgekeeper · 2024-12-13T13:18:31Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-12-13T13:19:27Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException

Reviewed-by: lancea

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 41 new commits pushed to the master branch:

18d1d61: 8346046: Enable copyright header format check
a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
3030230: 8346278: Clean up some flag handing in flags-cflags.m4
fd0207d: 8345327: JDK 24 RDP1 L10n resource files update
31c3b19: 8346174: UMAX/UMIN are missing from XXXVector::reductionOperations
c75b1d4: 8346082: Output JVMTI agent information in hserr files
51662c2: 8346232: Remove leftovers of the jar --normalize feature
d335941: 8345678: compute_modifiers should not be in create_mirror
57adf64: 8346261: Cleanup in JDP tests
... and 31 more: https://git.openjdk.org/jdk/compare/db9eab3f29e9cb26a8c0a7c31c55aaf140f21bed...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-12-13T13:20:05Z

@jaikiran The following labels will be automatically applied to this pull request:

compiler
core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

jaikiran · 2024-12-13T13:21:53Z

/label remove compiler

openjdk · 2024-12-13T13:22:00Z

@jaikiran
The compiler label was successfully removed.

mlbridge · 2024-12-13T13:24:48Z

Webrevs

01: Full - Incremental (68c695e1)
00: Full (d505e418)

AlanBateman · 2024-12-13T13:31:01Z

src/jdk.jartool/share/classes/sun/tools/jar/Main.java

@@ -431,8 +431,9 @@ public synchronized boolean run(String[] args) {
                    file = new File(fname);
                } else {
                    file = createTemporaryFile("tmpJar", ".jar");
-                    try (InputStream in = new FileInputStream(FileDescriptor.in)) {
-                        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?

Thank you Lance for the review. Alan, is it OK to proceed with this current change or do you think we should pursue the REPLACE_EXISTING option here?

I don't object to what you have, it's the mixing of old and new APIs that jumped out. Maybe some day there will be some wider updates to the jar tool in this area, e.g. it could have use a temp directory rather than a temp file.

Thank you for that input, Alan. I'll go ahead with integrating this current PR. As a separate activity, I will take a broader look at this jar tool code and see what changes can be accomodated to modernize the code.

jaikiran · 2024-12-17T10:14:11Z

/integrate

openjdk · 2024-12-17T10:14:30Z

Going to push as commit 725079b.
Since your change was applied there have been 46 commits pushed to the master branch:

5e25c48: 8346289: Confusing phrasing in IR Framework README / User-defined Regexes
fbbc7c3: 8346120: VirtualThreadPinned event recorded for Object.wait may have wrong duration or may record second event
466c00a: 8346234: javax/swing/text/DefaultEditorKit/4278839/bug4278839.java still fails in CI
bd3c0be: 8268611: jar --validate should check targeted classes in MR-JAR files
87804f2: 8346294: Invalid lint category specified in compiler.properties
18d1d61: 8346046: Enable copyright header format check
a7631cc: 8346235: RISC-V: Optimize bitwise AND with mask values
929d4a5: 8346231: RISC-V: Fix incorrect assertion in SharedRuntime::generate_handler_blob
3030230: 8346278: Clean up some flag handing in flags-cflags.m4
fd0207d: 8345327: JDK 24 RDP1 L10n resource files update
... and 36 more: https://git.openjdk.org/jdk/compare/db9eab3f29e9cb26a8c0a7c31c55aaf140f21bed...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-17T10:14:39Z

@jaikiran Pushed as commit 725079b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jaikiran added 2 commits December 13, 2024 15:30

8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsEx…

64fc72a

…ception

8345506 add test

d505e41

openjdk bot added the rfr Pull request is ready for review label Dec 13, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Dec 13, 2024

openjdk bot removed the compiler compiler-dev@openjdk.org label Dec 13, 2024

AlanBateman reviewed Dec 13, 2024

View reviewed changes

include a test for "jar --update"

68c695e

LanceAndersen approved these changes Dec 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 17, 2024

openjdk bot closed this Dec 17, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException #22734

8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException #22734

jaikiran commented Dec 13, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 13, 2024

openjdk bot commented Dec 13, 2024 •

edited

Loading

openjdk bot commented Dec 13, 2024

jaikiran commented Dec 13, 2024

openjdk bot commented Dec 13, 2024

mlbridge bot commented Dec 13, 2024 •

edited

Loading

AlanBateman Dec 13, 2024

jaikiran Dec 13, 2024 •

edited

Loading

jaikiran Dec 17, 2024

AlanBateman Dec 17, 2024

jaikiran Dec 17, 2024

jaikiran commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException #22734

8345506: jar --validate may lead to java.nio.file.FileAlreadyExistsException #22734

Conversation

jaikiran commented Dec 13, 2024 • edited by openjdk bot Loading

Progress

Warning

Issue

Reviewers

Reviewing

bridgekeeper bot commented Dec 13, 2024

openjdk bot commented Dec 13, 2024 • edited Loading

openjdk bot commented Dec 13, 2024

jaikiran commented Dec 13, 2024

openjdk bot commented Dec 13, 2024

mlbridge bot commented Dec 13, 2024 • edited Loading

Webrevs

AlanBateman Dec 13, 2024

Choose a reason for hiding this comment

jaikiran Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

jaikiran Dec 17, 2024

Choose a reason for hiding this comment

AlanBateman Dec 17, 2024

Choose a reason for hiding this comment

jaikiran Dec 17, 2024

Choose a reason for hiding this comment

jaikiran commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

openjdk bot commented Dec 17, 2024

jaikiran commented Dec 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 13, 2024 •

edited

Loading

mlbridge bot commented Dec 13, 2024 •

edited

Loading

jaikiran Dec 13, 2024 •

edited

Loading