bval-commits mailing list archives

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

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


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

commit df96952b844c0c09e65e0c69ac874f2fba928397
Author: Tomasz Wysocki <t.wysocki@oberthur.com>
AuthorDate: Sun Jun 2 23:49:00 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
    it unaccessible to other code - this is design flaw. 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).
---
 .../java/org/apache/bval/util/FieldAccess.java     |  6 +---
 .../java/org/apache/bval/util/MethodAccess.java    |  6 +---
 .../java/org/apache/bval/util/PropertyAccess.java  | 20 +++----------
 .../apache/bval/util/reflection/Reflection.java    | 33 +++++++---------------
 .../java/org/apache/bval/jsr/ClassValidator.java   |  6 +---
 .../bval/jsr/ConstraintAnnotationAttributes.java   |  6 +---
 .../bval/jsr/xml/AnnotationProxyBuilder.java       |  8 ++----
 7 files changed, 20 insertions(+), 65 deletions(-)

diff --git a/bval-core/src/main/java/org/apache/bval/util/FieldAccess.java b/bval-core/src/main/java/org/apache/bval/util/FieldAccess.java
index 720d1db..04fd9d1 100644
--- a/bval-core/src/main/java/org/apache/bval/util/FieldAccess.java
+++ b/bval-core/src/main/java/org/apache/bval/util/FieldAccess.java
@@ -45,15 +45,11 @@ public class FieldAccess extends AccessStrategy {
      */
     @Override
     public Object get(final Object instance) {
-        final boolean mustUnset = Reflection.setAccessible(field, true);
+        Reflection.makeAccessible(field);
         try {
             return field.get(instance);
         } catch (IllegalAccessException e) {
             throw new IllegalArgumentException(e);
-        } finally {
-            if (mustUnset) {
-                Reflection.setAccessible(field, false);
-            }
         }
     }
 
diff --git a/bval-core/src/main/java/org/apache/bval/util/MethodAccess.java b/bval-core/src/main/java/org/apache/bval/util/MethodAccess.java
index 298272e..604723d 100644
--- a/bval-core/src/main/java/org/apache/bval/util/MethodAccess.java
+++ b/bval-core/src/main/java/org/apache/bval/util/MethodAccess.java
@@ -86,17 +86,13 @@ public class MethodAccess extends AccessStrategy {
      */
     @Override
     public Object get(final Object instance) {
-        final boolean mustUnset = Reflection.setAccessible(method, true);
+        Reflection.makeAccessible(method);
         try {
             return method.invoke(instance);
         } catch (IllegalAccessException e) {
             throw new IllegalArgumentException(e);
         } catch (InvocationTargetException e) {
             throw new IllegalArgumentException(e);
-        } finally {
-            if (mustUnset) {
-                Reflection.setAccessible(method, false);
-            }
         }
     }
 
diff --git a/bval-core/src/main/java/org/apache/bval/util/PropertyAccess.java b/bval-core/src/main/java/org/apache/bval/util/PropertyAccess.java
index 481df3c..d846d3c 100644
--- a/bval-core/src/main/java/org/apache/bval/util/PropertyAccess.java
+++ b/bval-core/src/main/java/org/apache/bval/util/PropertyAccess.java
@@ -132,14 +132,8 @@ public class PropertyAccess extends AccessStrategy {
         if (readMethod == null) {
             throw new NoSuchMethodException(toString());
         }
-        final boolean unset = Reflection.setAccessible(readMethod, true);
-        try {
-            return readMethod.invoke(bean);
-        } finally {
-            if (unset) {
-                Reflection.setAccessible(readMethod, false);
-            }
-        }
+        Reflection.makeAccessible(readMethod);
+        return readMethod.invoke(bean);
     }
 
     /**
@@ -231,14 +225,8 @@ public class PropertyAccess extends AccessStrategy {
     }
 
     private static Object readField(Field field, Object bean) throws IllegalAccessException
{
-        final boolean mustUnset = Reflection.setAccessible(field, true);
-        try {
-            return field.get(bean);
-        } finally {
-            if (mustUnset) {
-                Reflection.setAccessible(field, false);
-            }
-        }
+    	Reflection.makeAccessible(field);
+    	return field.get(bean);
     }
 
     /**
diff --git a/bval-core/src/main/java/org/apache/bval/util/reflection/Reflection.java b/bval-core/src/main/java/org/apache/bval/util/reflection/Reflection.java
index 674cf94..70aaa26 100644
--- a/bval-core/src/main/java/org/apache/bval/util/reflection/Reflection.java
+++ b/bval-core/src/main/java/org/apache/bval/util/reflection/Reflection.java
@@ -109,14 +109,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);
     }
 
     /**
@@ -309,28 +303,21 @@ 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);
     }
-
+   
+    
 }
diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/ClassValidator.java b/bval-jsr/src/main/java/org/apache/bval/jsr/ClassValidator.java
index d7b4640..eab0b82 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/ClassValidator.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/ClassValidator.java
@@ -316,15 +316,11 @@ public class ClassValidator implements CascadingPropertyValidator, ExecutableVal
         if (cons == null) {
             throw new ValidationException("Cannot instantiate " + cls);
         }
-        final boolean mustUnset = Reflection.setAccessible(cons, true);
+        Reflection.makeAccessible(cons);
         try {
             return cons.newInstance(factoryContext);
         } 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/ConstraintAnnotationAttributes.java
b/bval-jsr/src/main/java/org/apache/bval/jsr/ConstraintAnnotationAttributes.java
index 24b38ea..edca1cd 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
@@ -207,15 +207,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/xml/AnnotationProxyBuilder.java b/bval-jsr/src/main/java/org/apache/bval/jsr/xml/AnnotationProxyBuilder.java
index dedfabc..4b88eb0 100644
--- a/bval-jsr/src/main/java/org/apache/bval/jsr/xml/AnnotationProxyBuilder.java
+++ b/bval-jsr/src/main/java/org/apache/bval/jsr/xml/AnnotationProxyBuilder.java
@@ -100,16 +100,12 @@ public final class AnnotationProxyBuilder<A extends Annotation>
{
         this((Class<A>) annot.annotationType());
         // Obtain the "elements" of the annotation
         for (Method m : methods) {
-            final boolean mustUnset = Reflection.setAccessible(m, true);
+            Reflection.makeAccessible(m);
             try {
                 Object value = m.invoke(annot);
                 this.elements.put(m.getName(), value);
             } catch (Exception e) {
                 throw new ValidationException("Cannot access annotation " + annot + " element:
" + m.getName(), e);
-            } finally {
-                if (mustUnset) {
-                    Reflection.setAccessible(m, false);
-                }
             }
         }
     }
@@ -211,7 +207,7 @@ public final class AnnotationProxyBuilder<A extends Annotation>
{
     private A doCreateAnnotation(final Class<A> proxyClass, final InvocationHandler
handler) {
         try {
             Constructor<A> constructor = proxyClass.getConstructor(InvocationHandler.class);
-            Reflection.setAccessible(constructor, true); // java 8
+            Reflection.makeAccessible(constructor); // java 8
             return constructor.newInstance(handler);
         } catch (Exception e) {
             throw new ValidationException("Unable to create annotation for configured constraint",
e);


Mime
View raw message