Re: RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

> On Nov 27, 2016, at 7:13 PM, Wang Weijun  wrote:
> 
>> 
>> On Nov 27, 2016, at 6:12 PM, Alan Bateman  wrote:
>> 
>> On 26/11/2016 08:54, Wang Weijun wrote:
>> 
>>> Please take a review at
>>> 
>>>   http://cr.openjdk.java.net/~weijun/8170364/webrev.00/
>>> 
>>> The compatibility layer introduced in the new FilePermission implementation 
>>> requires one FilePermission to imply another with either a relative path or 
>>> an absolute path. This is solved with a private field npath2 inside. When 
>>> this field is set, the permission's name is modified a little (JDK-8168127) 
>>> so that when adding to a FilePermissionCollection, it is not confused with 
>>> one without the field. Unfortunately, this modified name is reused in the 
>>> merge to create a new "merged" FilePermission which contains a malformed 
>>> path now.
>>> 
>>> This fix creates a new non-public method to create this "merged" 
>>> FilePermission.
>>> 
>>> I checked other places and seems the name is not used to create a new 
>>> FilePermission.
>>> 
>> Ideally FilePermission (and all permissions) would have final fields, I hope 
>> that can be fixed some day. 
> 
> Me too, but we have that huge init() method now.
> 
>> In the mean-time then having withNewActions create a new FilePermission and 
>> then mutate it looks a bit ugly, can't you just introduce a new non-public 
>> constructor to create it with the effective mask?
> 
> The private "clone" constructor is now used in 3 places, all with a mutation 
> right after (lines 267, 280, 993). I've taken care that only newly created 
> objects are mutated this way. Creating 3 new constructors looks like too many 
> duplicated codes.

Do you still have a strong opinion?

> 
>> 
>> In passing then maybe the comment at L1063-1065 can be re-examined and it 
>> looks to be stale (the specific bootstrapping issue mentioned was fixed in 
>> 2015).
> 
> I can try.

Changed to lambda. Tests on -testset core most pass. 
java/lang/invoke/lambda/LogGeneratedClassesTest.java still fails but it already 
failed before my change.

> 
>> 
>> The test is one specific scenario and I wonder if you've considered beefing 
>> this up to other scenarios and other targets so that it's more broadly 
>> testing the merging scenarios.
> 
> I can add 2 more lines on checking the absolute path, and maybe with delete, 
> execute and readlink (1+1+1+1, 2+2, 3+1 etc). The merge is only about the 
> name and any name is the same, so it seems unnecessary to try other names.

Test enhanced. Permissions are granted in "read", "write", "delete", "execute", 
or "read,write", "delete,execute", or "read,write,delete", "execute", or 
"read,write,delete,execute", and 2^4-1 combinations of actions for both "x" and 
"/abs/x" checked.

If you are OK with the above, I'll post an updated webrev.

Thanks
Max

> 
> Thanks
> Max
> 
> 
>> 
>> -Alan

Re: Code Review Request JDK-8170329 New SSLSocket testing template

[Xuelei Fan]

On 11/27/2016 6:04 PM, Wang Weijun wrote:

This is not only a test update.

No, I happened to find an implementation issue with the new test, so fix 
it altogether.  The issue is that the simple validator 
(SimpleValidator.java) does not support SKID/AKID during cert path 
build.  If two trusted certs has the same subject,  the simple validator 
may not be able to find the right one.


Thanks,
Xuelei


On Nov 27, 2016, at 9:35 AM, Xuelei Fan  wrote:

Hi,

Please review this test update:

  http://cr.openjdk.java.net/~xuelei/8170329/webrev.00/

The new template (SSLSocketTemplate.java) could be used to avoid the 
anti-free-port issues.  By using sub-classes of it, the new one can simplify 
the general SSLSocket test code significantly.

Thanks,
Xuelei

Re: RFR 8170364: FilePermission path modified during merge

[Alan Bateman]

On 26/11/2016 08:54, Wang Weijun wrote:


Please take a review at

http://cr.openjdk.java.net/~weijun/8170364/webrev.00/

The compatibility layer introduced in the new FilePermission implementation requires one 
FilePermission to imply another with either a relative path or an absolute path. This is 
solved with a private field npath2 inside. When this field is set, the permission's name 
is modified a little (JDK-8168127) so that when adding to a FilePermissionCollection, it 
is not confused with one without the field. Unfortunately, this modified name is reused 
in the merge to create a new "merged" FilePermission which contains a malformed 
path now.

This fix creates a new non-public method to create this "merged" FilePermission.

I checked other places and seems the name is not used to create a new 
FilePermission.

Ideally FilePermission (and all permissions) would have final fields, I 
hope that can be fixed some day. In the mean-time then having 
withNewActions create a new FilePermission and then mutate it looks a 
bit ugly, can't you just introduce a new non-public constructor to 
create it with the effective mask?


In passing then maybe the comment at L1063-1065 can be re-examined and 
it looks to be stale (the specific bootstrapping issue mentioned was 
fixed in 2015).


The test is one specific scenario and I wonder if you've considered 
beefing this up to other scenarios and other targets so that it's more 
broadly testing the merging scenarios.


-Alan