From fd019f6b78c201d7eb1a88b63884a155bd5e0ca7 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Fri, 3 Jul 2026 10:55:46 +0300 Subject: [PATCH 1/3] Make AciList reads lock-free with true copy-on-write AciList.getCandidateAcis() acquired a ReentrantReadWriteLock read lock on every access-controlled operation (compare, search, modify, ...) to walk the ACI map. The field comment claims "copy-on-write technique to avoid locking when reading", but mutators actually modified the volatile DITCacheMap (and its value lists) in place under the write lock, so the read lock could not be dropped. Make the copy-on-write real: mutators (add/remove/rename/modify) build a copy of the map with copied value lists under the write lock and publish it through the volatile reference; a published map is never modified in place. getCandidateAcis() now reads a snapshot lock-free. ACI mutations are rare (startup, backend initialization, ACI modifications), so the copy cost is negligible. --- .../authorization/dseecompat/AciList.java | 127 +++++++++++------- 1 file changed, 79 insertions(+), 48 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java index 605b47951a..3514087d9c 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java @@ -48,12 +48,17 @@ public class AciList { /** * A map containing all the ACIs. - * We use the copy-on-write technique to avoid locking when reading. + * We use the copy-on-write technique to avoid locking when reading: + * mutators build a copy of the map (with copied value lists), modify the + * copy and publish it through this volatile reference. A published map is + * never modified in place, so getCandidateAcis() can read it lock-free — + * it runs for every access-controlled operation, and even a read lock + * becomes a cross-core hotspot under load. */ private volatile DITCacheMap> aciList = new DITCacheMap<>(); /** - * Lock to protect internal data structures. + * Lock serializing mutators (ACI additions, removals and renames are rare). */ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); @@ -87,46 +92,56 @@ public List getCandidateAcis(DN baseDN) { return candidates; } - lock.readLock().lock(); - try - { - //Save the baseDN in case we need to evaluate a global ACI. - DN entryDN=baseDN; - while (baseDN != null) { - List acis = aciList.get(baseDN); - if (acis != null) { - //Check if there are global ACIs. Global ACI has a NULL DN. - if (baseDN.isRootDN()) { - for (Aci aci : acis) { - AciTargets targets = aci.getTargets(); - //If there is a target, evaluate it to see if this ACI should - //be included in the candidate set. - if (targets != null - && AciTargets.isTargetApplicable(aci, targets, entryDN)) - { - candidates.add(aci); //Add this ACI to the candidates. - } + // Lock-free read of the copy-on-write map: mutators never modify a + // published map or its value lists in place. + final DITCacheMap> aciList = this.aciList; + //Save the baseDN in case we need to evaluate a global ACI. + DN entryDN=baseDN; + while (baseDN != null) { + List acis = aciList.get(baseDN); + if (acis != null) { + //Check if there are global ACIs. Global ACI has a NULL DN. + if (baseDN.isRootDN()) { + for (Aci aci : acis) { + AciTargets targets = aci.getTargets(); + //If there is a target, evaluate it to see if this ACI should + //be included in the candidate set. + if (targets != null + && AciTargets.isTargetApplicable(aci, targets, entryDN)) + { + candidates.add(aci); //Add this ACI to the candidates. } - } else { - candidates.addAll(acis); } - } - if(baseDN.isRootDN()) { - break; - } - DN parentDN=baseDN.parent(); - if(parentDN == null) { - baseDN=DN.rootDN(); } else { - baseDN=parentDN; + candidates.addAll(acis); } } - return candidates; + if(baseDN.isRootDN()) { + break; + } + DN parentDN=baseDN.parent(); + if(parentDN == null) { + baseDN=DN.rootDN(); + } else { + baseDN=parentDN; + } } - finally + return candidates; + } + + /** + * Returns a copy of the current ACI map with copied value lists, suitable + * for mutation and subsequent publication through the volatile reference. + * Must be called while holding the write lock. + */ + private DITCacheMap> copyAciList() + { + DITCacheMap> copy = new DITCacheMap<>(); + for (Map.Entry> entry : aciList.entrySet()) { - lock.readLock().unlock(); + copy.put(entry.getKey(), new LinkedList<>(entry.getValue())); } + return copy; } /** @@ -144,14 +159,16 @@ public int addAci(List entries, lock.writeLock().lock(); try { + DITCacheMap> copy = copyAciList(); int validAcis = 0; for (Entry entry : entries) { DN dn=entry.getName(); List attributeList = entry.getOperationalAttribute(AciHandler.aciType); - validAcis += addAciAttributeList(aciList, dn, configDN, + validAcis += addAciAttributeList(copy, dn, configDN, attributeList, failedACIMsgs); } + aciList = copy; return validAcis; } finally @@ -173,7 +190,9 @@ public void addAci(DN dn, SortedSet acis) { lock.writeLock().lock(); try { - aciList.put(dn, new LinkedList<>(acis)); + DITCacheMap> copy = copyAciList(); + copy.put(dn, new LinkedList<>(acis)); + aciList = copy; } finally { @@ -198,21 +217,23 @@ public int addAci(Entry entry, boolean hasAci, lock.writeLock().lock(); try { + DITCacheMap> copy = copyAciList(); int validAcis = 0; //Process global "ds-cfg-global-aci" attribute type. The oldentry //DN is checked to verify it is equal to the config DN. If not those //attributes are skipped. if(hasGlobalAci && entry.getName().equals(configDN)) { List attributeList = entry.getAllAttributes(globalAciType); - validAcis = addAciAttributeList(aciList, DN.rootDN(), configDN, + validAcis = addAciAttributeList(copy, DN.rootDN(), configDN, attributeList, failedACIMsgs); } if(hasAci) { List attributeList = entry.getAllAttributes(aciType); - validAcis += addAciAttributeList(aciList, entry.getName(), configDN, + validAcis += addAciAttributeList(copy, entry.getName(), configDN, attributeList, failedACIMsgs); } + aciList = copy; return validAcis; } finally @@ -285,24 +306,26 @@ public void modAciOldNewEntry(Entry oldEntry, Entry newEntry, lock.writeLock().lock(); try { + DITCacheMap> copy = copyAciList(); List failedACIMsgs=new LinkedList<>(); //Process "aci" attribute types. if(hasAci) { - aciList.remove(oldEntry.getName()); + copy.remove(oldEntry.getName()); List attributeList = newEntry.getOperationalAttribute(aciType); - addAciAttributeList(aciList,newEntry.getName(), configDN, + addAciAttributeList(copy,newEntry.getName(), configDN, attributeList, failedACIMsgs); } //Process global "ds-cfg-global-aci" attribute type. The oldentry //DN is checked to verify it is equal to the config DN. If not those //attributes are skipped. if(hasGlobalAci && oldEntry.getName().equals(configDN)) { - aciList.remove(DN.rootDN()); + copy.remove(DN.rootDN()); List attributeList = newEntry.getAllAttributes(globalAciType); - addAciAttributeList(aciList, DN.rootDN(), configDN, + addAciAttributeList(copy, DN.rootDN(), configDN, attributeList, failedACIMsgs); } + aciList = copy; } finally { @@ -346,17 +369,21 @@ public boolean removeAci(Entry entry, boolean hasAci, lock.writeLock().lock(); try { + DITCacheMap> copy = copyAciList(); DN entryDN = entry.getName(); if (hasGlobalAci && entryDN.equals(configDN) && - aciList.remove(DN.rootDN()) == null) + copy.remove(DN.rootDN()) == null) { + aciList = copy; return false; } + boolean result = true; if (hasAci || !hasGlobalAci) { - return aciList.removeSubtree(entryDN, null); + result = copy.removeSubtree(entryDN, null); } - return true; + aciList = copy; + return result; } finally { @@ -374,8 +401,9 @@ public void removeAci(LocalBackend backend) { lock.writeLock().lock(); try { + DITCacheMap> copy = copyAciList(); Iterator>> iterator = - aciList.entrySet().iterator(); + copy.entrySet().iterator(); while (iterator.hasNext()) { Map.Entry> mapEntry = iterator.next(); @@ -384,6 +412,7 @@ public void removeAci(LocalBackend backend) { iterator.remove(); } } + aciList = copy; } finally { @@ -402,9 +431,10 @@ public void renameAci(DN oldDN, DN newDN ) { lock.writeLock().lock(); try { + DITCacheMap> copy = copyAciList(); Map> tempAciList = new HashMap<>(); Iterator>> iterator = - aciList.entrySet().iterator(); + copy.entrySet().iterator(); while (iterator.hasNext()) { Map.Entry> hashEntry = iterator.next(); DN keyDn = hashEntry.getKey(); @@ -427,7 +457,8 @@ public void renameAci(DN oldDN, DN newDN ) { iterator.remove(); } } - aciList.putAll(tempAciList); + copy.putAll(tempAciList); + aciList = copy; } finally { From 1c179537ebf1d569ffe4cac32a45a7ad2d8a7eb8 Mon Sep 17 00:00:00 2001 From: Valery Kharseko Date: Fri, 3 Jul 2026 11:38:09 +0300 Subject: [PATCH 2/3] Update copyright information in AciList.java --- .../java/org/opends/server/authorization/dseecompat/AciList.java | 1 + 1 file changed, 1 insertion(+) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java index 3514087d9c..cdfa55aeee 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java @@ -13,6 +13,7 @@ * * Copyright 2008-2010 Sun Microsystems, Inc. * Portions Copyright 2013-2016 ForgeRock AS. + * Portions Copyright 2026 3A Systems, LLC */ package org.opends.server.authorization.dseecompat; From b712f6ec3c0f250508bd13dff86e0e1bf5e956e3 Mon Sep 17 00:00:00 2001 From: Valera V Harseko Date: Sat, 4 Jul 2026 12:12:44 +0300 Subject: [PATCH 3/3] AciList review follow-ups: plain mutator lock, COW regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the ReentrantReadWriteLock with a ReentrantLock — the read lock has no users since reads went lock-free — and document that Aci instances are treated as immutable once published. Add AciListTests, which hammers getCandidateAcis() snapshots from several threads while a writer appends to the list under the queried key, resets it and renames a neighbouring key; validated to fail against an in-place-mutation regression and pass with copy-on-write. --- .../authorization/dseecompat/AciList.java | 37 ++-- .../dseecompat/AciListTests.java | 161 ++++++++++++++++++ 2 files changed, 180 insertions(+), 18 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/AciListTests.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java index cdfa55aeee..0784c49218 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/authorization/dseecompat/AciList.java @@ -27,7 +27,7 @@ import java.util.List; import java.util.Map; import java.util.SortedSet; -import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.concurrent.locks.ReentrantLock; import org.forgerock.i18n.LocalizableMessage; import org.forgerock.i18n.slf4j.LocalizedLogger; @@ -54,15 +54,16 @@ public class AciList { * copy and publish it through this volatile reference. A published map is * never modified in place, so getCandidateAcis() can read it lock-free — * it runs for every access-controlled operation, and even a read lock - * becomes a cross-core hotspot under load. + * becomes a cross-core hotspot under load. Aci instances themselves are + * treated as immutable once published: the map and its value lists are + * the only mutable containers. */ private volatile DITCacheMap> aciList = new DITCacheMap<>(); /** * Lock serializing mutators (ACI additions, removals and renames are rare). */ - private final ReentrantReadWriteLock lock = - new ReentrantReadWriteLock(); + private final ReentrantLock lock = new ReentrantLock(); /** The configuration DN used to compare against the global ACI entry DN. */ private final DN configDN; @@ -157,7 +158,7 @@ private DITCacheMap> copyAciList() public int addAci(List entries, LinkedList failedACIMsgs) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -174,7 +175,7 @@ public int addAci(List entries, } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -188,7 +189,7 @@ public int addAci(List entries, * */ public void addAci(DN dn, SortedSet acis) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -197,7 +198,7 @@ public void addAci(DN dn, SortedSet acis) { } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -215,7 +216,7 @@ public void addAci(DN dn, SortedSet acis) { public int addAci(Entry entry, boolean hasAci, boolean hasGlobalAci, List failedACIMsgs) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -239,7 +240,7 @@ public int addAci(Entry entry, boolean hasAci, } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -304,7 +305,7 @@ public void modAciOldNewEntry(Entry oldEntry, Entry newEntry, boolean hasAci, boolean hasGlobalAci) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -330,7 +331,7 @@ public void modAciOldNewEntry(Entry oldEntry, Entry newEntry, } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -367,7 +368,7 @@ private static void addAci(DITCacheMap> aciList, DN dn, */ public boolean removeAci(Entry entry, boolean hasAci, boolean hasGlobalAci) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -388,7 +389,7 @@ public boolean removeAci(Entry entry, boolean hasAci, } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -399,7 +400,7 @@ public boolean removeAci(Entry entry, boolean hasAci, */ public void removeAci(LocalBackend backend) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -417,7 +418,7 @@ public void removeAci(LocalBackend backend) { } finally { - lock.writeLock().unlock(); + lock.unlock(); } } @@ -429,7 +430,7 @@ public void removeAci(LocalBackend backend) { */ public void renameAci(DN oldDN, DN newDN ) { - lock.writeLock().lock(); + lock.lock(); try { DITCacheMap> copy = copyAciList(); @@ -463,7 +464,7 @@ public void renameAci(DN oldDN, DN newDN ) { } finally { - lock.writeLock().unlock(); + lock.unlock(); } } } diff --git a/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/AciListTests.java b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/AciListTests.java new file mode 100644 index 0000000000..e5ca1930e9 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/authorization/dseecompat/AciListTests.java @@ -0,0 +1,161 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions Copyright [year] [name of copyright owner]". + * + * Copyright 2026 3A Systems, LLC + */ +package org.opends.server.authorization.dseecompat; + +import static org.testng.Assert.*; + +import java.util.Collections; +import java.util.LinkedList; +import java.util.List; +import java.util.SortedSet; +import java.util.TreeSet; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; + +import org.forgerock.i18n.LocalizableMessage; +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; +import org.opends.server.DirectoryServerTestCase; +import org.opends.server.TestCaseUtils; +import org.opends.server.types.Entry; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +/** + * Locks in the copy-on-write contract of {@link AciList}: readers snapshot + * the map through the volatile reference without locking, so mutators must + * never modify a published map or its value lists in place. + */ +@SuppressWarnings("javadoc") +public class AciListTests extends DirectoryServerTestCase +{ + @BeforeClass + public void setUp() throws Exception + { + // The full server schema is needed for "aci" to be an operational + // attribute, which the Entry-based append path relies on. + TestCaseUtils.startServer(); + } + + @Test(timeOut = 60000) + public void testCandidateSnapshotsUnderConcurrentMutation() throws Exception + { + final DN aciDN = DN.valueOf("dc=example,dc=com"); + final DN queryDN = DN.valueOf("uid=user.0,ou=People,dc=example,dc=com"); + // Off the query path: churned to force structural map changes. + final DN movingDnA = DN.valueOf("ou=A,dc=example,dc=com"); + final DN movingDnB = DN.valueOf("ou=B,dc=example,dc=com"); + final AciList aciList = new AciList(DN.valueOf("cn=Access Control Handler,cn=config")); + + final Aci aci1 = Aci.decode(ByteString.valueOfUtf8( + "(version 3.0; acl \"cow test 1\"; allow(all) userdn=\"ldap:///anyone\";)"), aciDN); + final Aci aci2 = Aci.decode(ByteString.valueOfUtf8( + "(version 3.0; acl \"cow test 2\"; allow(read) userdn=\"ldap:///all\";)"), aciDN); + final SortedSet oneAci = new TreeSet<>(); + oneAci.add(aci1); + final SortedSet twoAcis = new TreeSet<>(); + twoAcis.add(aci1); + twoAcis.add(aci2); + + // Entry whose "aci" attribute is APPENDED to the existing list under + // aciDN by addAci(List, ...): with copy-on-write the append goes + // to a fresh copy; an in-place append would mutate the published list + // under the readers' fail-fast iterators. + final Entry appendEntry = TestCaseUtils.makeEntry( + "dn: " + aciDN, + "objectClass: top", + "objectClass: domain", + "dc: example", + "aci: (version 3.0; acl \"cow test 2\"; allow(read) userdn=\"ldap:///all\";)"); + + aciList.addAci(aciDN, oneAci); + aciList.addAci(movingDnA, oneAci); + + // Sanity-check the append channel before relying on it for churn. + final LinkedList failedACIMsgs = new LinkedList<>(); + aciList.addAci(Collections.singletonList(appendEntry), failedACIMsgs); + assertTrue(failedACIMsgs.isEmpty(), String.valueOf(failedACIMsgs)); + assertEquals(aciList.getCandidateAcis(queryDN).size(), 2, + "addAci(List) must append to the list under aciDN"); + aciList.addAci(aciDN, oneAci); + + final AtomicBoolean done = new AtomicBoolean(); + final AtomicReference failure = new AtomicReference<>(); + Thread[] readers = new Thread[3]; + for (int i = 0; i < readers.length; i++) + { + readers[i] = new Thread("AciList COW reader " + i) + { + @Override + public void run() + { + try + { + while (!done.get()) + { + // Iterates the published value lists: an in-place mutator + // would make this throw or return a torn snapshot. + List candidates = aciList.getCandidateAcis(queryDN); + int size = candidates.size(); + if (size != 1 && size != 2) + { + throw new AssertionError("Torn candidate snapshot: " + candidates); + } + } + } + catch (Throwable t) + { + failure.compareAndSet(null, t); + } + } + }; + readers[i].start(); + } + + try + { + for (int i = 0; i < 50000 && failure.get() == null; i++) + { + // Append a second ACI to the list under aciDN, then reset to one. + aciList.addAci(Collections.singletonList(appendEntry), failedACIMsgs); + aciList.addAci(aciDN, oneAci); + // Structural churn off the query path: rename moves the key + // through iterator.remove() and putAll() on every iteration. + if (i % 2 == 0) + { + aciList.renameAci(movingDnA, movingDnB); + } + else + { + aciList.renameAci(movingDnB, movingDnA); + } + assertTrue(failedACIMsgs.isEmpty(), String.valueOf(failedACIMsgs)); + } + } + finally + { + done.set(true); + for (Thread reader : readers) + { + reader.join(10000); + } + } + if (failure.get() != null) + { + fail("Reader failed under concurrent ACI mutation", failure.get()); + } + } +}