Re: Review Request: JDK-8173096 jmod files are not world-readable

[Mandy Chung]

> On Jan 22, 2017, at 3:17 AM, Peter Levart  wrote:
> 
> Hi Mandy,
> 
> On 01/21/2017 03:32 AM, Mandy Chung wrote:
>> Updated webrev to put the temporary file in the same containing directory of 
>> the target file:
>>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01 
>> 
> That's ok. Maybe also specify ATOMIC_MOVE to CopyOption ... options in 
> Files.move() ? Currently the move will be atomic (rename in UNIX terminology) 
> because the source and target are in the same directory, but if renaming 
> fails for some reason, it will try the fallback to copy(src, dst) + 
> remove(src). If you add ATOMIC_MOVE to options, failure to rename will result 
> in exception. I don't know which one do you prefer?

I file https://bugs.openjdk.java.net/browse/JDK-8173280 


I prefer to see how jmod is used and any one runs into this issue that the 
atomic move and fallback to copy/remove may be needed.  The build could avoid 
the interference by creating a JMOD of the same name in a different directory.

Mandy

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Chris Hegarty]

On 21/01/17 02:32, Mandy Chung wrote:

Updated webrev to put the temporary file in the same containing directory of 
the target file:
  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01


Looks fine. Trivially, the test can now drop "-Djava.io.tmpdir=." .

-Chris.


I want to fix this regression in jdk-9+154.  We can revisit the approach after 
this fix if necessary.

Mandy


On Jan 20, 2017, at 8:55 AM, Mandy Chung  wrote:

Webrev:
  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html

A recent change in jmod tool [1] creates the JMOD file with no group
and other readable permission as that’s the default permission when
creating a temporary file with Files::createTempFile.  This fixes
the permission issue by creating the JMOD file in a temporary directory.

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

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Peter Levart]

Hi Mandy,

On 01/21/2017 03:32 AM, Mandy Chung wrote:

Updated webrev to put the temporary file in the same containing directory of 
the target file:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01


That's ok. Maybe also specify ATOMIC_MOVE to CopyOption ... options in 
Files.move() ? Currently the move will be atomic (rename in UNIX 
terminology) because the source and target are in the same directory, 
but if renaming fails for some reason, it will try the fallback to 
copy(src, dst) + remove(src). If you add ATOMIC_MOVE to options, failure 
to rename will result in exception. I don't know which one do you prefer?


Regards, Peter


I want to fix this regression in jdk-9+154.  We can revisit the approach after 
this fix if necessary.

Mandy


On Jan 20, 2017, at 8:55 AM, Mandy Chung  wrote:

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html

A recent change in jmod tool [1] creates the JMOD file with no group
and other readable permission as that’s the default permission when
creating a temporary file with Files::createTempFile.  This fixes
the permission issue by creating the JMOD file in a temporary directory.

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

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Mandy Chung]

> On Jan 21, 2017, at 5:08 AM, Alan Bateman  wrote:
> 
> On 21/01/2017 02:32, Mandy Chung wrote:
> 
>> Updated webrev to put the temporary file in the same containing directory of 
>> the target file:
>>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01
>> 
>> I want to fix this regression in jdk-9+154.  We can revisit the approach 
>> after this fix if necessary.
>> 
> This looks okay, I guess an alternative fix would be for CreateJmods.gmk to 
> set the permissions.

I prefer jmod tool to create a file with the proper permission in this fix.  
Thanks

> There are a couple of issues that have come up here and I agree that we 
> should re-visit once this regression is out of the way.

Thanks
Mandy

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Alan Bateman]

On 21/01/2017 02:32, Mandy Chung wrote:


Updated webrev to put the temporary file in the same containing directory of 
the target file:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01

I want to fix this regression in jdk-9+154.  We can revisit the approach after 
this fix if necessary.

This looks okay, I guess an alternative fix would be for CreateJmods.gmk 
to set the permissions. There are a couple of issues that have come up 
here and I agree that we should re-visit once this regression is out of 
the way.


-Alan

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Mandy Chung]

Updated webrev to put the temporary file in the same containing directory of 
the target file:
  http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.01

I want to fix this regression in jdk-9+154.  We can revisit the approach after 
this fix if necessary.

Mandy

> On Jan 20, 2017, at 8:55 AM, Mandy Chung  wrote:
> 
> Webrev:
>   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html
> 
> A recent change in jmod tool [1] creates the JMOD file with no group
> and other readable permission as that’s the default permission when
> creating a temporary file with Files::createTempFile.  This fixes
> the permission issue by creating the JMOD file in a temporary directory.
> 
> Mandy
> [1] https://bugs.openjdk.java.net/browse/JDK-8173096

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Mandy Chung]

> On Jan 20, 2017, at 1:22 PM, Alan Bateman  wrote:
> 
> Mandy - a while back then Erik changed the build so that JMOD files are 
> created in JMODS_TEMPDIR rather than JMODS_DIR. I think this was to reduce 
> interference with concurrent execution of the `jmod` tool. This might mean 
> you can avoid using /tmp.

The issue is jmod —-hash-modules resolves a module graph from a given module 
path which is the containing directory of the JMOD file being created:

$ jmod create -—module-path dir —-hash-modules “.*” dir/m.jmod

It seems one solution is to have the temporary file name be a hidden file.
Mandy

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Alan Bateman]

On 20/01/2017 20:37, Peter Levart wrote:



I guess the JmodTask is creating a temp file because it wants the file 
to not be observed by other processes while it is being written to, 
right? At the end when the tempTarget is fully written, it is renamed 
to its intended name with:


Files.move(tempTarget, target);


...but that operation will not be atomic since you didn't pass 
ATOMIC_MOVE to the CopyOption... options. Even if you did, the atomic 
move will succeed only when both tempTarget and target point to the 
same filesystem. Since you are creating tempTarget in default 
temporary files location (/tmp on UNIXes) this might not be the case 
(depending on where the real target is located).


So why don't you simply append a .tmp suffix to the intended target 
file name when opening the JmodOutputStream. This would ensure the 
file location will be on the same filesystem as the final target name 
as it will be created in the same directory. Module path scanning 
should skip files with .tmp extension then.
Module path scanning doesn't skip unrecognized files, this is because it 
allows for implementation specific packaging formats.


Mandy - a while back then Erik changed the build so that JMOD files are 
created in JMODS_TEMPDIR rather than JMODS_DIR. I think this was to 
reduce interference with concurrent execution of the `jmod` tool. This 
might mean you can avoid using /tmp.


-Alan.

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Mandy Chung]

> On Jan 20, 2017, at 12:37 PM, Peter Levart  wrote:
> 
> I guess the JmodTask is creating a temp file because it wants the file to not 
> be observed by other processes while it is being written to, right? At the 
> end when the tempTarget is fully written, it is renamed to its intended name 
> with:
> 
> Files.move(tempTarget, target);
> 
> 
> ...but that operation will not be atomic since you didn't pass ATOMIC_MOVE to 
> the CopyOption... options. Even if you did, the atomic move will succeed only 
> when both tempTarget and target point to the same filesystem. Since you are 
> creating tempTarget in default temporary files location (/tmp on UNIXes) this 
> might not be the case (depending on where the real target is located).
> 
> So why don't you simply append a .tmp suffix to the intended target file name 
> when opening the JmodOutputStream. This would ensure the file location will 
> be on the same filesystem as the final target name as it will be created in 
> the same directory. Module path scanning should skip files with .tmp 
> extension then.

Module path only ignores files starts with “.” or is hidden.   This says making 
 the temp file starting with “.” is a possibility.

$ java -p mods -m hi
Error occurred during initialization of VM
java.lang.module.ResolutionException: Unrecognized module: mods/abc.tmp

Mandy

Re: Review Request: JDK-8173096 jmod files are not world-readable

[Peter Levart]

Hi Mandy,

On 01/20/2017 05:55 PM, Mandy Chung wrote:

Webrev:
http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html

A recent change in jmod tool [1] creates the JMOD file with no group
and other readable permission as that’s the default permission when
creating a temporary file with Files::createTempFile.  This fixes
the permission issue by creating the JMOD file in a temporary directory.

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


I guess the JmodTask is creating a temp file because it wants the file 
to not be observed by other processes while it is being written to, 
right? At the end when the tempTarget is fully written, it is renamed to 
its intended name with:


Files.move(tempTarget, target);


...but that operation will not be atomic since you didn't pass 
ATOMIC_MOVE to the CopyOption... options. Even if you did, the atomic 
move will succeed only when both tempTarget and target point to the same 
filesystem. Since you are creating tempTarget in default temporary files 
location (/tmp on UNIXes) this might not be the case (depending on where 
the real target is located).


So why don't you simply append a .tmp suffix to the intended target file 
name when opening the JmodOutputStream. This would ensure the file 
location will be on the same filesystem as the final target name as it 
will be created in the same directory. Module path scanning should skip 
files with .tmp extension then.


Regards, Peter

Review Request: JDK-8173096 jmod files are not world-readable

[Mandy Chung]

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8173096/webrev.00/index.html

A recent change in jmod tool [1] creates the JMOD file with no group
and other readable permission as that’s the default permission when
creating a temporary file with Files::createTempFile.  This fixes
the permission issue by creating the JMOD file in a temporary directory.

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