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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

--
Michal Vala
OpenJDK QE
Red Hat Czech
Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman
On 09/02/2018 15:23, Michal Vala wrote:

> 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/
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
Reply | Threaded
Open this post in threaded view
|

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

Andrew Hughes
On 9 February 2018 at 15:38, Alan Bateman <[hidden email]> wrote:

> On 09/02/2018 15:23, Michal Vala wrote:
>>
>> 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/
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Michal Vala
In reply to this post by Alan Bateman
ah, accidentally replied just to Alan :/ Here it is


---------- Forwarded message ----------
From: Michal Vala <[hidden email]>
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 <[hidden email]>


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

On Fri, Feb 9, 2018 at 4:38 PM, Alan Bateman <[hidden email]> wrote:

> On 09/02/2018 15:23, Michal Vala wrote:
>>
>> 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/
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung
In reply to this post by Michal Vala
On 2/9/18 7:23 AM, Michal Vala wrote:
>
> 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

Reply | Threaded
Open this post in threaded view
|

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

Alan Bateman


On 10/02/2018 00:04, mandy chung wrote:

> On 2/9/18 7:23 AM, Michal Vala wrote:
>>
>> 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
Reply | Threaded
Open this post in threaded view
|

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

Michal Vala
In reply to this post by Mandy Chung


On 02/10/2018 01:04 AM, mandy chung wrote:

> On 2/9/18 7:23 AM, Michal Vala wrote:
>>
>> 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.

>
> Mandy
>
>

--
Michal Vala
OpenJDK QE
Red Hat Czech
Reply | Threaded
Open this post in threaded view
|

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

Michal Vala
In reply to this post by Alan Bateman


On 02/10/2018 11:25 AM, Alan Bateman wrote:

>
>
> On 10/02/2018 00:04, mandy chung wrote:
>> On 2/9/18 7:23 AM, Michal Vala wrote:
>>>
>>> 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.

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?


--
Michal Vala
OpenJDK QE
Red Hat Czech
Reply | Threaded
Open this post in threaded view
|

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

mark.reinhold
2018/2/11 23:10:46 -0800, Michal Vala <[hidden email]>:

> On 02/10/2018 11:25 AM, Alan Bateman wrote:
>> ...
>>
>> 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.
>
> 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
Reply | Threaded
Open this post in threaded view
|

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

Michal Vala
On Mon, 2018-02-12 at 09:39 -0800, [hidden email] wrote:

> 2018/2/11 23:10:46 -0800, Michal Vala <[hidden email]>:
> > On 02/10/2018 11:25 AM, Alan Bateman wrote:
> > > ...
> > >
> > > 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.
> >
> > 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.

>
> 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
OpenJDK QE
Red Hat Czech