RFR (JAXP) JDK-8067170: Enable security manager on JAXP unit tests
Joe Wang
huizhe.wang at oracle.com
Fri Aug 5 17:59:01 UTC 2016
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.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