Author: peter_firmstone Date: Mon Dec 26 11:59:41 2011 New Revision: 1224720
URL: http://svn.apache.org/viewvc?rev=1224720&view=rev Log: River-323 Refactoring as part of a process to fix the following falling tests: com/sun/jini/test/spec/lookupservice/test_set00/NotifyOnAttrAdd.td com/sun/jini/test/spec/lookupservice/test_set00/NotifyOnAttrDel.td com/sun/jini/test/spec/lookupservice/test_set00/NotifyOnAttrMod.td com/sun/jini/test/spec/lookupservice/test_set02/NotifyOnAttrSet.td com/sun/jini/test/impl/start/aggregatepolicyprovider/GetContextTest.td Improvements to PrincipalGrant equals and hashCode implementations to avoid inadvertent dns lookup caused by SocketPermission. Modified: river/jtsk/skunk/peterConcurrentPolicy/qa/src/com/sun/jini/qa/harness/MergedPolicyProvider.java river/jtsk/skunk/peterConcurrentPolicy/src/com/sun/jini/start/AggregatePolicyProvider.java river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/ConcurrentPermissions.java river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/DynamicPermissionCollection.java river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/GrantPermission.java river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/PermissionComparator.java river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/policy/ConcurrentPolicyFile.java river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/CertificateGrant.java river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/PrincipalGrant.java river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/URIGrant.java Modified: river/jtsk/skunk/peterConcurrentPolicy/qa/src/com/sun/jini/qa/harness/MergedPolicyProvider.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/qa/src/com/sun/jini/qa/harness/MergedPolicyProvider.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/qa/src/com/sun/jini/qa/harness/MergedPolicyProvider.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/qa/src/com/sun/jini/qa/harness/MergedPolicyProvider.java Mon Dec 26 11:59:41 2011 @@ -24,10 +24,14 @@ import java.security.Policy; import java.security.ProtectionDomain; import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; import java.util.Iterator; import java.util.StringTokenizer; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import net.jini.security.policy.PolicyInitializationException; import net.jini.security.policy.PolicyFileProvider; @@ -42,10 +46,12 @@ import net.jini.security.policy.PolicyFi */ public class MergedPolicyProvider extends Policy { + /** class state */ + private static final Lock lock = new ReentrantLock();; // protects first + private static boolean first = false; // Why is first static? + /** the collection of underlying policies */ - private ArrayList policies = new ArrayList(); - - private static boolean first = false; + private final Collection<Policy> policies ; /** * Creates a new <code>MergedPolicyProvider</code> instance that wraps a @@ -74,6 +80,7 @@ public class MergedPolicyProvider extend } // no-arg semantics for 'default policy' necessary for correct behavior // of PolicyFileProvider.refresh + Collection<Policy> policies = new ArrayList<Policy>(); try { if (p1 != null) { policies.add(new PolicyFileProvider()); @@ -93,6 +100,7 @@ public class MergedPolicyProvider extend throw new PolicyInitializationException( "unable to construct base policy", e); } + this.policies = Collections.unmodifiableCollection(policies); } /** @@ -133,53 +141,58 @@ public class MergedPolicyProvider extend */ public PermissionCollection getPermissions(ProtectionDomain domain) { Iterator it = policies.iterator(); - ArrayList list = new ArrayList(); - if (it.hasNext()) { - PermissionCollection pc = - ((Policy) it.next()).getPermissions(domain); - if (first) { - first = false; - Enumeration en = pc.elements(); - list.add("BASE PERMISSIONS for domain " + domain); - while (en.hasMoreElements()) { - Permission perm = (Permission) en.nextElement(); - list.add(perm.toString()); - } - first = true; - } - while (it.hasNext()) { - PermissionCollection pc2 = - ((Policy) it.next()).getPermissions(domain); - Enumeration en = pc2.elements(); - while (en.hasMoreElements()) { - Permission perm = (Permission) en.nextElement(); - if (!pc.implies(perm)) { - if (first) { - first = false; - list.add("checking " + perm + " and adding"); - first = true; - } - pc.add(perm); - } else { - if (first) { - first = false; - list.add("checking " + perm + " and not adding"); - first = true; - } - } - } - } - if (first) { - first = false; - for (int i = 0; i < list.size(); i++) { - System.out.println((String) list.get(i)); - } - first = true; - } - return pc; - } else { - throw new IllegalStateException("No policies in provider"); - } + ArrayList list = new ArrayList(64); + lock.lock(); + try { + if (it.hasNext()) { + PermissionCollection pc = + ((Policy) it.next()).getPermissions(domain); + if (first) { + first = false; + Enumeration en = pc.elements(); + list.add("BASE PERMISSIONS for domain " + domain); + while (en.hasMoreElements()) { + Permission perm = (Permission) en.nextElement(); + list.add(perm.toString()); + } + first = true; + } + while (it.hasNext()) { + PermissionCollection pc2 = + ((Policy) it.next()).getPermissions(domain); + Enumeration en = pc2.elements(); + while (en.hasMoreElements()) { + Permission perm = (Permission) en.nextElement(); + if (!pc.implies(perm)) { + if (first) { + first = false; + list.add("checking " + perm + " and adding"); + first = true; + } + pc.add(perm); + } else { + if (first) { + first = false; + list.add("checking " + perm + " and not adding"); + first = true; + } + } + } + } + if (first) { + first = false; + for (int i = 0; i < list.size(); i++) { + System.out.println((String) list.get(i)); + } + first = true; + } + return pc; + } else { + throw new IllegalStateException("No policies in provider"); + } + }finally{ + lock.unlock(); + } } /** Modified: river/jtsk/skunk/peterConcurrentPolicy/src/com/sun/jini/start/AggregatePolicyProvider.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/com/sun/jini/start/AggregatePolicyProvider.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/com/sun/jini/start/AggregatePolicyProvider.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/com/sun/jini/start/AggregatePolicyProvider.java Mon Dec 26 11:59:41 2011 @@ -494,7 +494,7 @@ public class AggregatePolicyProvider */ private static class AggregateSecurityContext implements SecurityContext { - private final ClassLoader ccl = getContextClassLoader(); + private final ClassLoader ccl; private final SecurityContext sc; AggregateSecurityContext(SecurityContext sc) { @@ -502,6 +502,7 @@ public class AggregatePolicyProvider throw new NullPointerException(); } this.sc = sc; + ccl = getContextClassLoader(); } public PrivilegedAction wrap(PrivilegedAction a) { Modified: river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/ConcurrentPermissions.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/ConcurrentPermissions.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/ConcurrentPermissions.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/ConcurrentPermissions.java Mon Dec 26 11:59:41 2011 @@ -109,7 +109,7 @@ implements Serializable { // this get saves unnecessary object creation. Class clas = permission.getClass(); PermissionCollection pc = permsMap.get(clas); - if (pc == null){ + if (pc == null){ pc = new DynamicPermissionCollection(null, clas); PermissionCollection existed = permsMap.putIfAbsent(clas, pc); Modified: river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/DynamicPermissionCollection.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/DynamicPermissionCollection.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/DynamicPermissionCollection.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/DynamicPermissionCollection.java Mon Dec 26 11:59:41 2011 @@ -31,6 +31,8 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.Enumeration; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import org.apache.river.impl.util.CollectionsConcurrent; /** @@ -47,17 +49,18 @@ final class DynamicPermissionCollection private final Class cl; private final Comparator<Permission> comp; + private final Lock lock; + private PermissionCollection cache; // protected by lock. + private volatile int cacheCount; + private volatile boolean noPC; + DynamicPermissionCollection(Comparator<Permission> c, Class cl){ perms = CollectionsConcurrent.multiReadCollection(new ArrayList<Permission>()); this.cl = cl; comp = c; - } - - private DynamicPermissionCollection(Class cl, Comparator<Permission> c){ - this.cl = cl; - comp = c; - Collection<Permission> col = new ArrayList<Permission>(); - perms = CollectionsConcurrent.multiReadCollection(col); + lock = new ReentrantLock(); + cacheCount = 0; + noPC = false; } @Override @@ -71,21 +74,46 @@ final class DynamicPermissionCollection @Override public boolean implies(Permission permission) { if ( ! cl.isInstance(permission)) return false; - Thread thread = Thread.currentThread(); - if (thread.isInterrupted()) return false; + if ( !noPC && lock.tryLock()){ + try { + if (cache != null && cache.implies(permission)) return true; + } finally { + lock.unlock(); + } + } Permission [] p = perms.toArray(new Permission[0]); //perms.size() may change if (comp != null){ Arrays.sort(p, comp); } - PermissionCollection pc = permission.newPermissionCollection(); - int l = p.length; + if (noPC) return rawImplies(permission, p); + int len = p.length; + PermissionCollection pc = null; + boolean result = false; + pc = permission.newPermissionCollection(); if (pc != null) { - for ( int i = 0; i < l ; i++ ){ + for ( int i = 0; i < len ; i++ ){ pc.add(p[i]); } - return pc.implies(permission); + result = pc.implies(permission); + if ( cacheCount < len){ + lock.lock(); + try { + cache = pc; + cacheCount = len; + } finally { + lock.unlock(); + } + } + return result; + } else { + noPC = true; + return rawImplies(permission, p); } - for ( int i = 0; i < l ; i++ ){ + } + + private boolean rawImplies(Permission permission, Permission[] p){ + int len = p.length; + for ( int i = 0; i < len ; i++ ){ if (p[i].implies(permission)) return true; } return false; @@ -138,7 +166,7 @@ final class DynamicPermissionCollection private Object readResolve() { PermissionCollection pc = - new DynamicPermissionCollection(clazz, comp); + new DynamicPermissionCollection(comp, clazz); int length = permissions.length; for ( int i = 0 ; i < length ; i++){ pc.add(permissions[i]); Modified: river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/GrantPermission.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/GrantPermission.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/GrantPermission.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/GrantPermission.java Mon Dec 26 11:59:41 2011 @@ -36,11 +36,13 @@ import java.security.Permissions; import java.security.UnresolvedPermission; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Enumeration; import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.TreeSet; import net.jini.security.policy.DynamicPolicy; /** @@ -763,7 +765,8 @@ public final class GrantPermission exten new ObjectStreamField("perms", List.class, true) }; - private List perms = new ArrayList(40); + private Collection<Permission> perms = + new TreeSet<Permission>(new PermissionComparator()); private Implier implier = new Implier(); public synchronized void add(Permission p) { @@ -774,13 +777,13 @@ public final class GrantPermission exten throw new SecurityException( "can't add to read-only PermissionCollection"); } -// if (!perms.contains(p)){ + if (!perms.contains(p)){ perms.add(p); implier.add((GrantPermission) p); -// } + } } - public synchronized Enumeration elements() { + public synchronized Enumeration<Permission> elements() { return Collections.enumeration(perms); } Modified: river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/PermissionComparator.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/PermissionComparator.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/PermissionComparator.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/PermissionComparator.java Mon Dec 26 11:59:41 2011 @@ -12,6 +12,7 @@ import java.util.Comparator; public class PermissionComparator implements Comparator<Permission> { public int compare(Permission o1, Permission o2) { + if (o1 == o2) return 0; Class c1 = o1.getClass(); String name1 = o1.getName(); String actions1 = o1.getActions(); Modified: river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/policy/ConcurrentPolicyFile.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/policy/ConcurrentPolicyFile.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/policy/ConcurrentPolicyFile.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/net/jini/security/policy/ConcurrentPolicyFile.java Mon Dec 26 11:59:41 2011 @@ -42,6 +42,7 @@ import java.security.PrivilegedActionExc import java.security.PrivilegedExceptionAction; import java.security.ProtectionDomain; import java.security.SecurityPermission; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Enumeration; @@ -189,7 +190,7 @@ public class ConcurrentPolicyFile extend private final Lock wl; - private final Set<PermissionGrant> grants ; // protected by rwl + private final Collection<PermissionGrant> grants ; // protected by rwl // Calculated Permissions cache, organized as // Map{Object->Collection<Permission>}. @@ -223,7 +224,7 @@ public class ConcurrentPolicyFile extend ReadWriteLock rwl = new ReentrantReadWriteLock(); rl = rwl.readLock(); wl = rwl.writeLock(); - grants = new HashSet<PermissionGrant>(120); + grants = new ArrayList<PermissionGrant>(120); ConcurrentMap<Referrer<Object>, Referrer<Collection<Permission>>> cacheInternal = new ConcurrentHashMap<Referrer<Object>, Referrer<Collection<Permission>>>(120); cache = RC.concurrentMap(cacheInternal, Ref.WEAK_IDENTITY, Ref.SOFT); @@ -356,17 +357,22 @@ public class ConcurrentPolicyFile extend @Override public boolean implies(ProtectionDomain domain, Permission permission) { if (permission == null) throw new NullPointerException("permission not allowed to be null"); - PermissionCollection pc = - domain != null ? impliesCache.get(domain): null; - if (pc == null){ - pc = getPermissions(domain); - if (domain != null){ - PermissionCollection existed = impliesCache.putIfAbsent(domain, pc); - // Don't bother replacing pc with existed, we're not mutating - // so it doesn't matter. + rl.lock(); + try { + PermissionCollection pc = + domain != null ? impliesCache.get(domain): null; + if (pc == null){ + pc = getPermissions(domain); + if (domain != null){ + PermissionCollection existed = impliesCache.putIfAbsent(domain, pc); + // Don't bother replacing pc with existed, we're not mutating + // so it doesn't matter. + } } + return pc.implies(permission); + }finally{ + rl.unlock(); } - return pc.implies(permission); } /** Modified: river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/CertificateGrant.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/CertificateGrant.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/CertificateGrant.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/CertificateGrant.java Mon Dec 26 11:59:41 2011 @@ -77,13 +77,13 @@ class CertificateGrant extends Principal @Override public String toString(){ StringBuilder sb = new StringBuilder(400); - sb.append(super.toString()) - .append("Certificate's: \n"); + sb.append("\nCertificate's: \n"); Iterator<Certificate> it = certs.iterator(); while (it.hasNext()){ sb.append(it.next().toString()) .append("\n"); } + sb.append(super.toString()); return sb.toString(); } Modified: river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/PrincipalGrant.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/PrincipalGrant.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/PrincipalGrant.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/PrincipalGrant.java Mon Dec 26 11:59:41 2011 @@ -29,6 +29,7 @@ import java.security.Principal; import java.security.ProtectionDomain; import java.security.acl.Group; import java.security.cert.Certificate; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -36,6 +37,8 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.TreeSet; +import net.jini.security.PermissionComparator; import org.apache.river.impl.security.policy.util.PolicyUtils; /** @@ -57,18 +60,30 @@ class PrincipalGrant implements Permissi palCol.addAll(Arrays.asList(pals)); this.pals = Collections.unmodifiableSet(palCol); }else { - this.pals = Collections.EMPTY_SET; + this.pals = Collections.emptySet(); } if (perm == null || perm.length == 0) { - this.perms = Collections.EMPTY_SET; + this.perms = Collections.emptySet(); }else{ - Set<Permission> perms = new HashSet<Permission>(perm.length); + // PermissionComparator is used to avoid broken hashCode and equals + Set<Permission> perms = new TreeSet<Permission>(new PermissionComparator()); perms.addAll(Arrays.asList(perm)); this.perms = Collections.unmodifiableSet(perms); } int hash = 5; hash = 97 * hash + (this.pals != null ? this.pals.hashCode() : 0); - hash = 97 * hash + (this.perms != null ? this.perms.hashCode() : 0); + Iterator<Permission> i = perms.iterator(); + while (i.hasNext()){ + Permission p = i.next(); + if (p != null){ + Class c = p.getClass(); + String name = p.getName(); + String actions = p.getActions(); + hash = 97 * hash + (c != null ? c.hashCode() : 0); + hash = 97 * hash + (name != null ? name.hashCode() : 0); + hash = 97 * hash + (actions != null ? actions.hashCode() : 0); + } + } hashCode = hash; this.inverse = inverse; } @@ -81,7 +96,9 @@ class PrincipalGrant implements Permissi if (o instanceof PrincipalGrant ){ PrincipalGrant p = (PrincipalGrant) o; if (pals.equals(p.pals) - && perms.equals(p.perms)) return true; + // To avoid broken equals: + && perms.containsAll(p.perms) + && p.perms.containsAll(perms)) return true; } return false; } @@ -94,19 +111,18 @@ class PrincipalGrant implements Permissi @Override public String toString(){ StringBuilder sb = new StringBuilder(300); - sb.append(this.getClass().getCanonicalName()) - .append("Permissions: \n"); - Iterator<Permission> permIt = perms.iterator(); - while (permIt.hasNext()){ - sb.append(permIt.next().toString()) - .append("\n"); - } sb.append("Principals: \n"); Iterator<Principal> palIt = pals.iterator(); while (palIt.hasNext()){ sb.append(palIt.next().toString()) .append("\n"); } + sb.append("\nPermissions: \n"); + Iterator<Permission> permIt = perms.iterator(); + while (permIt.hasNext()){ + sb.append(permIt.next().toString()) + .append("\n"); + } return sb.toString(); } Modified: river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/URIGrant.java URL: http://svn.apache.org/viewvc/river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/URIGrant.java?rev=1224720&r1=1224719&r2=1224720&view=diff ============================================================================== --- river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/URIGrant.java (original) +++ river/jtsk/skunk/peterConcurrentPolicy/src/org/apache/river/api/security/URIGrant.java Mon Dec 26 11:59:41 2011 @@ -80,9 +80,11 @@ class URIGrant extends CertificateGrant @Override public String toString(){ StringBuilder sb = new StringBuilder(500); - return sb.append(super.toString()) - .append("URI: \n") + return sb.append("\n") + .append("URI: ") .append(location.toString()) + .append(super.toString()) + .append("\n") .toString(); }
