Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Jul 2, 2021

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 change 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:

{@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.

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8233020: (fs) UnixFileSystemProvider should use StaticProperty.userDir().

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4668/head:pull/4668
$ git checkout pull/4668

Update a local copy of the PR:
$ git checkout pull/4668
$ git pull https://git.openjdk.java.net/jdk pull/4668/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4668

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4668.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 2, 2021

👋 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 openjdk bot added the rfr label Jul 2, 2021
@openjdk
Copy link

openjdk bot commented Jul 2, 2021

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

  • nio

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

@openjdk openjdk bot added the nio label Jul 2, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 2, 2021

Webrevs

@openjdk
Copy link

openjdk bot commented Jul 3, 2021

@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:

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

Reviewed-by: alanb

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 12 new commits pushed to the master branch:

  • 17f53f2: Merge
  • 1c18f91: 8269768: JFR Terminology Refresh
  • 6f0e8e7: 8269775: compiler/codegen/ClearArrayTest.java failed with "assert(false) failed: bad AD file"
  • c4ea13e: 8269543: The warning for System::setSecurityManager should only appear once for each caller
  • 2db9005: 8262017: C2: assert(n != __null) failed: Bad immediate dominator info.
  • 7bc96db: 8269771: assert(tmp == _callprojs.fallthrough_catchproj) failed: allocation control projection
  • 5644c4f: 8265132: C2 compilation fails with assert "missing precedence edge"
  • 0d0f6a4: 8268664: The documentation of the Scanner.hasNextLine is incorrect
  • cb79589: 8188046: java.lang.Math.mutliplyHigh does not run in constant time
  • ca4bea4: 8188044: We need Math.unsignedMultiplyHigh
  • ... and 2 more: https://git.openjdk.java.net/jdk/compare/f8bcbf0172af25ac17b110d22232bd618cfd621a...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 openjdk bot added the ready label Jul 3, 2021
@jaikiran
Copy link
Member Author

jaikiran commented Jul 5, 2021

Thank you for the review, Alan.

@jaikiran
Copy link
Member Author

jaikiran commented Jul 5, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 5, 2021

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

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 5, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 5, 2021
@openjdk
Copy link

openjdk bot commented Jul 5, 2021

@jaikiran Pushed as commit fd4de1e.

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

@jaikiran jaikiran deleted the 8233020 branch Jul 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated nio
2 participants