RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Frank Yuan
frank.yuan at oracle.com
Fri Aug 5 13:26:54 UTC 2016
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.yuan at 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.wang at 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.wang at 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
> > >>>
> > >>>
> > >>>
> > >>> 70
> > >>>
> 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
> > >>>>
> > >>>
> > >>
> > >
More information about the core-libs-dev
mailing list