Review request: 8040059 Change default policy for extensions to no permission

[Erik Joelsson]

Hello Mandy,

The logic looks fine. Just some style issues. I would like indentation 
for the conditionals to be 2 spaces as is currently the standard in the 
makefiles. I would also like to have POLICY_SRC_LIST to be declared 
empty with := instead of just =. We only use = assignment when 
explicitly needed.

/Erik

On 2014-04-25 01:07, Mandy Chung wrote:
> Thanks Sean.
>
> I have updated the webrev:
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8040059/webrev.01/
>
> Erik - I'm including build-dev to review the build change for 
> java.policy file.
>
> Thanks
> Mandy
>
> On 4/24/14 11:32 AM, Sean Mullan wrote:
>> On 04/23/2014 05:29 PM, Mandy Chung wrote:
>>>
>>> On 4/23/2014 1:10 PM, Sean Mullan wrote:
>>>> Just a few comments:
>>>>
>>>> 1. When you write a test that uses the jtreg /policy option, the
>>>> policy file overrides the system policy file. If the test depends on a
>>>> standard extension, then you may get SecurityExceptions unless
>>>> additional perms are granted. Thus, there are quite a few tests that
>>>> define their own policy files and duplicate the grant statement for
>>>> extensions from the system policy:
>>>>
>>>>     grant codeBase "file:${{java.ext.dirs}}/*" {
>>>>         permission java.security.AllPermission;
>>>>     }
>>>>
>>>> These tests should be modified to only grant the necessary
>>>> permissions. However, ideally I think that a better solution would be
>>>> to add a jtreg /policy option that doesn't override the system policy
>>>> file, but rather appends to it, for example, using "==" :
>>>>
>>>
>>> I suspect most of the tests only want to grant the permissions for the
>>> test itself rather than overriding the system policy file. Having a new
>>> jtreg/policy option not to override the system policy file may be a
>>> better solution.    This would avoid updating the test's policy file
>>> every time the system's policy is modified.   On the other hand, I 
>>> think
>>> the test policy may not need to grant permissions to the extensions
>>> directory if it doesn't use the classes in extensions.  It's a good
>>> opportunity to clean that up. This will require some closer look at the
>>> tests.
>>>
>>> If you are okay, I'd like to separate the test's custom policy 
>>> update as
>>> a follow-on work.
>>
>> That's fine with me.
>>
>>>> @run main/othervm/policy==test.policy
>>>>
>>>> (this is the reverse behavior of the java.security.policy system
>>>> property, so it might be a little confusing, so maybe it is better to
>>>> add a new option)
>>>>
>>>
>>> "==" is a confusing syntax. -Djava.security.policy==test.policy
>>> overrides the system policy file ("=" prefix) whereas jtreg uses the
>>> reverse syntax? I think it would be better to make jtreg/policy
>>> consistent with -Djava.security.policy (i.e. default is to extend the
>>> system java.security).
>>
>> Would be nice, but not sure if we can change it at this point. 
>> Anyway, one of us should file a jtreg RFE.
>>
>>>> 3. jdk/nio/zipfs/ZipFileSystem.java
>>>>
>>>> If I understand the changes, the previous code would throw
>>>> SecurityExceptions when run under a SecurityManager? It's not
>>>> specifically related to this bug, is it?
>>>>
>>>
>>> It's a bug in the zipfs provider and I can file a new JBS issue for the
>>> zipfs change.  I prefer to push them in the same changeset that reduces
>>> zipfs's privileges and added tests to run with security manager.
>>
>> Sure, it is fine with me to include them with this. I just wanted to 
>> make sure I understood the changes.
>>
>> --Sean
>