Integrated: 8233020: (fs) UnixFileSystemProvider should use StaticProperty.userDir().

[Jaikiran Pai]

On Fri, 2 Jul 2021 13:50:20 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review for this change which addresses https://bugs.openjdk.java.net/browse/JDK-8233020?
> 
> The commit in this PR updates `sun.nio.fs.UnixFileSystemProvider` to use the `jdk.internal.util.StaticProperty`, which was introduced in https://bugs.openjdk.java.net/browse/JDK-8066709, to get the value for the "user.dir" system property. Given the nature of this change, no new jtreg test has been added.
> 
> Alan notes in JDK-8233020:
> 
>> The changes in JDK-8066709 were okay to leave out the Unix file system provider because it was initialized very early in the startup at the time.
> 
> So, with input from Roger, I generated the classloading logs of a trivial helloworld application, before and after this change. I used `-verbose:class` to generate them.
> 
> 
> public class Test {
> 	public static void main(final String[] args) throws Exception {
> 		System.out.println("Hello world");
> 	}
> }
> 
> 
> The classloading logs showed that the order, both before and after the change, has the `jdk.internal.util.StaticProperty` loaded way before the `sun.nio.fs.UnixFileSystemProvider`. Given the way `jdk.internal.util.StaticProperty` class gets loaded in phase 1 of java.lang.System initialization[1], this classloading order also implies that `jdk.internal.util.StaticProperty` is initialized before any instance of `sun.nio.fs.UnixFileSystemProvider` gets created. I also checked a few versions of JDKs - 11, 15, 16 and even 8 (which seems to have this backported) and none of them show the `sun.nio.fs.UnixFileSystemProvider` loading before the `StaticProperty`. I wasn't able to build the version of a JDK (due to build tool incompatibilities with my local system) before the change in JDK-8066709 was introduced. So I don't have any version of JDK to compare where `UnixFileSystemProvider` was perhaps being loaded way too early to cause any issue. So in short, on the ordering front, IMO this c
 hange shouldn't cause any issues that I can think of. 
> 
> For the sake of reference, a snippet of the classloading logs before and after this change is provided at the end of this comment.
> 
> One other aspect to consider is the usage of "StaticProperty" itself. The javadoc of this class states:
> 
>>  <strong>{@link SecurityManager#checkPropertyAccess} is NOT checked
>>  in these access methods. The caller of these methods should take care to ensure
>>  that the returned property is not made accessible to untrusted code.</strong>
>> 
> 
> 
> In this specific case, the `sun.nio.fs.UnixFileSystemProvider` is a public class. It now uses this `StaticProperty` to get the "user.dir" system property value and passes it on to the `newFileSystem(String dir)` method of its sub-classes. However, the `newFileSystem` method that's being called has package-private access, so no application specific sub-classes or any code outside of the JDK can override it and get access to the passed "user.dir" system property value. So the absence of security manager check for the property access in this case won't pose an issue, IMO.
> 
> Following are the classloading logs:
> 
> Before:
> 
> 
> line 252: [0.031s][info][class,load] jdk.internal.util.StaticProperty source: shared objects file
> [0.031s][info][class,load] java.io.FileInputStream source: shared objects file
> [0.031s][info][class,load] java.io.FileDescriptor source: shared objects file
> [0.031s][info][class,load] jdk.internal.access.JavaIOFileDescriptorAccess source: shared objects file
> ....
> [0.074s][info][class,load] java.lang.module.ModuleReader source: shared objects file
> [0.074s][info][class,load] jdk.internal.module.SystemModuleFinders$SystemModuleReader source: shared objects file
> [0.074s][info][class,load] jdk.internal.module.ModulePatcher$PatchedModuleReader source: jrt:/java.base
> [0.074s][info][class,load] jdk.internal.module.SystemModuleFinders$SystemImage source: shared objects file
> [0.074s][info][class,load] jdk.internal.jimage.ImageReaderFactory source: shared objects file
> [0.074s][info][class,load] java.nio.file.Paths source: shared objects file
> [0.074s][info][class,load] java.nio.file.FileSystems source: shared objects file
> [0.074s][info][class,load] java.nio.file.FileSystems$DefaultFileSystemHolder source: shared objects file
> [0.074s][info][class,load] java.nio.file.FileSystems$DefaultFileSystemHolder$1 source: shared objects file
> [0.074s][info][class,load] sun.nio.fs.DefaultFileSystemProvider source: shared objects file
> [0.074s][info][class,load] java.nio.file.spi.FileSystemProvider source: shared objects file
> [0.074s][info][class,load] sun.nio.fs.AbstractFileSystemProvider source: shared objects file
> line 579: [0.074s][info][class,load] sun.nio.fs.UnixFileSystemProvider source: shared objects file
> 
> 
> After:
> 
> 
> line 252: [0.037s][info][class,load] jdk.internal.util.StaticProperty source: shared objects file
> [0.037s][info][class,load] java.io.FileInputStream source: shared objects file
> [0.037s][info][class,load] java.io.FileDescriptor source: shared objects file
> ...
> [0.081s][info][class,load] jdk.internal.module.ModulePatcher$PatchedModuleReader source: jrt:/java.base
> [0.081s][info][class,load] jdk.internal.module.SystemModuleFinders$SystemImage source: shared objects file
> [0.081s][info][class,load] jdk.internal.jimage.ImageReaderFactory source: shared objects file
> [0.081s][info][class,load] java.nio.file.Paths source: shared objects file
> [0.081s][info][class,load] java.nio.file.FileSystems source: shared objects file
> [0.081s][info][class,load] java.nio.file.FileSystems$DefaultFileSystemHolder source: shared objects file
> [0.081s][info][class,load] java.nio.file.FileSystems$DefaultFileSystemHolder$1 source: shared objects file
> [0.082s][info][class,load] sun.nio.fs.DefaultFileSystemProvider source: shared objects file
> [0.082s][info][class,load] java.nio.file.spi.FileSystemProvider source: shared objects file
> [0.082s][info][class,load] sun.nio.fs.AbstractFileSystemProvider source: shared objects file
> line 579: [0.082s][info][class,load] sun.nio.fs.UnixFileSystemProvider source: shared objects file
> 
> 
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/System.java#L2089

This pull request has now been integrated.

Changeset: fd4de1ed
Author:    Jaikiran Pai <jpai at openjdk.org>
URL:       https://git.openjdk.java.net/jdk/commit/fd4de1ed404640ee0e744f022bbfa89db200ef05
Stats:     4 lines in 1 file changed: 1 ins; 2 del; 1 mod

8233020: (fs) UnixFileSystemProvider should use StaticProperty.userDir().

Reviewed-by: alanb

-------------

PR: https://git.openjdk.java.net/jdk/pull/4668