Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

Joe Wang Fri, 05 Aug 2016 11:00:57 -0700

Hi Frank,

Looks good overall. Thanks for adding runs with/without SM, that's a lotof tedious work! I wish there's a way to do this in a generalconfiguration. But it's fine since it does serve the purpose.


Thanks,
Joe

On 8/5/16, 6:26 AM, Frank Yuan wrote:

Hi Joe and Daniel

Please review the update: http://cr.openjdk.java.net/~fyuan/8067170/webrev.04/
In this version, I
1. made all tests run twice, to pass in the different argument to the jtreg 
TestNG test, it has to run in othervm(jaxp test also run
in othervm before this but it's possible to run in agentvm), however, I didn't 
delete the code supporting agentvm from
JAXPPolicyManager.java because jtreg may provide more support in agentvm mode 
someday:)
2. enabled sm in unittest/catalog/CatalogTest.java as jdk-8161176's conclusion
3. fixed the bug in runWithTmpPermission methods, Daniel mentioned it but I 
didn't understand at that time :P

Thanks
Frank

-----Original Message-----
From: Frank Yuan [mailto:frank.y...@oracle.com]
Sent: Thursday, August 04, 2016 6:06 PM
To: 'Joe Wang'; 'Daniel Fuchs'
Cc: 'core-libs-dev'
Subject: RE: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

-----Original Message-----
From: Joe Wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP

unit tests



On 8/3/16, 3:45 AM, Daniel Fuchs wrote:

Hi Frank,

I looked at the diffs of the diffs between webrev.02 and webrev.03 and
it looks good to me.

+1 - provided all tests pass :-)

+1, thanks for re-enabling the tests that had dependencies on 8162848. I
also closed 8162848.

But I have the same question than Joe: aren't all the test supposed
to run twice: once with security manager and once without?
(except for those test that might explicitly require a security

manager)

I agree, all of the tests need to run with and without security manager.
It would be good to combine the  runWithSecurityManager and
runWithoutSecurityManager methods into one with a condition that
determines whether a security manager is present.

Alright, I will make the tests run twice. To impl this, I have to add
jtreg tags like the following:
/*
  * @test
  * @library /javax/xml/jaxp/libs /javax/xml/jaxp/unittest
  * @run testng/othervm -DrunSecMngr=true common.Bug6350682
  * @run testng/othervm common.Bug6350682
  */

And modify the Policy class accordingly. I am writing a small program to
update the tests, will send the new version tomorrow...

Frank

Best,
Joe

best regards,

-- daniel

On 03/08/16 11:12, Frank Yuan wrote:

Hi Joe

Thank you for your review!

I updated the tests according to the latest comments as well as had
unittest/transform/TransformerTest.java up to date, please check
http://cr.openjdk.java.net/~fyuan/8067170/webrev.03/


Thanks
Frank

-----Original Message-----
From: Joe Wang [mailto:huizhe.w...@oracle.com]
Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP
unit tests



On 8/2/16, 5:30 AM, Daniel Fuchs wrote:

Hi Frank,

I am intrigued by these change which do not seem
to have anything to do with the rest. I mean, I'm not opposed
as long as they are intended and that Joe validates them:
========================================================

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
nctional/org/xml/sax/ptests/XMLReaderNSTableTest.java.fram

es.html


118         spf.setNamespaceAware(false);

Not sure why this was changed.

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
nctional/org/xml/sax/xmlfiles/out/EntityResolverGF.out.fra

mes.html
The test itself wasn't very clear on the content it tests. If it only
verifies whether a resolver is used as is said, then this and related
changes in ResolverTest and publish.xml are fine. To the extent it
verifies, it didn't have to output to a file.

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
nctional/org/xml/sax/xmlfiles/out/NSTableFTGF.out.frames.h

tml
We have to let Frank explain why namespace was turned off for these
tests :-)  In this case, XMLReaderNSTableTest. In general, I would

say

enabling security shouldn't change tests or gold files in this case.

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/fu
nctional/org/xml/sax/xmlfiles/publish.xml.frames.html


This looks like a desirable change - but what does it have to do
with enabling security manager?

Probably to remove the reference to that particular server?  Seems to

be

fine as a cleanup. Again, it (the original test) only verifies a

resolve

is used, it didn't even need to write out a file to prove that.

Also:
========================================================

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/un
ittest/transform/XSLTFunctionsTest.java.frames.html

tf.setFeature("http://www.oracle.com/xml/jaxp/properties/enableExtensionFu
nctions",

true);

Is this needed only when there is a security manager?

Yes, when Security Manager is present, this feature is turned off by
default.

========================================================

http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/test/javax/xml/jaxp/li
bs/jaxp/library/JAXPPolicyManager.java.html




  119         /*
  120         addPermission(new RuntimePermission("getClassLoader"));
  121         addPermission(new

RuntimePermission("createClassLoader"));

  122         addPermission(new
RuntimePermission("createSecurityManager"));
  123         addPermission(new RuntimePermission("modifyThread"));
  124         addPermission(new PropertyPermission("*",

"read,write"));

  125
  126         addPermission(new RuntimePermission("setIO"));
  127         addPermission(new
RuntimePermission("setContextClassLoader"));
  128         addPermission(new
RuntimePermission("accessDeclaredMembers"));*/


Please remove before pushing.


========================================================

test/javax/xml/jaxp/internaltest/javax/xml/transform/cli/tigertest-in.xml

test/javax/xml/jaxp/internaltest/javax/xml/transform/cli/tigertest.xsl

It seems these two files have been removed, but shouldn't they have
been moved to test/javax/xml/jaxp/internaltest/javax/xml/transform
instead?
I see they are used by
test/javax/xml/jaxp/internaltest/javax/xml/transform/CLITest.java

Did you forget to hg add them?

========================================================


Otherwise the new JAXPPolicyManager&  its Policy implementation
look good. This is much simpler and better than the first
iteration :-)

This was a *very* long patch - so congratulations for seeing
this through!

Very long patch, indeed! Thanks for the great effort!  Very well done
with the unified Policy/Permission management.

Best,
Joe

cheers,

-- daniel


On 02/08/16 11:20, Frank Yuan wrote:

Hi Joe and Daniel

I have finished the rework as your comments, please check
http://cr.openjdk.java.net/~fyuan/8067170/webrev.02/

JAXP tests use Policy classes, as well as 3 other patterns provided
by JAXPTestUtilities:
1. runWithAllPerm methods, are only used for user setup code, never
run jaxp code with them.
2. tryRunWithPolicyManager method, is used to run some test methods
with security manager and run the others without security
manager in single test class
3. runWithTmpPermission methods, which may run jaxp code with some
limited permissions, those permissions belong to user permissions
and jaxp code won't doPrivileged for them. E.g.
PropertyPermission("MyInputFactory", "read") or
FilePermission("/tmp/this/does/not/exist/but/that/is/ok", "read")


Btw, some tests or test methods are disabled or commented sm

support

temporarily for some known jaxp security bugs.

Thanks
Frank

Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests

Reply via email to