RFR 8170364: FilePermission path modified during merge (original) (raw)

Wang Weijun weijun.wang at oracle.com
Sun Nov 27 11:13:52 UTC 2016

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

More information about the core-libs-dev mailing list