Re: RFR 8170364: FilePermission path modified during merge

[Alan Bateman]

On 28/11/2016 08:40, Wang Weijun wrote:

Hi Alan

Updated webrev at

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

Changes since webrev.00:

- a private constructor that can clones 4 fields and modifies 5 others

- using lambda

- test enhancement

This looks much better. A minor nit but the for readability purposes 
then it would be easy to read if the parameters to the constructor were 
all aligned rather than groups of two.


-Alan

Re: RFR 8170364: FilePermission path modified during merge

[Wang Weijun]

Hi Alan

Updated webrev at

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

Changes since webrev.00:

- a private constructor that can clones 4 fields and modifies 5 others

- using lambda

- test enhancement

Thanks
Max

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