On 4/25/2014 12:55 AM, Erik Joelsson wrote:
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.
Thanks for the review Erik. I'll make the change before the push.
Mandy
/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
