Hi Sean,
Thanks for the updates. Looks like the details are all there now. One minor
point:
87 * If a class name is specified, it must be {@code
java.lang.SecurityManager}
88 * or a public subclass and have a public no-arg constructor. The class is
89 * loaded by the {@linkplain ClassLoader#getSystemClassLoader()
90 * system class loader}.
The class is loaded by the system class loader, only if the class name is
something other than "java.lang.SecurityManager".
Editorial:
83 * the system property "{@code java.security.manager}" on the command line
to
Here and in several places, I don't think it's necessary to have the property
name both in code font and in quotation marks. I think the code font is sufficient.
Thanks,
s'marks
On 10/4/18 2:04 PM, Sean Mullan wrote:
Excellent comments, Stuart. Thanks for taking the time to review this.
I have posted another review that should address most of your comments, but also
responded inline with replies below.
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/
I also posted the javadoc so you can see what it looks like, esp. the table:
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/SecurityManager.html
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.02/docs/api/java.base/java/lang/System.html
On 10/3/18 7:11 PM, Stuart Marks wrote:
Hi Sean,
The new arrangement of using special tokens for java.security.manager makes a
lot more sense than having a separate property. Overall, the proposed
semantics seem reasonable to me.
I have some suggestions to clarify the proposed specification. (But also see
below.) From webrev.01:
81 * Environments using a security manager will typically set the security
82 * manager at startup. In the JDK implementation, this is done by setting
83 * the system property "{@code java.security.manager}" on the command
line to
84 * the class name of the security manager, or to the empty String ("")
85 * or "{@code default}" to utilize the default security manager.
86 * If the "{@code java.security.manager}" system property is not set, the
87 * default value is {@code null}, which means a security manager will
not be
88 * installed at startup.
I'd suggest using the term "special token" to describe the string "default"
here, to make it clear that this string is interpreted specially and is not
interpreted as a security manager class name. (The FilePermission docs use the
term "special token" to describe "<<ALL FILES>>".)
Similarly, I'd suggest "special token" be used to describe "allow" and
"disallow" below.
Good suggestion, I added "special token" to those places as well as to the
reference to "disallow" in System.setSecurityManager. I also broke up the first
sentence above on lines 81-85 in two sentences to make it easier to read.
93 * In the JDK implementation, if the Java virtual machine is started with
94 * the "{@code java.security.manager}" system property set to
95 * "{@code =disallow}" then the
96 * {@link System#setSecurityManager(SecurityManager) setSecurityManager}
97 * method cannot be used to set a security manager and will throw an
98 * {@code UnsupportedOperationException}. The ability to dynamically set
the
(I assume this will be changed from "=disallow" to "disallow" and similar for
"allow" below.)
Oops. Yes. I had fixed that but forgot to upload it in the webrev. Fixed now.
This should clarify that if "disallow" is used then no security manager will
be installed, in addition to preventing one from being installed in the future.
Yes, fixed.
98 * The ability to dynamically set
the
99 * security manager in a running system is an impediment to some
performance
100 * optimizations, even if the method is never called.
While I think this is true, it's a bit of rationale stuck into the middle of
the specification for the values of the system property, and as such it sticks
out. It kind of begs for more explanation. I'd suggest removing it.
I was on the fence about including that as well. I have removed it (and also
from the implNote in System.setSecurityManager). I think the JBS issue is the
best place to keep this type of information for now.
100 * If a security manager
is
101 * set at startup, or if the property is set to "{@code =allow}", then a
102 * security manager can be set dynamically.
I'd split this into two (or multiple) sentences, because there's actually a
lot going on here.
If the property is set to the special token "allow", no security manager is
installed at startup, but one can be set dynamically using the
setSecurityManager method. (Right?)
Correct.
I think there are more cases that need to be covered than just "allow". If the
property is set to "allow", the class name of a security manager, the empty
string "", or the special token "default", then the setSecurityManager()
method can be used to attempt to set the security manager dynamically.
However, an already-installed security manager may refuse this request. (Right?)
Correct. Initially I was a bit reluctant to document the behavior of
System.setSecurityManager for all the different property values. It also depends
on whether you are calling it for the first time or not. The only real
difference in behavior is if "disallow" is set. Otherwise, it works exactly as
the API is specified. But I can see how it can cause confusion since there are
many different options for java.security.manager now.
**
Whew, this is kind of a lot. The reason is that there are several different
values that the property can be set to, and they have an effect on the initial
SM that's set AND an effect on the behavior of calls to setSecurityManager()
at runtime. To get this straight, I wrote down a table:
Prop Value SM installed initially setSecurityManager() works
------------------------------------------------------------------------------
null none yes
empty string java.lang.SecurityManager maybe
"default" java.lang.SecurityManager maybe
"disallow" none no
"allow" none yes
a class name the named class maybe
Note: "maybe" means that setSecurityManager() will attempt to set the SM, in
that it won't throw UOE; however, the current SM might disallow it depending
upon its policies and the privilege of the caller.
From this table one can see that setting the property to the empty string and
to "default" have identical effects. Also, not setting the property (i.e., its
value is null) and setting it to "allow" have the same effects. Did I get this
right?
Yes, more or less. For the "yes" boxes, it is only if you assume it is the first
time it is called and null is not the value of SM.
Anyway, you can describe all of this in prose, but it has to be worded very
carefully in order to get all the details right. Or, you could put a table
directly into the spec. Or both! Up to you how you want to proceed.
I did both! For the table, my main issue was how to document the last column,
and whether to assume setSecurityManager was being called for the first time or
not. I assumed it was not, because this is really about static vs dynamic and
not about how many times an SM can be set.
**
An aside, possibly off topic. If the j.s.m property names a class that's to be
used as the security manager, presumably it must be a j.l.SecurityManager or a
subclass. Are there other requirements, such as having a public no-arg
constructor? Does the class need to be public, and does it need to be in
exported package or anything? Is the classpath or the module path searched,
and if the alternative SM is in a named module, is there a syntax for naming it?
I don't think the package needs to be exported - it looks like the code tries to
work around that by using reflection. It can be in a named or unnamed module.
I have kept it simple and added these 2 sentences: "If a class name is
specified, it must be java.lang.SecurityManager or a public subclass and have a
public no-arg constructor. The class is loaded by the system class loader." We
could revisit this later if it should be more precise.
--Sean
Sorry for all the impertinent questions; I can't find where this is
documented. Feel free to redirect.
s'marks
On 10/2/18 8:34 AM, Sean Mullan wrote:
Thanks for all the comments so far, and the interesting discussions about the
future of the SecurityManager. We will definitely return to those discussions
in the near future, but for now I have a second webrev ready for review for
this enhancement:
http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.01/
The main changes since the initial revision are:
1. System.setSecurityManager is no longer deprecated. This type of change
clearly needs more discussion and is not an essential part of this RFE.
2. After further thought, I took Bernd's suggestion [1] and instead of adding
a new property to disallow the setting of a SecurityManager at runtime, have
added new tokens to the existing "java.security.manager" system property,
named "=disallow", and "=allow" to toggle this behavior. The "=" is to avoid
any potential clashes with custom SM classes with those names. I think this
is a cleaner approach. There are a couple of new paragraphs in the
SecurityManager class description describing the "java.security.manager"
property and how the new tokens work.
3. I also added a comment that Bernd had requested [2] on why
System.setSecurityManager calls checkPackageAccess("java.lang").
Also, the CSR has been updated: https://bugs.openjdk.java.net/browse/JDK-8203316
Thanks,
Sean
[1]
http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018217.html
[2]
http://mail.openjdk.java.net/pipermail/security-dev/2018-September/018193.html
On 9/13/18 4:02 PM, Sean Mullan wrote:
This is a SecurityManager related change which warrants some additional
details for its motivation.
The current System.setSecurityManager() API allows a SecurityManager to be
set at run-time. However, because of this mutability, it incurs a
performance overhead even for applications that never call it and do not
enable a SecurityManager dynamically, which is probably the majority of
applications.
For example, there are lots of "SecurityManager sm =
System.getSecurityManager(); if (sm != null) ..." checks in the JDK. If it
was known that a SecurityManager could never be set at run-time, these
checks could be optimized using constant-folding.
There are essentially two main parts to this change:
1. Deprecation of System.securityManager()
Going forward, we want to discourage applications from calling
System.setSecurityManager(). Instead they should enable a SecurityManager
using the java.security.manager system property on the command-line.
2. A new JDK-specific system property to disallow the setting of the
security manager at run-time: jdk.allowSecurityManager
If set to false, it allows the run-time to optimize the code and improve
performance when it is known that an application will never run with a
SecurityManager. To support this behavior, the System.setSecurityManager()
API has been updated such that it can throw an UnsupportedOperationException
if it does not allow a security manager to be set dynamically.
webrev: http://cr.openjdk.java.net/~mullan/webrevs/8191053/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8203316
JBS: https://bugs.openjdk.java.net/browse/JDK-8191053
(I will likely also send this to core-libs for additional review later)
--Sean
