RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

> On Nov 27, 2016, at 7:13 PM, Wang Weijun <weijun.wang at oracle.com> wrote:
> 
>> 
>> On Nov 27, 2016, at 6:12 PM, Alan Bateman <Alan.Bateman at oracle.com> 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