Cleanup fallthrough in FilePermission and PropertyPermission was: Request for Review of 7116890 (Warning Cleanup java.io)
Sebastian Sickelmann
sebastian.sickelmann at gmx.de
Tue Dec 6 04:35:00 UTC 2011
Sorry for the broken history.
I am answering to
Answering to
http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008582.html
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 java.io)
*>Eamonn McManus* eamonn at mcmanus.net
<mailto:core-libs-dev%40openjdk.java.net?Subject=Cleanup%20fallthrough%20in%20FilePermission%20and%20PropertyPermission%20was%3A%0A%09Request%20for%20Review%20of%207116890%20%28Warning%20Cleanup%20java.io%29&In-Reply-To=4EDCE716.1000404%40gmx.de>
/>Mon Dec 5 14:19:10 PST 2011/
* Previous message: Cleanup fallthrough in FilePermission and
PropertyPermission was: Request for Review of 7116890 (Warning
Cleanup java.io)
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008571.html>
* Next message: 7116722: Miscellaneous warnings sun.misc ( and
related classes )
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/008449.html>
* *Messages sorted by:* [ date ]
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/date.html#8582>
[ thread ]
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/thread.html#8582>
[ subject ]
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/subject.html#8582>
[ author ]
<http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-December/author.html#8582>
------------------------------------------------------------------------
>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 sun.security.PermissionParser.
Reintegration it back into an abstract base class for the PermissionWithActions
classes should be not the biggest problem.
Thanks for the input.
-- Sebastian
>
>Éamonn
>
>
>On 5 December 2011 07:45, Sebastian Sickelmann
><sebastian.sickelmann at gmx.de <http://mail.openjdk.java.net/mailman/listinfo/core-libs-dev>> 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