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 20402156fe7..0fad72eb0ef 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); + // Deny watch delivery to ordinary sessions when a node's ACL is unknown. + // checkACL treats null and empty ACLs as unrestricted. Public for tests. + public 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,26 @@ 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); + // Intern the ACL before checking whether the node exists: a fuzzy + // snapshot can contain the node but omit its ACL (ZOOKEEPER-3306). + Long aclId = aclCache.convertAcls(acl); DataNode existingChild = nodes.get(path); if (existingChild != null) { - existingChild.acl = acls; + // Use the ACL value from the txn; snapshot ids are server-local + // and may not resolve. Release the node's old reference to balance + // convertAcls, including 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 +483,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 +569,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 +579,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 +642,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 +799,27 @@ public List getACL(DataNode node) { } } + /** + * Returns the ACL for filtering a node's watch events. A node loaded from a + * fuzzy snapshot can reference an ACL id that is missing from the cache + * until replay repairs it. In that case this returns + * {@link #NO_PERMISSIONS_ACL} instead of throwing, so replay can continue. + */ + 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..baadca40fa4 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 @@ -33,6 +33,8 @@ import java.io.PrintWriter; import java.io.StringWriter; import java.lang.reflect.Field; +import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Semaphore; @@ -42,15 +44,18 @@ import org.apache.jute.BinaryOutputArchive; import org.apache.jute.InputArchive; import org.apache.jute.Record; +import org.apache.zookeeper.KeeperException.NoAuthException; import org.apache.zookeeper.KeeperException.NoNodeException; import org.apache.zookeeper.KeeperException.NodeExistsException; import org.apache.zookeeper.Quotas; import org.apache.zookeeper.ZKTestCase; import org.apache.zookeeper.ZooDefs; import org.apache.zookeeper.common.PathTrie; +import org.apache.zookeeper.data.Id; 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 +652,49 @@ 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 replay overlaps + * a fuzzy snapshot, e.g. after a restart or SNAP 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()); + } + + /** + * The ACL used to filter watch events of a node whose ACL is unknown must + * deny READ to ordinary sessions. Null or an empty ACL would not, because + * checkACL treats both as unrestricted. + * + * See ZOOKEEPER-4689. + */ + @Test + public void testNoPermissionsAclDeniesRead() { + ZooKeeperServer zks = new ZooKeeperServer(); + List authInfo = Collections.singletonList(new Id("ip", "127.0.0.1")); + assertThrows(NoAuthException.class, () -> + zks.checkACL(null, DataTree.NO_PERMISSIONS_ACL, ZooDefs.Perms.READ, authInfo, "/x", null)); + } + 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..e5488c957cd 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,94 @@ 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. {@link #start} serializes the ACL cache, and + * {@link #finishAndRestore} serializes the nodes and loads them into a new + * tree. Transactions passed to {@link #apply} in between are in the fuzzy + * range, so serialized nodes can reference ACL ids that are missing 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, File file) throws IOException { + this.dataTree = dataTree; + this.file = file; + this.outputStream = new FileOutputStream(file); + this.outputArchive = BinaryOutputArchive.getArchive(outputStream); + } + + /** Serializes the ACL cache to a snapshot file under {@code dir}. */ + static FuzzySnapshot start(DataTree dataTree, File dir) throws IOException { + FuzzySnapshot snapshot = new FuzzySnapshot(dataTree, File.createTempFile("snapshot", ".zk", dir)); + dataTree.serializeAcls(snapshot.outputArchive); + return snapshot; + } + + /** Applies a transaction to the source tree during the fuzzy range. */ + void apply(ReplayTxn txn) { + txn.applyTo(dataTree); + } + + /** Serializes the nodes and loads the snapshot into a new tree without replaying transactions. */ + 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 +427,296 @@ 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, tmpDir); + + ReplayTxn create = createTxn(2, "/a1", ZooDefs.Ids.CREATOR_ALL_ACL); + snapshot.apply(create); + + 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, tmpDir); + + // 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); + snapshot.apply(createA); + snapshot.apply(createB); + snapshot.apply(deleteM); + snapshot.apply(recreateM); + + 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, tmpDir); + + ReplayTxn delete = deleteTxn(3, "/x"); + ReplayTxn recreate = createTxn(4, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + snapshot.apply(delete); + snapshot.apply(recreate); + + // 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, tmpDir); + + ReplayTxn setData = setDataTxn(3, "/x"); + ReplayTxn delete = deleteTxn(4, "/x"); + ReplayTxn recreate = createTxn(5, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + snapshot.apply(setData); + snapshot.apply(delete); + snapshot.apply(recreate); + + // 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 is missing from the cache are filtered + * with {@link DataTree#NO_PERMISSIONS_ACL}, which denies READ. Null or an + * empty ACL would allow delivery, reintroducing CVE-2024-23944 + * (ZOOKEEPER-4799). Once replay repairs the node, its events carry the + * restored ACL again. + * + * 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, tmpDir); + + ReplayTxn delete = deleteTxn(3, "/x"); + ReplayTxn recreate = createTxn(4, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + snapshot.apply(delete); + snapshot.apply(recreate); + 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) { + assertEquals(DataTree.NO_PERMISSIONS_ACL, acl); + } + + // Finish replay and check that the next watch event carries the restored ACL. + deliveredAcls.clear(); + recreate.applyTo(restored); + + assertAcl(restored, "/x", ZooDefs.Ids.OPEN_ACL_UNSAFE); + assertFalse(deliveredAcls.isEmpty()); + for (List acl : deliveredAcls) { + assertEquals(ZooDefs.Ids.OPEN_ACL_UNSAFE, acl); + } + } + + /** + * Replaying a create txn whose parent has a dangling ACL reference must + * not crash the restore. + * + * createNode eagerly fetches the parent's ACL for watch triggering + * (ZOOKEEPER-4799). The parent is repaired only when its own re-creating + * txn is replayed, after the 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, tmpDir); + + 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); + snapshot.apply(createChild); + snapshot.apply(deleteChild); + snapshot.apply(deleteParent); + snapshot.apply(recreateParent); + + // 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, tmpDir); + + 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"); + snapshot.apply(createA1); + snapshot.apply(createA2); + snapshot.apply(deleteA1); + + 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, tmpDir); + + 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); + snapshot.apply(createA2); + snapshot.apply(createA3); + snapshot.apply(createA4); + + 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