On 7/22/2016 5:53 AM, Daniel Fuchs wrote:
On 22/07/16 10:15, Frank Yuan wrote:
Hi Daniel
Thank you very much for your review and the comments!
-----Original Message-----
From: Daniel Fuchs [mailto:daniel.fu...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
unit tests
Hi Frank,
I see that in order to be able to run the tests, you were forced
to add a few permissions that the test/test infrastructure need
to setup things:
107 addPermission(new SecurityPermission("getPolicy"));
108 addPermission(new SecurityPermission("setPolicy"));
109 addPermission(new RuntimePermission("getClassLoader"));
110 addPermission(new
RuntimePermission("createClassLoader"));
111 addPermission(new
RuntimePermission("setSecurityManager"));
112 addPermission(new
RuntimePermission("createSecurityManager"));
113 addPermission(new RuntimePermission("modifyThread"));
114 addPermission(new PropertyPermission("*", "read,
write"));
115 addPermission(new
ReflectPermission("suppressAccessChecks"));
116 addPermission(new RuntimePermission("setIO"));
117 addPermission(new
RuntimePermission("setContextClassLoader"));
118 addPermission(new
RuntimePermission("accessDeclaredMembers"));
These are quite powerful permissions, and adding them by default
also means that you might miss a bug - if e.g. a doPrivileged is
missing somewhere in the JAXP code when jaxp tries to e.g. get/create
a classloader, or read a system property, you might not see
it.
Very true. Some of these permissions came from our standalone JAXP tests
that were granted to ant and junit.
Yes, I agree completely. I would control the permission assignment
more tightly:
1. only allow the following necessary permissions in default:
addPermission(new SecurityPermission("getPolicy"));
addPermission(new SecurityPermission("setPolicy"));
addPermission(new RuntimePermission("setSecurityManager"));
These are safe for the current code base. So may be add a note to remind
for any future changes.
2. other permissions in current default set are commonly used for the
tests, so I would add more Policy classes like existing
FilePolicy, etc.
//ClassLoaderPolicy, an individual test may only require some of
them, but I would put them in only one class if you agree
addPermission(new RuntimePermission("getClassLoader"));
addPermission(new RuntimePermission("createClassLoader"));
addPermission(new RuntimePermission("setContextClassLoader"));
// PropertyPolicy, there are various properties needed by the
tests, I would not classify them further...
addPermission(new PropertyPermission("*", "read, write"));
//Reflection permissions, jtreg may require them in deed, so I may
add them in default set
addPermission(new ReflectPermission("suppressAccessChecks"));
addPermission(new RuntimePermission("accessDeclaredMembers"));
//other permissions are used in less tests, I may use
tryRunWithTmpPermission instead of Policy class
addPermission(new RuntimePermission("setIO"));
addPermission(new RuntimePermission("createSecurityManager"));
addPermission(new RuntimePermission("modifyThread"));
How about you think?
My understanding is that you intend to grand specific permissions
limited to the test that will extend these policies, e.g. FilePolicy. I
think this is ok and an improvement.
It would definitely improve things - but then all the classes
in the test that runs with this new policy class will inherit
from these permissions as well. This may or may not be an issue.
(I confess I haven't looked at all the webrev ;-()
Yeah, it would be good to make sure a policy is safe with the code. If
both the test code and the code may need a permission, we may have a
conflict that we need to solve. It may be good to start with the basic
permission, and add only if required by the test code, with a note
preferably noting what exactly is needed. "DefaultFeaturesTest" in this
regard, doesn't seem to require FilePolicy, but
CatalogReferCircularityTest requires permission to where the source
files are located, in this case, it would be good to make it specific.
For example, instead of being called "FilePolicy", it may be clearer to
add a parameter so that it's known where the File permission is given
(e.g. the source dir in this case).
Best,
Joe
I had a similar issue when writing logging test, were I wanted
to temporarily disable permission checking in the middle of a test
to perform an infrastructure configuration.
So what I did is use an ThreadLocal<AtomicBoolean> to temporarily
disable permission checking - which allows me in my tests to do things
like:
boolean before = allowAll.get().get();
allowAll.get().set(true);
try {
do something that requires a permission
} finally {
allowAll.get().set(before);
}
JAXPTestUtilities.tryRunWithTmpPermission is similar with this, see
the example:
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/test/javax/xml/jaxp/unittest/stream/XMLEventReaderTest/Bug6555001.java.sdiff.htm
l
Yes that part looks fine.
cheers,
-- daniel
My implementation of Policy::implies also checks for
if (allowAll.get().get()) return true;
This allows me to control more tightly the set of permissions
I want my test to run under, while still being able to
perform any action I want to set up things without having
to give the same permission to all.
Hope this helps,
-- daniel
On 22/07/16 07:59, Frank Yuan wrote:
According to Amy's suggestion, re-generate a webrev
http://cr.openjdk.java.net/~fyuan/8067170/webrev.01/ as well as fix
some
issues,
please check.
Thanks
Frank
-----Original Message-----
From: Amy Lu [mailto:amy...@oracle.com]
Sent: Monday, July 18, 2016 5:42 PM
To: Frank Yuan; 'core-libs-dev'
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on
JAXP unit tests
On 7/18/16 5:32 PM, Frank Yuan wrote:
Btw, I moved internaltest into unittest because it's unnecessary
to separate them.
Maybe you'd like to regenerate the webrev with hg move for those
files?
Thanks,
Amy
