From 432af2c2e193d73fdcd071afdcc517b16090227b Mon Sep 17 00:00:00 2001 From: Adam Yi Date: Thu, 16 Jul 2026 00:40:46 +0800 Subject: [PATCH] ZOOKEEPER-4689: Fix ACL cache recovery from fuzzy snapshots ZooKeeper serializes the ACL cache before the data tree while taking a fuzzy snapshot. A node can therefore be serialized with an ACL id which is absent from the serialized cache. Transaction replay repairs these references using the ACL value carried by the transaction (ZOOKEEPER-3306 and ZOOKEEPER-4846). Three issues remained: * On a fresh restore, aclIndex accounted only for ids in the serialized cache. Replay could reissue an id referenced by a snapshot node to a different ACL. A delete of a node still carrying that stale id could then decrement and remove an unrelated live ACL entry. Advance aclIndex past every id referenced by the loaded tree, including ids absent from the cache. * The existing-node create path incremented the new ACL's reference count without releasing the node's previous reference. Transfer the reference under the node lock so each node contributes exactly one count. * ZOOKEEPER-4799 made createNode, deleteNode, and setData resolve ACLs before triggering watches. During restore the watch tables are normally empty, but a dangling ACL still threw before the watch manager was reached and aborted replay. Use a non-throwing lookup only for watch filtering. Represent an unknown ACL with a no-permissions ACL so any unexpected delivery fails closed; null and empty ACLs are treated as unrestricted by checkACL. Client ACL lookups remain strict. The change requires no snapshot format change and does not require ACL ids to remain equal across servers or restarts. --- .../org/apache/zookeeper/server/DataTree.java | 68 ++- .../server/ReferenceCountedACLCache.java | 27 ++ .../apache/zookeeper/server/DataTreeTest.java | 29 ++ .../persistence/FileTxnSnapLogTest.java | 400 ++++++++++++++++-- 4 files changed, 477 insertions(+), 47 deletions(-) diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java index 1f93f18c778..cee7627bcd4 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java @@ -96,6 +96,11 @@ public class DataTree { private static final Logger LOG = LoggerFactory.getLogger(DataTree.class); + // An unknown ACL must fail closed when filtering watch events. In + // particular, null and an empty list are treated as unrestricted ACLs. + private static final List NO_PERMISSIONS_ACL = + Collections.singletonList(new ACL(0, ZooDefs.Ids.ANYONE_ID_UNSAFE)); + private final RateLogger RATE_LOGGER = new RateLogger(LOG, 15 * 60 * 1000); /** @@ -441,24 +446,27 @@ public void createNode(final String path, byte[] data, List acl, long ephem } List parentAcl; synchronized (parent) { - parentAcl = getACL(parent); + parentAcl = getACLForWatchTrigger(parent, parentName); - // Add the ACL to ACL cache first, to avoid the ACL not being - // created race condition during fuzzy snapshot sync. - // - // This is the simplest fix, which may add ACL reference count - // again if it's already counted in the ACL map of fuzzy - // snapshot, which might also happen for deleteNode txn, but - // at least it won't cause the ACL not exist issue. - // - // Later we can audit and delete all non-referenced ACLs from - // ACL map when loading the snapshot/txns from disk, like what - // we did for the global sessions. - Long acls = aclCache.convertAcls(acl); + Long aclId = aclCache.convertAcls(acl); DataNode existingChild = nodes.get(path); if (existingChild != null) { - existingChild.acl = acls; + // A create txn can be replayed after its node was already + // serialized into a fuzzy snapshot. The txn carries the + // authoritative ACL value, whereas the id in the snapshot may + // be absent from the serialized ACL cache. Move the node to the + // locally interned id and release its previous reference. This + // also balances convertAcls above when both ids are equal. + synchronized (existingChild) { + Long previousAcl = existingChild.acl; + existingChild.acl = aclId; + aclCache.removeUsage(previousAcl); + if (!aclId.equals(previousAcl)) { + LOG.info("ACL reference of existing node {} changed from {} to {} while applying a create txn", + path, previousAcl, aclId); + } + } throw new NodeExistsException(); } @@ -476,7 +484,7 @@ public void createNode(final String path, byte[] data, List acl, long ephem parent.stat.setCversion(parentCVersion); parent.stat.setPzxid(zxid); } - DataNode child = new DataNode(data, acls, stat); + DataNode child = new DataNode(data, aclId, stat); parent.addChild(childName); nodes.postChange(parentName, parent); nodeDataSize.addAndGet(getNodeSize(path, child.data)); @@ -562,7 +570,7 @@ public void deleteNode(String path, long zxid) throws NoNodeException { List acl; nodes.remove(path); synchronized (node) { - acl = getACL(node); + acl = getACLForWatchTrigger(node, path); aclCache.removeUsage(node.acl); nodeDataSize.addAndGet(-getNodeSize(path, node.data)); } @@ -572,7 +580,7 @@ public void deleteNode(String path, long zxid) throws NoNodeException { // separate patch. List parentAcl; synchronized (parent) { - parentAcl = getACL(parent); + parentAcl = getACLForWatchTrigger(parent, parentName); long owner = node.stat.getEphemeralOwner(); EphemeralType ephemeralType = EphemeralType.get(owner); if (ephemeralType == EphemeralType.CONTAINER) { @@ -635,7 +643,7 @@ public Stat setData(String path, byte[] data, int version, long zxid, long time) List acl; byte[] lastData; synchronized (n) { - acl = getACL(n); + acl = getACLForWatchTrigger(n, path); lastData = n.data; nodes.preChange(path, n); n.data = data; @@ -792,6 +800,30 @@ public List getACL(DataNode node) { } } + /** + * Returns the ACL used to filter watch events for a node. A fuzzy snapshot + * can temporarily contain a node whose ACL id is absent from the serialized + * ACL cache. Transaction replay repairs that reference; until then, use a + * no-permissions ACL instead of aborting replay. During normal restore the + * watch tables are empty, so the fallback has no watch-delivery effect. If + * the state is encountered while watches are present, it fails closed + * (ZOOKEEPER-4799). + */ + private List getACLForWatchTrigger(DataNode node, String path) { + Long aclId; + List acls; + synchronized (node) { + aclId = node.acl; + acls = aclCache.convertLongIfCached(aclId); + } + if (acls != null) { + return acls; + } + LOG.info("ACL id {} of {} is not in the ACL cache; filtering its watch events as unreadable", + aclId, path); + return NO_PERMISSIONS_ACL; + } + public int aclCacheSize() { return aclCache.size(); } diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java index 78977000ca2..27d02ff18b1 100644 --- a/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java @@ -95,6 +95,24 @@ public synchronized List convertLong(Long longVal) { return acls; } + /** + * Converts a long to a list of ACLs without throwing if it is absent from + * the cache. A node in a fuzzy snapshot can temporarily reference such an + * absent id until transaction replay repairs it (ZOOKEEPER-4689). + * + * @param longVal ACL id + * @return the corresponding ACLs, or null if the id is not cached + */ + synchronized List convertLongIfCached(Long longVal) { + if (longVal == null) { + return null; + } + if (longVal == OPEN_UNSAFE_ACL_ID) { + return ZooDefs.Ids.OPEN_ACL_UNSAFE; + } + return longKeyMap.get(longVal); + } + private long incrementIndex() { return ++aclIndex; } @@ -170,6 +188,15 @@ public synchronized void addUsage(Long acl) { return; } + if (acl > aclIndex) { + // The ACL cache is serialized before the nodes in a fuzzy snapshot. + // A node can therefore reference an id absent from the serialized + // cache. Reserve every id mentioned by the loaded tree so replay + // cannot assign it to a different ACL before repairing the node. + LOG.info("Advancing aclIndex from {} to {} based on a node reference in the snapshot", aclIndex, acl); + aclIndex = acl; + } + if (!longKeyMap.containsKey(acl)) { LOG.info("Ignoring acl {} as it does not exist in the cache", acl); return; diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java index 89a2f17b75f..82ce95a6ffd 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/DataTreeTest.java @@ -51,6 +51,7 @@ import org.apache.zookeeper.data.Stat; import org.apache.zookeeper.metrics.MetricsUtils; import org.apache.zookeeper.txn.CreateTxn; +import org.apache.zookeeper.txn.DeleteTxn; import org.apache.zookeeper.txn.TxnHeader; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; @@ -647,6 +648,34 @@ public void testCreateNodeFixMissingACL() throws Exception { dt.createNode("/the_parent/the_child", new byte[0], ZooDefs.Ids.CREATOR_ALL_ACL, -1, 2, 2, 2); } + /** + * Applying a create txn to an existing node (routine when txns overlap + * with the state restored from a fuzzy snapshot, e.g. on DIFF sync) must + * keep the ACL reference counts exact, so that unused ACLs are garbage + * collected and the cache does not grow until the next reload. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testCreateTxnReplayOnExistingNodeKeepsAclRefCountExact() throws Exception { + DataTree dt = new DataTree(); + int baseline = dt.aclCacheSize(); + + TxnHeader hdr = new TxnHeader(1, 2, 2, 2, ZooDefs.OpCode.create); + Record txn = new CreateTxn("/x", new byte[0], ZooDefs.Ids.CREATOR_ALL_ACL, false, -1); + dt.processTxn(hdr, txn); + // Duplicate apply of the same create txn hits the node-exists branch. + dt.processTxn(hdr, txn); + assertEquals(baseline + 1, dt.aclCacheSize()); + + TxnHeader deleteHdr = new TxnHeader(1, 2, 3, 2, ZooDefs.OpCode.delete); + Record deleteTxn = new DeleteTxn("/x"); + dt.processTxn(deleteHdr, deleteTxn); + // The ACL is no longer referenced by any node, so it must have been + // garbage collected. + assertEquals(baseline, dt.aclCacheSize()); + } + private DataTree buildDataTreeForTest() { final DataTree dt = new DataTree(); assertEquals(dt.lastProcessedZxid, 0); diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java index 656eeb8a0aa..f2216b2a133 100644 --- a/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java +++ b/zookeeper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java @@ -30,6 +30,8 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import org.apache.jute.BinaryInputArchive; @@ -37,14 +39,19 @@ import org.apache.jute.InputArchive; import org.apache.jute.OutputArchive; import org.apache.jute.Record; +import org.apache.zookeeper.AddWatchMode; +import org.apache.zookeeper.WatchedEvent; import org.apache.zookeeper.ZooDefs; +import org.apache.zookeeper.data.ACL; import org.apache.zookeeper.server.DataNode; import org.apache.zookeeper.server.DataTree; import org.apache.zookeeper.server.Request; +import org.apache.zookeeper.server.ServerWatcher; import org.apache.zookeeper.server.ZooKeeperServer; import org.apache.zookeeper.test.ClientBase; import org.apache.zookeeper.test.TestUtils; import org.apache.zookeeper.txn.CreateTxn; +import org.apache.zookeeper.txn.DeleteTxn; import org.apache.zookeeper.txn.SetDataTxn; import org.apache.zookeeper.txn.TxnDigest; import org.apache.zookeeper.txn.TxnHeader; @@ -304,6 +311,86 @@ public void testDirCheckWithLogFilesInSnapDir() throws IOException { }); } + private static final byte[] TEST_DATA = "foo".getBytes(); + + private static final class ReplayTxn { + + private final TxnHeader header; + private final Record record; + + ReplayTxn(long zxid, int type, Record record) { + this.header = new TxnHeader(1, 2, zxid, 2, type); + this.record = record; + } + + void applyTo(DataTree dataTree) { + dataTree.processTxn(header, record); + } + } + + private static ReplayTxn createTxn(long zxid, String path, List acl) { + return new ReplayTxn(zxid, ZooDefs.OpCode.create, new CreateTxn(path, TEST_DATA, acl, false, -1)); + } + + private static ReplayTxn deleteTxn(long zxid, String path) { + return new ReplayTxn(zxid, ZooDefs.OpCode.delete, new DeleteTxn(path)); + } + + private static ReplayTxn setDataTxn(long zxid, String path) { + return new ReplayTxn(zxid, ZooDefs.OpCode.setData, new SetDataTxn(path, TEST_DATA, 1)); + } + + /** + * Simulates a fuzzy snapshot: the ACL cache is serialized when the + * snapshot starts, and the nodes are serialized when it finishes, so + * transactions applied in between can leave nodes referencing ACL ids + * which are absent from the serialized cache. + */ + private static final class FuzzySnapshot { + + private final DataTree dataTree; + private final File file; + private final FileOutputStream outputStream; + private final OutputArchive outputArchive; + + private FuzzySnapshot(DataTree dataTree) throws IOException { + this.dataTree = dataTree; + this.file = File.createTempFile("snapshot", "zk"); + this.outputStream = new FileOutputStream(file); + this.outputArchive = BinaryOutputArchive.getArchive(outputStream); + } + + static FuzzySnapshot start(DataTree dataTree) throws IOException { + FuzzySnapshot snapshot = new FuzzySnapshot(dataTree); + dataTree.serializeAcls(snapshot.outputArchive); + return snapshot; + } + + DataTree finishAndRestore() throws IOException { + dataTree.serializeNodes(outputArchive); + outputStream.close(); + + DataTree restored = new DataTree(); + try (FileInputStream inputStream = new FileInputStream(file)) { + InputArchive inputArchive = BinaryInputArchive.getArchive(inputStream); + restored.deserialize(inputArchive, "tree"); + } + return restored; + } + } + + private static List concatAcl(List first, List second) { + List result = new ArrayList<>(first); + result.addAll(second); + return result; + } + + private static void assertAcl(DataTree dataTree, String path, List expected) { + DataNode node = dataTree.getNode(path); + assertNotNull(node); + assertEquals(expected, dataTree.getACL(node)); + } + /** * Make sure the ACL is exist in the ACL map after SNAP syncing. * @@ -332,35 +419,290 @@ public void testDirCheckWithLogFilesInSnapDir() throws IOException { */ @Test public void testACLCreatedDuringFuzzySnapshotSync() throws IOException { - DataTree leaderDataTree = new DataTree(); - - // Start the simulated snap-sync by serializing ACL cache. - File file = File.createTempFile("snapshot", "zk"); - FileOutputStream os = new FileOutputStream(file); - OutputArchive oa = BinaryOutputArchive.getArchive(os); - leaderDataTree.serializeAcls(oa); - - // Add couple of transaction in-between. - TxnHeader hdr1 = new TxnHeader(1, 2, 2, 2, ZooDefs.OpCode.create); - Record txn1 = new CreateTxn("/a1", "foo".getBytes(), ZooDefs.Ids.CREATOR_ALL_ACL, false, -1); - leaderDataTree.processTxn(hdr1, txn1); - - // Finish the snapshot. - leaderDataTree.serializeNodes(oa); - os.close(); - - // Simulate restore on follower and replay. - FileInputStream is = new FileInputStream(file); - InputArchive ia = BinaryInputArchive.getArchive(is); - DataTree followerDataTree = new DataTree(); - followerDataTree.deserialize(ia, "tree"); - followerDataTree.processTxn(hdr1, txn1); - - DataNode a1 = leaderDataTree.getNode("/a1"); - assertNotNull(a1); - assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, leaderDataTree.getACL(a1)); - - assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, followerDataTree.getACL(a1)); + DataTree leader = new DataTree(); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + ReplayTxn create = createTxn(2, "/a1", ZooDefs.Ids.CREATOR_ALL_ACL); + create.applyTo(leader); + + DataTree restored = snapshot.finishAndRestore(); + create.applyTo(restored); + + assertAcl(leader, "/a1", ZooDefs.Ids.CREATOR_ALL_ACL); + // ACL ids are server-local, so resolve the restored tree's own node. + assertAcl(restored, "/a1", ZooDefs.Ids.CREATOR_ALL_ACL); + } + + /** + * ACL ids referenced by a fuzzy snapshot must not be re-issued to other + * ACL lists while the transactions of the fuzzy range are replayed. + * + * A node can be serialized with a reference to an ACL id which was + * interned only after the ACL cache had been serialized. Such an id is + * missing from the deserialized cache, so it is not accounted for in + * aclIndex. If the id is re-issued to a different ACL list during replay, + * a replayed delete txn of an unrelated node still referencing the id + * decrements a reference count that node never contributed to, and the + * entry is garbage collected while live nodes still point to it. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testAclIdsReferencedBySnapshotAreNotReissuedDuringReplay() throws IOException { + DataTree leader = new DataTree(); + + // Leave a gap between aclIndex and the highest live ACL id. + createTxn(2, "/m", ZooDefs.Ids.CREATOR_ALL_ACL).applyTo(leader); + List discardedAcl = concatAcl(ZooDefs.Ids.CREATOR_ALL_ACL, ZooDefs.Ids.OPEN_ACL_UNSAFE); + createTxn(3, "/gone", discardedAcl).applyTo(leader); + deleteTxn(4, "/gone").applyTo(leader); + + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + // These ACL ids are absent from the serialized cache but referenced by + // the serialized nodes. Before the fix, replay re-issued /m's stale id + // to /b; deleting /m then garbage-collected /b's live ACL entry. + List aclB = concatAcl(ZooDefs.Ids.READ_ACL_UNSAFE, ZooDefs.Ids.CREATOR_ALL_ACL); + ReplayTxn createA = createTxn(5, "/a", ZooDefs.Ids.OPEN_ACL_UNSAFE); + ReplayTxn createB = createTxn(6, "/b", aclB); + ReplayTxn deleteM = deleteTxn(7, "/m"); + ReplayTxn recreateM = createTxn(8, "/m", ZooDefs.Ids.OPEN_ACL_UNSAFE); + createA.applyTo(leader); + createB.applyTo(leader); + deleteM.applyTo(leader); + recreateM.applyTo(leader); + + DataTree restored = snapshot.finishAndRestore(); + createA.applyTo(restored); + createB.applyTo(restored); + deleteM.applyTo(restored); + recreateM.applyTo(restored); + + for (DataTree dataTree : new DataTree[]{leader, restored}) { + assertAcl(dataTree, "/a", ZooDefs.Ids.OPEN_ACL_UNSAFE); + assertAcl(dataTree, "/b", aclB); + assertAcl(dataTree, "/m", ZooDefs.Ids.OPEN_ACL_UNSAFE); + } + assertEquals(leader.aclCacheSize(), restored.aclCacheSize()); + } + + /** + * Replaying a delete txn on top of a fuzzy snapshot which contains the + * re-created node with a dangling ACL reference must not crash the + * restore. + * + * The eager ACL fetch for watch triggering (ZOOKEEPER-4799) made such + * dangling references fatal in deleteNode; the reference is repaired only + * when the re-creating txn is replayed later (ZOOKEEPER-4846), so the + * delete has to tolerate it. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testDeleteTxnReplayOnDanglingAclDoesNotCrash() throws IOException { + DataTree leader = new DataTree(); + createTxn(2, "/x", ZooDefs.Ids.CREATOR_ALL_ACL).applyTo(leader); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + ReplayTxn delete = deleteTxn(3, "/x"); + ReplayTxn recreate = createTxn(4, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + delete.applyTo(leader); + recreate.applyTo(leader); + + // The snapshot contains the re-created /x but not its ACL. Replay + // first applies the old incarnation's delete to that dangling node. + DataTree restored = snapshot.finishAndRestore(); + delete.applyTo(restored); + recreate.applyTo(restored); + + assertAcl(restored, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + } + + /** + * Replaying a setData txn of an earlier incarnation on top of a fuzzy + * snapshot which contains the re-created node with a dangling ACL + * reference must not crash the restore. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testSetDataTxnReplayOnDanglingAclDoesNotCrash() throws IOException { + DataTree leader = new DataTree(); + createTxn(2, "/x", ZooDefs.Ids.CREATOR_ALL_ACL).applyTo(leader); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + ReplayTxn setData = setDataTxn(3, "/x"); + ReplayTxn delete = deleteTxn(4, "/x"); + ReplayTxn recreate = createTxn(5, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + setData.applyTo(leader); + delete.applyTo(leader); + recreate.applyTo(leader); + + // Replay first applies the old incarnation's setData to the re-created + // /x whose ACL is absent from the serialized cache. + DataTree restored = snapshot.finishAndRestore(); + setData.applyTo(restored); + delete.applyTo(restored); + recreate.applyTo(restored); + + assertAcl(restored, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + } + + /** + * Watch events for a node whose ACL reference is missing from the ACL + * cache are filtered as unreadable (fail closed), not delivered without + * an ACL check: delivering them without a check would reintroduce + * CVE-2024-23944 (see ZOOKEEPER-4799) for such nodes. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testWatchEventsForDanglingAclFailClosed() throws IOException { + DataTree leader = new DataTree(); + createTxn(2, "/x", ZooDefs.Ids.CREATOR_ALL_ACL).applyTo(leader); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + ReplayTxn delete = deleteTxn(3, "/x"); + ReplayTxn recreate = createTxn(4, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + delete.applyTo(leader); + recreate.applyTo(leader); + DataTree restored = snapshot.finishAndRestore(); + + List> deliveredAcls = new ArrayList<>(); + ServerWatcher watcher = new ServerWatcher() { + @Override + public void process(WatchedEvent event) { + } + + @Override + public void process(WatchedEvent event, List znodeAcl) { + deliveredAcls.add(znodeAcl); + } + }; + restored.addWatch("/x", watcher, AddWatchMode.PERSISTENT.getMode()); + + // Replaying the old incarnation's delete fires a watch event while + // the snapshot node's ACL is still unknown. + delete.applyTo(restored); + + assertFalse(deliveredAcls.isEmpty()); + for (List acl : deliveredAcls) { + assertNotNull(acl, "null ACLs pass checkACL"); + assertFalse(acl.isEmpty(), "empty ACLs pass checkACL"); + for (ACL entry : acl) { + assertEquals(0, entry.getPerms() & ZooDefs.Perms.READ, + "watch events with an unknown ACL must fail closed: " + acl); + } + } + } + + /** + * Replaying a create txn whose parent has a dangling ACL reference must + * not crash the restore. + * + * Same as above, but for the eager parent ACL fetch in createNode: the + * parent is repaired only when its own re-creating txn is replayed, which + * happens after the replay of create txns of an earlier incarnation's + * children. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testCreateTxnReplayUnderDanglingAclParentDoesNotCrash() throws IOException { + DataTree leader = new DataTree(); + createTxn(2, "/p", ZooDefs.Ids.CREATOR_ALL_ACL).applyTo(leader); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + ReplayTxn createChild = createTxn(3, "/p/c", ZooDefs.Ids.CREATOR_ALL_ACL); + ReplayTxn deleteChild = deleteTxn(4, "/p/c"); + ReplayTxn deleteParent = deleteTxn(5, "/p"); + ReplayTxn recreateParent = createTxn(6, "/p", ZooDefs.Ids.OPEN_ACL_UNSAFE); + createChild.applyTo(leader); + deleteChild.applyTo(leader); + deleteParent.applyTo(leader); + recreateParent.applyTo(leader); + + // The snapshot contains the re-created /p with a dangling ACL. Replay + // first creates an old incarnation's child beneath that parent. + DataTree restored = snapshot.finishAndRestore(); + createChild.applyTo(restored); + deleteChild.applyTo(restored); + deleteParent.applyTo(restored); + recreateParent.applyTo(restored); + + assertAcl(restored, "/p", ZooDefs.Ids.OPEN_ACL_UNSAFE); + assertNull(restored.getNode("/p/c")); + } + + /** + * Reference counting of ACLs interned during the fuzzy range works + * correctly and entries are not removed prematurely. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testAclRefCountDuringFuzzySnapshotSync() throws IOException { + DataTree leader = new DataTree(); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + ReplayTxn createA1 = createTxn(2, "/a1", ZooDefs.Ids.CREATOR_ALL_ACL); + ReplayTxn createA2 = createTxn(3, "/a2", ZooDefs.Ids.CREATOR_ALL_ACL); + ReplayTxn deleteA1 = deleteTxn(4, "/a1"); + createA1.applyTo(leader); + createA2.applyTo(leader); + deleteA1.applyTo(leader); + + DataTree restored = snapshot.finishAndRestore(); + createA1.applyTo(restored); + createA2.applyTo(restored); + deleteA1.applyTo(restored); + + assertAcl(leader, "/a2", ZooDefs.Ids.CREATOR_ALL_ACL); + assertAcl(restored, "/a2", ZooDefs.Ids.CREATOR_ALL_ACL); + assertEquals(leader.aclCacheSize(), restored.aclCacheSize()); + } + + /** + * Nodes repaired during replay end up with the ACL values carried by + * their create txns, also when the ids assigned during replay differ from + * the ids recorded in the snapshot. + * + * The repair must be keyed by the ACL value in the txn, never by the + * stale id recorded in the snapshot. Without id reservation, replay + * assigns /a4's stale id to /a3's ACL, so a repair keyed on whether that + * id exists in the cache (the approach of the original PR for this + * issue) silently gives /a4 the ACL of /a3. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testAclValuesMatchAfterFuzzySnapshotSyncReplay() throws IOException { + DataTree leader = new DataTree(); + + // Leave a gap between aclIndex and the serialized cache. + createTxn(2, "/gone", ZooDefs.Ids.OPEN_ACL_UNSAFE).applyTo(leader); + deleteTxn(3, "/gone").applyTo(leader); + FuzzySnapshot snapshot = FuzzySnapshot.start(leader); + + List otherAcl = concatAcl(ZooDefs.Ids.CREATOR_ALL_ACL, ZooDefs.Ids.READ_ACL_UNSAFE); + ReplayTxn createA2 = createTxn(4, "/a2", ZooDefs.Ids.CREATOR_ALL_ACL); + ReplayTxn createA3 = createTxn(5, "/a3", otherAcl); + ReplayTxn createA4 = createTxn(6, "/a4", ZooDefs.Ids.CREATOR_ALL_ACL); + createA2.applyTo(leader); + createA3.applyTo(leader); + createA4.applyTo(leader); + + DataTree restored = snapshot.finishAndRestore(); + createA2.applyTo(restored); + createA3.applyTo(restored); + createA4.applyTo(restored); + + for (DataTree dataTree : new DataTree[]{leader, restored}) { + assertAcl(dataTree, "/a2", ZooDefs.Ids.CREATOR_ALL_ACL); + assertAcl(dataTree, "/a3", otherAcl); + assertAcl(dataTree, "/a4", ZooDefs.Ids.CREATOR_ALL_ACL); + } + assertEquals(leader.aclCacheSize(), restored.aclCacheSize()); } @Test