Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup java.io)
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
Thu Dec 15 04:19:39 UTC 2011
Hi,
just a short update. I created a first concept-implementations that works
and is really slow (roughly 1/3 of the performance of the actual
Implementation). I am actually reading through some micro-benchmark
suggestions and working at what can i do to make it fast without getting
to much implementation-readability-penalty. I think this patch isn't time
critical. So i want to use the time and dig a little bit deeper into
this on my
own.
I will discuss my results here, when I am ready with it.
Hopefully in the first quarter of 2012.
-- Sebastian
Am 05.12.2011 23:19, schrieb Eamonn McManus:
> To my mind, introducing a new shared parent means defining a new API
> that can be used by third-party subclasses. The point is that these
> permissions have an action list like "read,write,delete", with parsing
> code that converts this into a bitmask and toString() code that
> converts it back. I think that code is pretty hairy as it stands, all
> the more so because it is duplicated across the existing subclasses.
> For MBeanPermission, where there are 17 possible actions, the parsing
> method is 370 lines long! So refactoring is definitely desirable.
>
> An abstract PermissionWithActions class could use enums to good
> effect, something like this:
>
> public abstract class PermissionWithActions<E extends Enum<E>> {
> public abstract Class<E> actionClass();
> private final EnumSet<E> actions;
> protected PermissionWithAction(String target, String actions) {
> ...use reflection to get E[] values() and parse the actions...
> }
> public String getActions() {
> ...use toString().toLowerCase() on the actions set...
> }
> public final Set<E> getActionSet() {
> return Collections.unmodifiableSet(actions);
> }
> public boolean implies(Permission p) {
> if (p.getClass() != this.getClass()) {
> return false;
> }
> @SuppressWarnings("unchecked")
> PermissionWIthActions<E> pwa = (PermissionWithActions<E>) p;
> return this.actions.containsAll(pwa.actions);
> }
> }
> public final class FilePermission extends
> PermissionWithActions<FilePermissionActions> {
> @Override public Class<FilePermissionActions> actionClass() {
> return FliePermissionActions.class;
> }
> }
> public enum FilePermissionActions {READ, WRITE, EXECUTE, DELETE}
>
> However I'm not sure we actually want to make such an API change now.
> A safer path would be to refactor the parsing and getActions() code
> into a sun.* class which is not the parent class of anything, using
> delegation rather than inheritance. Safer both because it doesn't
> involve figuring out the impact on various JSRs, and because there is
> some danger that subclassing might introduce unforeseen attack
> vectors. FilePermission etc could each have one instance of this class
> that they use for parsing and getActions(), and that instance could
> construct e.g. a trie for parsing that is essentially as fast as the
> unmaintainable hand-coded parsers that each of the affected Permission
> subclasses has today. Parsing does have to be fast because when there
> is a security manager every checked operation will construct a
> Permission to check against.
>
> Éamonn
>
>
> On 5 December 2011 07:45, Sebastian Sickelmann
> <sebastian.sickelmann at gmx.de> wrote:
>> Am 05.12.2011 12:53, schrieb Weijun Wang:
>>
>>>
>>> On 12/03/2011 06:12 AM, Stuart Marks wrote:
>>>> I'm adding Weijun (Max) Wang to this thread.
>>>>
>>>> The same "ackbarfaccept" code had come up a third time when I was
>>>> reviewing some of Max's changes. The code in question all has to do with
>>>> permissions, and Max is in the security group, so he might have a better
>>>> insight whether doing a refactoring is appropriate and how to approach
>>>> doing it.
>>>
>>> Maybe we can group all permissions with a target and a predefined set of actions under a new abstract class.
>>
>> This sounds like the best idea from the implementors point of view. I would prepare a patch as suggestion for this.
>> Regarding to my own question i would say that there isn't any additional jigsaw related problem here by
>> introducing a common abstract class. They all rely on java.security.Permission, so java.security seems the correct
>> package for the new abstract class.
>>
>> Any suggestions for the name? PermissionWithActions???? AbstractPermission??
>>
>> If i see it right there is the following class hierarchy:
>>
>> j.s.Permission implements Serializable
>> +j.i.FilePermission
>> +j.n.SocketPermission
>> +jx.m.MBeanPermission
>> +jx.s.a.k.ServicePermission
>> +j.s.BasicPermission
>> +j.u.PropertyPermission
>>
>> What happens to serialization when the new hierarchy would look like this:
>>
>> j.s.Permission implements Serializable
>> +j.s.AbstractPermission
>> +j.i.FilePermission
>> +j.n.SocketPermission
>> +jx.m.MBeanPermission
>> +jx.s.a.k.ServicePermission
>> +j.s.BasicPermission
>> +j.u.PropertyPermission
>>
>> All the classes have a serialVerionUID so i think serialisation is no problem, or is there
>> a problem i i don't see here?
>>
>> -- Sebastian
>>
>>> -Max
>>>
>>>> Some searching around reveals that "ackbarfaccept" appears in the
>>>> PlayerPermission class of the Java ME Mobile Media API (not in any of
>>>> the OpenJDK repositories) so it looks like the code has been copied even
>>>> more times than are visible here. Perhaps a public permissions parsing
>>>> utility method is called for.
>>>>
>>>> s'marks
>>>>
>>>> On 12/2/11 9:02 AM, Sebastian Sickelmann wrote:
>>>>> Am 02.12.2011 16:27, schrieb Brandon Passanisi:
>>>>>> Hi Sebastian. I'm not sure if you had seen the e-mail from Stuart Marks
>>>>>> regarding this, but Stuart was able to find more instances of the
>>>>>> similar
>>>>>> block of "fallthrough" code. I can volunteer to apply your upcoming
>>>>>> change
>>>>>> to FilePermission to the other files if you wish. Or, you can try
>>>>>> applying
>>>>>> the change to the other files, but if you don't have time I don't
>>>>>> mind doing
>>>>>> it. Here's the section of Stuart's e-mail on this topic:
>>>>>>
>>>>>> -------------------------------------------------------------------------------------
>>>>>>
>>>>>> (Incidentally, this is the third time I've reviewed code today that
>>>>>> looks exactly like this. The other cases are in java.io.FilePermission
>>>>>> and java.util.PropertyPermission. They each have the /*FALLTHROUGH*/
>>>>>> into a set of cases that do nothing but break; and they have similar
>>>>>> ("ackbarfaccept") comments. It would be nice if these chunks of code
>>>>>> could be unified, but they differ in a number of fiddly details.)
>>>>>>
>>>>>> (The string "ackbarfaccept" occurs in the following files:
>>>>>> 1. java/io/FilePermission.java
>>>>>> 2. java/net/SocketPermission.java
>>>>>> 3. java/util/PropertyPermission.java
>>>>>> 4. javax/management/MBeanPermission.java
>>>>>> 5. javax/security/auth/kerberos/ServicePermission.java
>>>>>> Hmmm.)
>>>>>> -------------------------------------------------------------------------------------
>>>>>>
>>>>>>
>>>>>> Thanks.
>>>>>
>>>>> Hi Brandon,
>>>>> Hi Stuart,
>>>>>
>>>>> i had a look at all those classes and it seems to be the same
>>>>> algorithm. In an
>>>>> normal project (not the jdk) i would suggest to completely refactor it
>>>>> and make
>>>>> some code de-duplication cleanup. But if i thinks on projects like
>>>>> jigsaw this
>>>>> can easily get a real problem. What do you think, should we try to
>>>>> cleanup them
>>>>> all. Or should we try to make some de-duplication/code-reuse refactoring.
>>>>>
>>>>>
>>>>> -- Sebastian
>>>>>>
>>>>>> On 12/1/2011 10:18 PM, Sebastian Sickelmann wrote:
>>>>>>> Hi Brandon,
>>>>>>>
>>>>>>> i will try to work out a fix for both and cc the review request to you.
>>>>>>>
>>>>>>> -- Sebastian
>>>>>>>
>>>>>>> Am 01.12.2011 23:54, schrieb Brandon Passanisi:
>>>>>>>> Hi Sebastian. I was speaking with Stuart Marks earlier today and he
>>>>>>>> mentioned that the "fallthrough" code in FilePermission.java also
>>>>>>>> exists in
>>>>>>>> java.util.PropertyPermission.java. Maybe the code author had done some
>>>>>>>> copy/paste when working on these files. Stuart had said that you
>>>>>>>> might be
>>>>>>>> planning on doing some work on this after the warnings cleanup work.
>>>>>>>> If/when you do this work, maybe you can let me know so that I can
>>>>>>>> sync the
>>>>>>>> same changes you apply to FilePermission.java to
>>>>>>>> PropertyPermission.java?
>>>>>>>> Or, maybe you can apply the same changes yourself to
>>>>>>>> PropertyPermission.java?
>>
More information about the core-libs-dev
mailing list