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

Frank Yuan frank.yuan at oracle.com
Thu Aug 4 10:05:39 UTC 2016 


> -----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/functional/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/functional/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/functional/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/functional/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/unittest/transform/XSLTFunctionsTest.java.frames.html
> >>>
> >>>
> >>>
> >>>   70
> >>> tf.setFeature("http://www.oracle.com/xml/jaxp/properties/enableExtensionFunctions",
> >>>
> >>> 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/libs/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