Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup

Sebastian Sickelmann sebastian.sickelmann at
Tue Dec 6 04:35:00 UTC 2011

Sorry for the broken history.

I am answering to
Answering to
the mail and my predesecor is not received by my email-provider yet. 
Email is not the best
tool to transport discussion, but the only practically we actually have :-(.

   >Cleanup fallthrough in FilePermission and PropertyPermission was:
  Request for Review of >7116890 (Warning Cleanup

*>Eamonn McManus* eamonn at 
/>Mon Dec 5 14:19:10 PST 2011/

    * Previous message: Cleanup fallthrough in FilePermission and
      PropertyPermission was: Request for Review of 7116890 (Warning
    * Next message: 7116722: Miscellaneous warnings sun.misc ( and
      related classes )
    * *Messages sorted by:* [ date ]
      [ thread ]
      [ subject ]
      [ author ]


>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.
This is my main part i worry about. Performance in Permission must be
really good. Looking through the code i see thatMBeanPermission  and
ServicePermission are not using the String identity check that every
other "PermissionWithActions" is using.

I will try to extract the parsing to an
Reintegration it back into an abstract base class for the PermissionWithActions
classes should be not the biggest problem.

Thanks for the input.

-- Sebastian

>On 5 December 2011 07:45, Sebastian Sickelmann
><sebastian.sickelmann at  <>>  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, so 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
/>>>>>>/  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/
/>>>>>>/  2. java/net/
/>>>>>>/  3. java/util/
/>>>>>>/  4. javax/management/
/>>>>>>/  5. javax/security/auth/kerberos/
/>>>>>>/  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 also
/>>>>>>>>/  exists in
/>>>>>>>>/ 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 to
/>>>>>>>>/  Or, maybe you can apply the same changes yourself to

More information about the core-libs-dev mailing list