Thank you! I pushed the change into jaxp repo. Frank
> -----Original Message----- > From: Joe Wang [mailto:huizhe.w...@oracle.com] > Sent: Saturday, August 06, 2016 1:59 AM > To: Frank Yuan > Cc: 'Daniel Fuchs'; 'core-libs-dev' > Subject: Re: RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit > tests > > Hi Frank, > > Looks good overall. Thanks for adding runs with/without SM, that's a lot > of tedious work! I wish there's a way to do this in a general > configuration. 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 > >>>>>> > >>>>>> > >>>>>> 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 > >>>>>>>
