-
Notifications
You must be signed in to change notification settings - Fork 61
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
8314260: Unable to load system libraries on Windows when using a SecurityManager #865
Conversation
👋 Welcome back pminborg! A progress list of the required criteria for merging this PR into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find!
@SuppressWarnings("removal") | ||
String systemRoot = AccessController.doPrivileged(new PrivilegedAction<String>() { | ||
@Override | ||
public String run() { | ||
return System.getenv("SystemRoot"); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use sun.security.action.GetPropertyAction.privilegedGetProperty
here for simplicity. See e.g. https://github.com/openjdk/panama-foreign/blob/foreign-memaccess%2Babi/src/java.base/share/classes/jdk/internal/foreign/CABI.java#L50 where it's used as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, that is for System properties. Here we're reading an environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe sun.security.action.GetEnvironmentPropertyAction::privilegedGetenv(String)
could be added for calling java.lang.System::getenv(String)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
sun.security.action.GetEnvironmentPropertyAction::privilegedGetenv(String)
could be added for callingjava.lang.System::getenv(String)
.
I am unable to find such a class/method:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean create such a class by copying what the other sun.security.action.Get*Action
classes do.
We will convert this PR into a PR on the mainline after the JEP PR is merged. |
Please close this PR if this is not meant to be integrated. |
These proposals will be targeted to the mainline and thus, I am closing this PR. |
This PR proposes to read the system environment variable "SystemRoot" using a privileged operation so it will work in the event a default
SecurityManager
is in place.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/panama-foreign.git pull/865/head:pull/865
$ git checkout pull/865
Update a local copy of the PR:
$ git checkout pull/865
$ git pull https://git.openjdk.org/panama-foreign.git pull/865/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 865
View PR using the GUI difftool:
$ git pr show -t 865
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/panama-foreign/pull/865.diff
Webrev
Link to Webrev Comment