RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

> 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.

> 
> 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.

> 
> 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.

Thanks
Max


> 
> -Alan
> 
>