RFR JDK-8170114 jimage extract to not an empty directory overwrites content of the directory

[Michal Vala]

Hi,

sending fix for jimage bug JDK-8170114[1]. I'm not sure whether this is correct
list. If it is not, please direct me somewhere else.

I don't have an openjdk account, so webrev is on my fedora public space. I will
need a sponsor for this.

webrev: https://michalvala.fedorapeople.org/webrevs/JDK-8170114/webrev.00/webrev/

Patch validates output directory before any jimage extracting happen. I've moved
validation to extra private method as it is few lines of code. I've also added
proper error message for case when output path is not a directory
(JImageTask.java#449).

I've implemented beforeTest method, that cleans cwd. This must be done because
files were extracted to cwd and constantly overwriting and tests were
non-deterministic (order did matter). Plus some tests added.




I want to fix JDK-8170120 with IOException when file is not an jimage. It is
blocked by this. This will fix 3 now problematic and excluded tests.

[1] - https://bugs.openjdk.java.net/browse/JDK-8170114

[Alan Bateman]

Post by Michal Vala
Hi,
sending fix for jimage bug JDK-8170114[1]. I'm not sure whether this
is correct list. If it is not, please direct me somewhere else.
I don't have an openjdk account, so webrev is on my fedora public
space. I will need a sponsor for this.
https://michalvala.fedorapeople.org/webrevs/JDK-8170114/webrev.00/webrev/

Do you have any buddies in Java team in Red Hat that you publish this on
cr.openjdk.java.net? We can't look at/accept contributions that are
published on non-OpenJDK infrastructure. If you don't then can you
include the patch in a mail.

-Alan

[Andrew Hughes]

Post by Alan Bateman

Post by Michal Vala
Hi,
sending fix for jimage bug JDK-8170114[1]. I'm not sure whether this is
correct list. If it is not, please direct me somewhere else.
I don't have an openjdk account, so webrev is on my fedora public space. I
will need a sponsor for this.
https://michalvala.fedorapeople.org/webrevs/JDK-8170114/webrev.00/webrev/

Do you have any buddies in Java team in Red Hat that you publish this on
cr.openjdk.java.net? We can't look at/accept contributions that are
published on non-OpenJDK infrastructure. If you don't then can you include
the patch in a mail.
-Alan

http://cr.openjdk.java.net/~shade/8170114/webrev.01/

FWIW, it looks like a pretty straight-forward fix to me.
--
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Web Site: http://fuseyism.com
Twitter: https://twitter.com/gnu_andrew_java
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222

[Michal Vala]

ah, accidentally replied just to Alan :/ Here it is


---------- Forwarded message ----------
From: Michal Vala <***@redhat.com>
Date: Fri, Feb 9, 2018 at 6:21 PM
Subject: Re: RFR JDK-8170114 jimage extract to not an empty directory
overwrites content of the directory
To: Alan Bateman <***@oracle.com>


sure, here it is http://cr.openjdk.java.net/~shade/8170114/webrev.01/

Post by Alan Bateman

Post by Michal Vala
Hi,
sending fix for jimage bug JDK-8170114[1]. I'm not sure whether this is
correct list. If it is not, please direct me somewhere else.
I don't have an openjdk account, so webrev is on my fedora public space. I
will need a sponsor for this.
https://michalvala.fedorapeople.org/webrevs/JDK-8170114/webrev.00/webrev/

Do you have any buddies in Java team in Red Hat that you publish this on
cr.openjdk.java.net? We can't look at/accept contributions that are
published on non-OpenJDK infrastructure. If you don't then can you include
the patch in a mail.
-Alan

[mandy chung]

Post by Michal Vala
Patch validates output directory before any jimage extracting happen.
I've moved validation to extra private method as it is few lines of
code. I've also added proper error message for case when output path
is not a directory (JImageTask.java#449).

Thanks for looking at JDK-8170114 and JDK-8170120.  I took a look at 
http://cr.openjdk.java.net/~shade/8170114/webrev.01/

Alternatively,  jimage extract can behave as jlink and it fails if the
specified output directory exists including empty directory. It'd be
easy to delete the directory in the command-line before running jimage.

You extend JImageCliTest to handle the beforeTest method to be invoked
before running each test case.  Is that necessary?  The test itself is
creating temp file/directory for each test case.    I think it'd be good
to update the test to create a named file/dir under the scratch area as
jtreg will take care of cleaning the scratch area.

Mandy

[Alan Bateman]

Post by mandy chung

Post by Michal Vala
Patch validates output directory before any jimage extracting happen.
I've moved validation to extra private method as it is few lines of
code. I've also added proper error message for case when output path
is not a directory (JImageTask.java#449).

Thanks for looking at JDK-8170114 and JDK-8170120.  I took a look at 
http://cr.openjdk.java.net/~shade/8170114/webrev.01/

Yes, thanks for looking at these issues.

For `jimage info <not-a-jimage-file>` issue then jimage correctly fails
(with a non-zero status) but the IOException isn't converted into an
error message as it does with other errors. Yes, it would be good to fix
that.

I think the `jimage extract --dir <existing-file> <jimage-file>`
scenario needs discussion. If <existing-file> is a non-directory file
then jimage has to fail, I don't expect disagreement on that. For the
case where it is an existing directory then the options seem to be:

1. Extract into the existing directory (existing JDK 9 and JDK 10 behavior)
2. Fail if it's not empty (your patch)
3. Fail if it exists (Mandy's mail, the motivation being to keep it
consistent with jlink)

I view `jimage extract --dir <dir>` as being similar to `unzip -d <dir>`
so I don't think existing behavior (#1) is incorrect, the only issue is
that it silently overrides files whereas unzip will prompt before
overriding (unless you specify -o). The `jar` tool, and legacy `tar`
tool side with `jimage` and are happy to silently replace existing files.

What would you think about focusing on the override case instead of
disallowing extracting into an existing non-empty directory? I realize
this is more work as it means deciding on whether to prompt, warn or
fail. It also means thinking about the equivalent of unzip -o to
allowing existing files be replaced.

-Alan

[Michal Vala]

Post by Alan Bateman

Post by mandy chung

Post by Michal Vala
Patch validates output directory before any jimage extracting happen. I've
moved validation to extra private method as it is few lines of code. I've
also added proper error message for case when output path is not a directory
(JImageTask.java#449).

Thanks for looking at JDK-8170114 and JDK-8170120.  I took a look at
http://cr.openjdk.java.net/~shade/8170114/webrev.01/

Yes, thanks for looking at these issues.
For `jimage info <not-a-jimage-file>` issue then jimage correctly fails (with a
non-zero status) but the IOException isn't converted into an error message as it
does with other errors. Yes, it would be good to fix that.
I think the `jimage extract --dir <existing-file> <jimage-file>` scenario needs
discussion. If <existing-file> is a non-directory file then jimage has to fail,
I don't expect disagreement on that. For the case where it is an existing
1. Extract into the existing directory (existing JDK 9 and JDK 10 behavior)
2. Fail if it's not empty (your patch)
3. Fail if it exists (Mandy's mail, the motivation being to keep it consistent
with jlink)
I view `jimage extract --dir <dir>` as being similar to `unzip -d <dir>` so I
don't think existing behavior (#1) is incorrect, the only issue is that it
silently overrides files whereas unzip will prompt before overriding (unless you
specify -o). The `jar` tool, and legacy `tar` tool side with `jimage` and are
happy to silently replace existing files.
What would you think about focusing on the override case instead of disallowing
extracting into an existing non-empty directory? I realize this is more work as
it means deciding on whether to prompt, warn or fail. It also means thinking
about the equivalent of unzip -o to allowing existing files be replaced.

Unzip prompts user for individual files and I'm not sure whether it's a good
option here. Maybe prompt user at the beginning that directory is not empty and
there is a risk that files there might be overwritten continue y/n?

[m***@oracle.com]

Post by Michal Vala

Post by Alan Bateman
...
I think the `jimage extract --dir <existing-file> <jimage-file>` scenario needs
discussion. If <existing-file> is a non-directory file then jimage has to fail,
I don't expect disagreement on that. For the case where it is an existing
1. Extract into the existing directory (existing JDK 9 and JDK 10 behavior)
2. Fail if it's not empty (your patch)
3. Fail if it exists (Mandy's mail, the motivation being to keep it consistent
with jlink)
I view `jimage extract --dir <dir>` as being similar to `unzip -d <dir>` so I
don't think existing behavior (#1) is incorrect, the only issue is that it
silently overrides files whereas unzip will prompt before overriding (unless you
specify -o). The `jar` tool, and legacy `tar` tool side with `jimage` and are
happy to silently replace existing files.
What would you think about focusing on the override case instead of disallowing
extracting into an existing non-empty directory? I realize this is more work as
it means deciding on whether to prompt, warn or fail. It also means thinking
about the equivalent of unzip -o to allowing existing files be replaced.

Unzip prompts user for individual files and I'm not sure whether it's a good
option here. Maybe prompt user at the beginning that directory is not empty and
there is a risk that files there might be overwritten continue y/n?

CLI tools that prompt the user are difficult to use in scripts,
so I advise against that.

jimage is a diagnostic tool meant for rare and specialized use.
I think its current behavior (extract into an existing directory)
is fine, and I can imagine use cases where that might actually be
desired.

- Mark

[Michal Vala]

Post by m***@oracle.com

Post by Michal Vala

Post by Alan Bateman
...
I think the `jimage extract --dir <existing-file> <jimage-file>` scenario needs
discussion. If <existing-file> is a non-directory file then jimage has to fail,
I don't expect disagreement on that. For the case where it is an existing
1. Extract into the existing directory (existing JDK 9 and JDK 10 behavior)
2. Fail if it's not empty (your patch)
3. Fail if it exists (Mandy's mail, the motivation being to keep it consistent
with jlink)
I view `jimage extract --dir <dir>` as being similar to `unzip -d <dir>` so I
don't think existing behavior (#1) is incorrect, the only issue is that it
silently overrides files whereas unzip will prompt before
overriding (unless you
specify -o). The `jar` tool, and legacy `tar` tool side with `jimage` and are
happy to silently replace existing files.
What would you think about focusing on the override case instead of disallowing
extracting into an existing non-empty directory? I realize this is more work as
it means deciding on whether to prompt, warn or fail. It also means thinking
about the equivalent of unzip -o to allowing existing files be replaced.

Unzip prompts user for individual files and I'm not sure whether it's a good
option here. Maybe prompt user at the beginning that directory is not empty and
there is a risk that files there might be overwritten continue y/n?

CLI tools that prompt the user are difficult to use in scripts,
so I advise against that.

Usability in scripts is a valid point, but I still think that silently
overwriting files is a bad practice. I can imagine better behavior
without prompting the user. Like error when non-empty dir and 'force-
overwrite' parameter. Or at least print a warning message that some
files were overwritten.

Post by m***@oracle.com
jimage is a diagnostic tool meant for rare and specialized use.
I think its current behavior (extract into an existing directory)
is fine, and I can imagine use cases where that might actually be
desired.
- Mark

[Michal Vala]

Post by mandy chung

Post by Michal Vala
Patch validates output directory before any jimage extracting happen. I've
moved validation to extra private method as it is few lines of code. I've also
added proper error message for case when output path is not a directory
(JImageTask.java#449).

Thanks for looking at JDK-8170114 and JDK-8170120.  I took a look at
http://cr.openjdk.java.net/~shade/8170114/webrev.01/
Alternatively,  jimage extract can behave as jlink and it fails if the specified
output directory exists including empty directory. It'd be easy to delete the
directory in the command-line before running jimage.
You extend JImageCliTest to handle the beforeTest method to be invoked before
running each test case.  Is that necessary?  The test itself is creating temp
file/directory for each test case.    I think it'd be good to update the test to
create a named file/dir under the scratch area as jtreg will take care of
cleaning the scratch area.

It's because of extract tests without specifying --dir (extract to cwd). AFAIK
you can't correctly change cwd in runtime, so calling a test method in context
of different cwd is not an option. Thus we need an empty scratch area for those.
Alternatively we can clean cwd explicitly just before these tests, instead of
before each test method.

Post by mandy chung
Mandy

[Michal Vala]

Hi,

Post by Michal Vala
Hi,
sending fix for jimage bug JDK-8170114[1].

as this was resolved with current behavior is correct[1], I've updated test case
according to this behavior.
Patch attached. This also greens JImageExtractTest so I've removed it from
ProblemList.

[1] -
https://bugs.openjdk.java.net/browse/JDK-8170114?focusedCommentId=14157816&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14157816

[Alan Bateman]

Post by Michal Vala
Hi,

Post by Michal Vala
Hi,
sending fix for jimage bug JDK-8170114[1].

as this was resolved with current behavior is correct[1], I've updated
test case according to this behavior.
Patch attached. This also greens JImageExtractTest so I've removed it
from ProblemList.

This looks okay to me. I've created JDK-8198380 to track/sponsor this.

-Alan