bval-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rmannibu...@apache.org
Subject [bval] branch master updated: [BVAL-176] Remove resetting of accessible flag when security manager is present
Date Tue, 04 Jun 2019 14:13:20 GMT
This is an automated email from the ASF dual-hosted git repository.

rmannibucau pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bval.git


The following commit(s) were added to refs/heads/master by this push:
     new bee61ea  [BVAL-176] Remove resetting of accessible flag when security manager is
present
bee61ea is described below

commit bee61eadba5335021a5be6c49c0b922630941c31
Author: Tomasz Wysocki <t.wysocki@oberthur.com>
AuthorDate: Mon Jun 3 12:19:25 2019 +0200

    [BVAL-176] Remove resetting of accessible flag when security manager is present
    
    This feature will not work without some synchronization on the
    reflection data itself in multithreaded environment.
    
    Therefore the feature has been removed due to following concerns:
    
    1. resetting accessible flag for security manager does not mean that for
    short period of time the flag is not actually set and bad code could
    exploit that - therefore resetting accesible back is not really making
    private/protected members unaccessible to other code. This is a design flaw in
    reflection , if accessible flag would be per thread it would work much better.
    
    2. since accessible flag is global it would require synchronization to make it work correctly,
    which is costly. Current implementation just breaks for SM present case
    - it throws 'inaccessible' exceptions since it does not synchronize at
    all.
    
    3. there is no saying what would need to be synchronized (probably the
    field or method reflected instances but it is not specified). Therefore
    synchronizing it would work only within scope of a single framework
    (bval).
    
    4. other frameworks typically don't reset back accessible and just keep
    the flag set. Therefore any synchronization mechanism specific to bval would not cooperate
    nicely or at all with other frameworks (like spring for instance).
---
 .../bval/jsr/ConstraintAnnotationAttributes.java   |  6 +----
 .../java/org/apache/bval/jsr/ValidatorImpl.java    |  6 +----
 .../org/apache/bval/jsr/descriptor/PropertyD.java  | 12 ++-------
 .../bval/jsr/util/AnnotationProxyBuilder.java      | 11 ++------
 .../apache/bval/jsr/util/AnnotationsManager.java   |  6 +----
 .../apache/bval/util/reflection/Reflection.java    | 30 ++++++----------------
 6 files changed, 15 insertions(+), 56 deletions(-)

diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/ConstraintAnnotationAttributes.java
b/bval-jsr/src/main/java/org/apache/bval/jsr/ConstraintAnnotationAttributes.java
index 6c285d5..ecc1ef7 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/ConstraintAnnotationAttributes.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/ConstraintAnnotationAttributes.java
@@ -227,15 +227,11 @@ public enum ConstraintAnnotationAttributes {
         }
 
         private Object doInvoke(final Annotation constraint) {
-            final boolean unset = Reflection.setAccessible(method, true);
+            Reflection.makeAccessible(method);
             try {
                 return method.invoke(constraint);
             } catch (Exception e) {
                 throw new RuntimeException(e);
-            } finally {
-                if (unset) {
-                    Reflection.setAccessible(method, false);
-                }
             }
         }
     }
diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/ValidatorImpl.java b/bval-jsr/src/main/java/org/apache/bval/jsr/ValidatorImpl.java
index 606e191..3d095cf 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/ValidatorImpl.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/ValidatorImpl.java
@@ -127,15 +127,11 @@ public class ValidatorImpl implements CascadingPropertyValidator, ExecutableVali
         if (cons == null) {
             throw new ValidationException("Cannot instantiate " + cls);
         }
-        final boolean mustUnset = Reflection.setAccessible(cons, true);
+        Reflection.makeAccessible(cons);
         try {
             return cons.newInstance(validatorContext);
         } catch (final Exception ex) {
             throw new ValidationException("Cannot instantiate " + cls, ex);
-        } finally {
-            if (mustUnset) {
-                Reflection.setAccessible(cons, false);
-            }
         }
     }
 }
diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java
index 0b1a285..267d3de 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/descriptor/PropertyD.java
@@ -49,15 +49,11 @@ public abstract class PropertyD<E extends AnnotatedElement> extends
CascadableCo
 
         @Override
         public Object getValue(Object parent) throws Exception {
-            final boolean mustUnset = Reflection.setAccessible(getTarget(), true);
+            Reflection.makeAccessible(getTarget());
             try {
                 return getTarget().get(parent);
             } catch (IllegalAccessException e) {
                 throw new IllegalArgumentException(e);
-            } finally {
-                if (mustUnset) {
-                    Reflection.setAccessible(getTarget(), false);
-                }
             }
         }
     }
@@ -75,15 +71,11 @@ public abstract class PropertyD<E extends AnnotatedElement> extends
CascadableCo
 
         @Override
         public Object getValue(Object parent) throws Exception {
-            final boolean mustUnset = Reflection.setAccessible(getTarget(), true);
+            Reflection.makeAccessible(getTarget());
             try {
                 return getTarget().invoke(parent);
             } catch (IllegalAccessException | InvocationTargetException e) {
                 throw new IllegalArgumentException(e);
-            } finally {
-                if (mustUnset) {
-                    Reflection.setAccessible(getTarget(), false);
-                }
             }
         }
     }
diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationProxyBuilder.java b/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationProxyBuilder.java
index 6d21098..a7c376e 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationProxyBuilder.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationProxyBuilder.java
@@ -192,15 +192,8 @@ public final class AnnotationProxyBuilder<A extends Annotation>
{
     private A doCreateAnnotation(final Class<A> proxyClass, final InvocationHandler
handler) {
         try {
             final Constructor<A> constructor = proxyClass.getConstructor(InvocationHandler.class);
-            final boolean mustUnset = Reflection.setAccessible(constructor, true); // java
-                                                                                   // 8
-            try {
-                return constructor.newInstance(handler);
-            } finally {
-                if (mustUnset) {
-                    Reflection.setAccessible(constructor, false);
-                }
-            }
+            Reflection.makeAccessible(constructor); // java 8
+            return constructor.newInstance(handler);
         } catch (Exception e) {
             throw new ValidationException("Unable to create annotation for configured constraint",
e);
         }
diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationsManager.java b/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationsManager.java
index 39ca237..e1d47b5 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationsManager.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/util/AnnotationsManager.java
@@ -209,15 +209,11 @@ public class AnnotationsManager {
 
         Stream.of(Reflection.getDeclaredMethods(a.annotationType())).filter(m -> m.getParameterCount()
== 0)
             .forEach(m -> {
-                final boolean mustUnset = Reflection.setAccessible(m, true);
+                Reflection.makeAccessible(m);
                 try {
                     result.get().put(m.getName(), m.invoke(a));
                 } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException
e) {
                     Exceptions.raise(ValidationException::new, e, "Caught exception reading
attributes of %s", a);
-                } finally {
-                    if (mustUnset) {
-                        Reflection.setAccessible(m, false);
-                    }
                 }
             });
         return result.optional().map(Collections::unmodifiableMap).orElseGet(Collections::emptyMap);
diff --git a/bval-jsr/src/main/java/org/apache/bval/util/reflection/Reflection.java b/bval-jsr/src/main/java/org/apache/bval/util/reflection/Reflection.java
index e8086b7..8c10c7d 100644
--- a/bval-jsr/src/main/java/org/apache/bval/util/reflection/Reflection.java
+++ b/bval-jsr/src/main/java/org/apache/bval/util/reflection/Reflection.java
@@ -209,14 +209,8 @@ public class Reflection {
             // do nothing
             return null;
         }
-        final boolean mustUnset = setAccessible(valueMethod, true);
-        try {
-            return valueMethod.invoke(annotation);
-        } finally {
-            if (mustUnset) {
-                setAccessible(valueMethod, false);
-            }
-        }
+        makeAccessible(valueMethod);
+        return valueMethod.invoke(annotation);
     }
 
     /**
@@ -434,28 +428,20 @@ public class Reflection {
     }
 
     /**
-     * Set the accessibility of {@code o} to {@code accessible}. If running without a {@link
SecurityManager}
-     * and {@code accessible == false}, this call is ignored (because any code could reflectively
make any
-     * object accessible at any time).
+     * Set the accessibility of {@code o} to true.
      * @param o
-     * @param accessible
-     * @return whether a change was made.
      */
-    public static boolean setAccessible(final AccessibleObject o, boolean accessible) {
-        if (o == null || o.isAccessible() == accessible) {
-            return false;
-        }
-        if (!accessible && System.getSecurityManager() == null) {
-            return false;
+    public static void makeAccessible(final AccessibleObject o) {
+        if (o == null || o.isAccessible()) {
+        	return;
         }
         final Member m = (Member) o;
 
         // For public members whose declaring classes are public, we need do nothing:
         if (Modifier.isPublic(m.getModifiers()) && Modifier.isPublic(m.getDeclaringClass().getModifiers()))
{
-            return false;
+            return;
         }
-        o.setAccessible(accessible);
-        return true;
+        o.setAccessible(true);
     }
 
     /**


Mime
View raw message