Bug 532194 - Added permission evaluation cache to increase performance
Increases performance when using many bundles and permission rows. Also
removes slow down as more bundles/rows are added to the system.
Change-Id: I31f2ee701654dc9e7d1598570bd04241126cd4da
Signed-off-by: Scott Tustison <scott.tustison@gmail.com>
diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/securityadmin/SecurityAdminUnitTests.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/securityadmin/SecurityAdminUnitTests.java
index a6b9dcc..fed04f2 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/securityadmin/SecurityAdminUnitTests.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/securityadmin/SecurityAdminUnitTests.java
@@ -39,6 +39,7 @@
private static final PermissionInfo[] RUNTIME_INFOS = new PermissionInfo[] {new PermissionInfo("java.lang.RuntimePermission", "exitVM", null)}; //$NON-NLS-1$ //$NON-NLS-2$
private static final ConditionInfo[] ALLLOCATION_CONDS = new ConditionInfo[] {new ConditionInfo("org.osgi.service.condpermadmin.BundleLocationCondition", new String[] {"*"})}; //$NON-NLS-1$ //$NON-NLS-2$
+ private static final ConditionInfo MUT_SAT = new ConditionInfo("ext.framework.b.TestCondition", new String[] {"MUT_SAT", "true", "false", "true"}); //$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
private static final ConditionInfo POST_MUT_SAT = new ConditionInfo("ext.framework.b.TestCondition", new String[] {"POST_MUT_SAT", "true", "true", "true"}); //$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
private static final ConditionInfo POST_MUT_UNSAT = new ConditionInfo("ext.framework.b.TestCondition", new String[] {"POST_MUT_UNSAT", "true", "true", "false"}); //$NON-NLS-1$//$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$
@@ -647,6 +648,30 @@
testSMPermission(pds, new FilePermission("test", "read"), true); //$NON-NLS-1$ //$NON-NLS-2$
}
+ public void testMutableConditions() {
+ installConditionBundle();
+ TestCondition.clearConditions();
+
+ Bundle test1 = installTestBundle(TEST_BUNDLE);
+ ProtectionDomain pd1 = test1.adapt(ProtectionDomain.class);
+ ProtectionDomain[] pds = new ProtectionDomain[] {pd1};
+
+ ConditionalPermissionUpdate update = cpa.newConditionalPermissionUpdate();
+ List<ConditionalPermissionInfo> rows = update.getConditionalPermissionInfos();
+ rows.add(cpa.newConditionalPermissionInfo(null, new ConditionInfo[] {MUT_SAT}, READONLY_INFOS, ConditionalPermissionInfo.DENY));
+ rows.add(cpa.newConditionalPermissionInfo(null, ALLLOCATION_CONDS, READONLY_INFOS, ConditionalPermissionInfo.ALLOW));
+ assertTrue("failed to commit", update.commit()); //$NON-NLS-1$);
+
+ testSMPermission(pds, new FilePermission("test", "read"), false); //$NON-NLS-1$ //$NON-NLS-2$
+
+ TestCondition tc1sat = TestCondition.getTestCondition("MUT_SAT_" + test1.getBundleId()); //$NON-NLS-1$
+
+ assertNotNull("tc1sat", tc1sat); //$NON-NLS-1$
+
+ tc1sat.setSatisfied(false);
+ testSMPermission(pds, new FilePermission("test", "read"), true); //$NON-NLS-1$ //$NON-NLS-2$
+ }
+
public void testAccessControlContext01() {
// test single row with signer condition
ConditionalPermissionUpdate update = cpa.newConditionalPermissionUpdate();
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/EvaluationCacheKey.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/EvaluationCacheKey.java
new file mode 100644
index 0000000..04b87fc
--- /dev/null
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/EvaluationCacheKey.java
@@ -0,0 +1,45 @@
+/*******************************************************************************
+ * Copyright (c) 2018 Connexta, LLC and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *
+ * Contributors:
+ * Connexta, LLC - initial implementation
+ *******************************************************************************/
+package org.eclipse.osgi.internal.permadmin;
+
+import java.security.Permission;
+import java.util.Objects;
+
+class EvaluationCacheKey {
+
+ private final Permission permission;
+
+ private final Long bundleId;
+
+ EvaluationCacheKey(BundlePermissions bundlePermissions, Permission permission) {
+ this.permission = permission;
+ this.bundleId = bundlePermissions.getBundle().getBundleId();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ EvaluationCacheKey that = (EvaluationCacheKey) o;
+ return Objects.equals(bundleId, that.bundleId) && Objects.equals(
+ permission,
+ that.permission);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(bundleId, permission);
+ }
+}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityAdmin.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityAdmin.java
index e156d6d..4b99217 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityAdmin.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityAdmin.java
@@ -405,6 +405,7 @@
permAdminCollections[i].clearPermissionCache();
for (int i = 0; i < condAdminRows.length; i++)
condAdminRows[i].clearCaches();
+ condAdminTable.clearEvaluationCache();
}
EquinoxSecurityManager getSupportedSecurityManager() {
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityRow.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityRow.java
index 49431e6..e6f6f09 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityRow.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityRow.java
@@ -7,6 +7,7 @@
*
* Contributors:
* IBM Corporation - initial API and implementation
+ * Connexta, LLC - performance improvements
*******************************************************************************/
package org.eclipse.osgi.internal.permadmin;
@@ -33,6 +34,7 @@
private final boolean deny;
/* GuardedBy(bundleConditions) */
final Map<BundlePermissions, Condition[]> bundleConditions;
+ final Object bundleConditionsLock = new Object();
public SecurityRow(SecurityAdmin securityAdmin, String name, ConditionInfo[] conditionInfos, PermissionInfo[] permissionInfos, String decision) {
if (permissionInfos == null || permissionInfos.length == 0)
@@ -237,62 +239,92 @@
securityAdmin.delete(this, true);
}
- Condition[] getConditions(Bundle bundle) {
- Condition[] conditions = new Condition[conditionInfos.length];
- for (int i = 0; i < conditionInfos.length; i++) {
- /*
- * TODO: Can we pre-get the Constructors in our own constructor
- */
- Class<?> clazz;
- try {
- clazz = Class.forName(conditionInfos[i].getType());
- } catch (ClassNotFoundException e) {
- /* If the class isn't there, we fail */
- return null;
+ Condition[] getConditions(BundlePermissions bundlePermissions) {
+ synchronized (bundleConditionsLock) {
+ Condition[] conditions = null;
+ if (bundleConditions != null) {
+ conditions = bundleConditions.get(bundlePermissions);
}
- Constructor<?> constructor = null;
- Method method = null;
- try {
- method = clazz.getMethod("getCondition", conditionMethodArgs); //$NON-NLS-1$
- if ((method.getModifiers() & Modifier.STATIC) == 0)
- method = null;
- } catch (NoSuchMethodException e) {
- // This is a normal case
- }
- if (method == null)
- try {
- constructor = clazz.getConstructor(conditionMethodArgs);
- } catch (NoSuchMethodException e) {
- // TODO should post a FrameworkEvent of type error here
- conditions[i] = Condition.FALSE;
- continue;
- }
+ if (conditions == null) {
+ conditions = new Condition[conditionInfos.length];
+ for (int i = 0; i < conditionInfos.length; i++) {
+ /*
+ * TODO: Can we pre-get the Constructors in our own constructor
+ */
+ Class<?> clazz;
+ try {
+ clazz = Class.forName(conditionInfos[i].getType());
+ } catch (ClassNotFoundException e) {
+ /* If the class isn't there, we fail */
+ return null;
+ }
+ Constructor<?> constructor = null;
+ Method method = getConditionMethod(clazz);
+ if (method == null) {
+ constructor = getConditionConstructor(clazz);
+ if (constructor == null) {
+ // TODO should post a FrameworkEvent of type error here
+ conditions[i] = Condition.FALSE;
+ continue;
+ }
+ }
- Object args[] = {bundle, conditionInfos[i]};
- try {
- if (method != null)
- conditions[i] = (Condition) method.invoke(null, args);
- else
- conditions[i] = (Condition) constructor.newInstance(args);
- } catch (Throwable t) {
- // TODO should post a FrameworkEvent of type error here
- conditions[i] = Condition.FALSE;
+ Object[] args = {bundlePermissions.getBundle(), conditionInfos[i]};
+ try {
+ if (method != null)
+ conditions[i] = (Condition) method.invoke(null, args);
+ else
+ conditions[i] = (Condition) constructor.newInstance(args);
+ } catch (Exception e) {
+ // TODO should post a FrameworkEvent of type error here
+ conditions[i] = Condition.FALSE;
+ }
+ }
+ if (bundleConditions != null) {
+ bundleConditions.put(bundlePermissions, conditions);
+ }
+ }
+ return conditions;
+ }
+ }
+
+ private Method getConditionMethod(Class<?> clazz) {
+ for (Method checkMethod : clazz.getMethods()) {
+ if (checkMethod.getName().equals("getCondition")
+ && (checkMethod.getModifiers() & Modifier.STATIC) == Modifier.STATIC
+ && checkParameterTypes(checkMethod.getParameterTypes())) {
+ return checkMethod;
}
}
- return conditions;
+ return null;
+ }
+
+ private Constructor<?> getConditionConstructor(Class<?> clazz) {
+ for (Constructor<?> checkConstructor : clazz.getConstructors()) {
+ if (checkParameterTypes(checkConstructor.getParameterTypes())) {
+ return checkConstructor;
+ }
+ }
+ return null;
+ }
+
+ private boolean checkParameterTypes(Class<?>[] foundTypes) {
+ if (foundTypes.length != conditionMethodArgs.length) {
+ return false;
+ }
+
+ for (int i = 0; i < foundTypes.length; i++) {
+ if (!foundTypes[i].isAssignableFrom(conditionMethodArgs[i])) {
+ return false;
+ }
+ }
+ return true;
}
Decision evaluate(BundlePermissions bundlePermissions, Permission permission) {
if (bundleConditions == null || bundlePermissions == null)
return evaluatePermission(permission);
- Condition[] conditions;
- synchronized (bundleConditions) {
- conditions = bundleConditions.get(bundlePermissions);
- if (conditions == null) {
- conditions = getConditions(bundlePermissions.getBundle());
- bundleConditions.put(bundlePermissions, conditions);
- }
- }
+ Condition[] conditions = getConditions(bundlePermissions);
if (conditions == ABSTAIN_LIST)
return DECISION_ABSTAIN;
if (conditions == SATISFIED_LIST)
@@ -314,7 +346,7 @@
} else {
if (!mutable)
// this will cause the row to always abstain; mark this to be ignored in future checks
- synchronized (bundleConditions) {
+ synchronized (bundleConditionsLock) {
bundleConditions.put(bundlePermissions, ABSTAIN_LIST);
}
return DECISION_ABSTAIN;
@@ -333,7 +365,7 @@
empty &= conditions[i] == null;
}
if (empty) {
- synchronized (bundleConditions) {
+ synchronized (bundleConditionsLock) {
bundleConditions.put(bundlePermissions, SATISFIED_LIST);
}
}
@@ -413,7 +445,7 @@
void clearCaches() {
permissionInfoCollection.clearPermissionCache();
if (bundleConditions != null)
- synchronized (bundleConditions) {
+ synchronized (bundleConditionsLock) {
bundleConditions.clear();
}
}
@@ -435,7 +467,7 @@
if (mutable || !condition.isPostponed())
return; // do nothing
if (isSatisfied) {
- synchronized (row.bundleConditions) {
+ synchronized (row.bundleConditionsLock) {
Condition[] rowConditions = row.bundleConditions.get(bundlePermissions);
boolean isEmpty = true;
for (int i = 0; i < rowConditions.length; i++) {
@@ -448,7 +480,7 @@
row.bundleConditions.put(bundlePermissions, SATISFIED_LIST);
}
} else {
- synchronized (row.bundleConditions) {
+ synchronized (row.bundleConditionsLock) {
row.bundleConditions.put(bundlePermissions, ABSTAIN_LIST);
}
}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityTable.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityTable.java
index bdd1c18..43dc911 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityTable.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/internal/permadmin/SecurityTable.java
@@ -7,13 +7,18 @@
*
* Contributors:
* IBM Corporation - initial API and implementation
+ * Connexta, LLC - evaluation cache implementation
*******************************************************************************/
package org.eclipse.osgi.internal.permadmin;
import java.security.Permission;
import java.security.PermissionCollection;
import java.util.Enumeration;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
import org.eclipse.osgi.internal.permadmin.SecurityRow.Decision;
+import org.osgi.service.condpermadmin.Condition;
public class SecurityTable extends PermissionCollection {
private static final long serialVersionUID = -1800193310096318060L;
@@ -22,9 +27,14 @@
static final int ABSTAIN = 0x0004;
static final int POSTPONED = 0x0008;
+ private static final int MUTABLE = 0x0016;
+
private final SecurityRow[] rows;
private final SecurityAdmin securityAdmin;
+ private final transient Map<EvaluationCacheKey, Integer> evaluationCache =
+ new ConcurrentHashMap<>(10000);
+
public SecurityTable(SecurityAdmin securityAdmin, SecurityRow[] rows) {
if (rows == null)
throw new NullPointerException("rows cannot be null!!"); //$NON-NLS-1$
@@ -37,63 +47,130 @@
}
int evaluate(BundlePermissions bundlePermissions, Permission permission) {
- if (isEmpty())
+ if (bundlePermissions == null) {
return ABSTAIN;
+ }
+ EvaluationCacheKey evaluationCacheKey = new EvaluationCacheKey(bundlePermissions,
+ permission);
+ if (isEmpty()) {
+ evaluationCache.put(evaluationCacheKey, ABSTAIN);
+ return ABSTAIN;
+ }
+
+ //can't short-circuit early, so try cache
+ Integer result = evaluationCache.get(evaluationCacheKey);
+ boolean hasMutable = false;
+ if (result != null) {
+ hasMutable = (result & MUTABLE) == MUTABLE;
+ if (!hasMutable) {
+ return result;
+ }
+ }
+ //cache miss or has mutable rows
boolean postponed = false;
Decision[] results = new Decision[rows.length];
int immediateDecisionIdx = -1;
// evaluate each row
- for (int i = 0; i < rows.length; i++) {
+ for (int i = 0; i < rows.length && immediateDecisionIdx == -1; i++) {
+ if (result == null) {
+ //check all conditions for any that are mutable, this will turn off the cache
+ hasMutable |= checkMutable(bundlePermissions,
+ evaluationCacheKey,
+ rows[i]);
+ }
try {
results[i] = rows[i].evaluate(bundlePermissions, permission);
- } catch (Throwable t) {
+ } catch (Exception e) {
// TODO log?
results[i] = SecurityRow.DECISION_ABSTAIN;
}
- if ((results[i].decision & ABSTAIN) != 0)
+ if ((results[i].decision & ABSTAIN) == ABSTAIN)
continue; // ignore this row and continue to next row
- if ((results[i].decision & POSTPONED) != 0) {
+ if ((results[i].decision & POSTPONED) == POSTPONED) {
// row is postponed; we can no longer return quickly on a denied decision
postponed = true;
continue; // continue to next row
}
- if (!postponed)
+ if (!postponed) {
// no postpones encountered yet; we can return the decision quickly
+ if (!hasMutable) {
+ evaluationCache.put(evaluationCacheKey, results[i].decision);
+ }
return results[i].decision; // return GRANTED or DENIED
+ }
// got an immediate answer; but it is after a postponed condition.
// no need to process the rest of the rows
immediateDecisionIdx = i;
- break;
}
- if (postponed) {
- int immediateDecision = immediateDecisionIdx < 0 ? DENIED : results[immediateDecisionIdx].decision;
- // iterate over all postponed conditions;
- // if they all provide the same decision as the immediate decision then return the immediate decision
- boolean allSameDecision = true;
- int i = immediateDecisionIdx < 0 ? results.length - 1 : immediateDecisionIdx - 1;
- for (; i >= 0 && allSameDecision; i--) {
- if (results[i] == null)
- continue;
- if ((results[i].decision & POSTPONED) != 0) {
- if ((results[i].decision & immediateDecision) == 0)
- allSameDecision = false;
- else
- results[i] = SecurityRow.DECISION_ABSTAIN; // we can clear postpones with the same decision as the immediate
+ Integer immediateDecision = handlePostponedConditions(evaluationCacheKey,
+ hasMutable,
+ postponed,
+ results,
+ immediateDecisionIdx);
+ if (immediateDecision != null)
+ return immediateDecision;
+ int finalDecision = postponed ? POSTPONED : ABSTAIN;
+ if (!hasMutable && (finalDecision & POSTPONED) != POSTPONED) {
+ evaluationCache.put(evaluationCacheKey, finalDecision);
+ }
+ return finalDecision;
+ }
+
+ private boolean checkMutable(BundlePermissions bundlePermissions,
+ EvaluationCacheKey evaluationCacheKey, SecurityRow row) {
+ Condition[] conditions = row.getConditions(bundlePermissions);
+ if (conditions != null) {
+ for (Condition condition : conditions) {
+ if (condition.isMutable()) {
+ evaluationCache.put(evaluationCacheKey, MUTABLE);
+ return true;
}
}
- if (allSameDecision)
- return immediateDecision;
-
- // we now are forced to postpone; we need to also remember the postponed decisions and
- // the immediate decision if there is one.
- EquinoxSecurityManager equinoxManager = securityAdmin.getSupportedSecurityManager();
- if (equinoxManager == null)
- // TODO this is really an error condition.
- // This should never happen. We checked for a supported manager when the row was postponed
- return ABSTAIN;
- equinoxManager.addConditionsForDomain(results);
}
- return postponed ? POSTPONED : ABSTAIN;
+ return false;
+ }
+
+ private Integer handlePostponedConditions(EvaluationCacheKey evaluationCacheKey,
+ boolean hasMutable, boolean postponed, Decision[] results, int immediateDecisionIdx) {
+ if (postponed) {
+ int immediateDecision = immediateDecisionIdx < 0 ? DENIED : results[immediateDecisionIdx].decision;
+ // iterate over all postponed conditions;
+ // if they all provide the same decision as the immediate decision then return the immediate decision
+ boolean allSameDecision = true;
+ int i = immediateDecisionIdx < 0 ? results.length - 1 : immediateDecisionIdx - 1;
+ for (; i >= 0 && allSameDecision; i--) {
+ if ((results[i].decision & POSTPONED) == POSTPONED) {
+ if ((results[i].decision & immediateDecision) == 0)
+ allSameDecision = false;
+ else
+ results[i] = SecurityRow.DECISION_ABSTAIN; // we can clear postpones with the same decision as the immediate
+ }
+ }
+ if (allSameDecision) {
+ if (!hasMutable) {
+ evaluationCache.put(evaluationCacheKey, immediateDecision);
+ }
+ return immediateDecision;
+ }
+
+ // we now are forced to postpone; we need to also remember the postponed decisions and
+ // the immediate decision if there is one.
+ EquinoxSecurityManager equinoxManager = securityAdmin.getSupportedSecurityManager();
+ if (equinoxManager == null) {
+ // TODO this is really an error condition.
+ // This should never happen. We checked for a supported manager when the row was postponed
+ if (!hasMutable) {
+ evaluationCache.put(evaluationCacheKey, ABSTAIN);
+ }
+ return ABSTAIN;
+ }
+ equinoxManager.addConditionsForDomain(results);
+ }
+ return null;
+ }
+
+ void clearEvaluationCache() {
+ evaluationCache.clear();
}
SecurityRow getRow(int i) {
