ZOOKEEPER-4689: Fix ACL cache recovery from fuzzy snapshots

[adamyi]

Supersedes #1997.

Background

ZooKeeper snapshots are fuzzy: the ACL cache is serialized first, and the data tree is serialized afterward while transactions continue to modify both. A snapshot can therefore contain a node whose ACL id is absent from the serialized ACL cache:

serialize ACL cache
create /x with a new ACL id
serialize /x

This is recoverable because the transaction which introduced the ACL is newer than the snapshot zxid and is replayed after the snapshot is loaded.

ZOOKEEPER-3306 made create replay intern the transaction's ACL even when the node already existed in the snapshot. ZOOKEEPER-4846 completed the basic repair by moving that existing node to the locally interned id. Using the ACL value carried by the transaction is important: numeric ACL ids are local cache implementation details and need not remain equal across servers or restarts.

Three issues remain on current master. They are fixed together deliberately: reserving snapshot-referenced ids (fix 1) keeps those ids unresolved until their repairing txns replay, which is exactly the state the watch-time ACL lookup (fix 3) must tolerate. None of this changes the snapshot format, and ACL ids are never required to match across servers.

1. An id referenced by the snapshot can be reissued to another ACL

On a fresh restore, ReferenceCountedACLCache.deserialize advances aclIndex only from ids present in the serialized cache. It does not account for ids which appear only in the nodes serialized later.

For example, suppose the serialized cache ends at id 2, while fuzzy snapshot nodes refer to ids 4 and 5. Replay can allocate id 4 to a different ACL before all snapshot nodes have been repaired. A replayed delete of a node still carrying stale id 4 then decrements the new entry's reference count. That entry can be garbage-collected while an unrelated live node still points to it, leaving that node with a dangling ACL after its repairing transaction has already replayed.

This patch advances aclIndex for every id referenced by a deserialized node, including ids absent from the cache. Such ids are never reissued; replay moves the nodes to locally interned ids derived from the ACL values in their transactions.

The invariant is local—this does not try to keep ACL ids equal across ensemble members:

An ACL id referenced by the loaded tree is never reissued to another ACL value.

2. Create replay leaves reference counts inflated

In the ZOOKEEPER-4846 path, convertAcls increments the new entry's reference count before the existing node is updated, but the node's previous reference is not released. This leaks a reference even when the old and new ids are equal, and prevents unused ACL entries from being collected until the next reload rebuilds the counts.

The existing-node path now transfers the reference under the node lock:

intern transaction ACL (+1)
replace node ACL id
release previous ACL id (-1, or no-op if absent)

When both ids are equal, the increment and decrement balance. When they differ, the node contributes exactly one reference to its new entry.

3. ACL lookup for watch filtering can abort replay

ZOOKEEPER-4799 added eager ACL lookup in createNode, deleteNode, and setData so watch events can be filtered by READ permission. During normal restore and learner synchronization, transactions are replayed before the server starts serving clients, so the watch tables are normally empty. Nevertheless, these methods resolve the ACL before calling the watch manager.

One example is a node deleted and re-created in the fuzzy range. The snapshot can contain the new incarnation with an ACL absent from the serialized cache, while replay begins with setData or delete transactions for the old incarnation. The eager lookup throws Failed to fetch acls for ... and aborts recovery before the empty watch manager is consulted; the later create transaction never gets a chance to repair the reference.

This patch uses a non-throwing lookup only for ACLs passed to watch delivery. A missing entry is represented by a no-permissions ACL:

• normal replay proceeds; with empty watch tables, the value is unused;
• if this state is encountered while watches are present, ordinary client delivery fails closed (super sessions retain their normal bypass);
• null or an empty ACL is deliberately not used, because checkACL treats both as unrestricted and that would reintroduce CVE-2024-23944 / ZOOKEEPER-4799 for these nodes;
• client operations such as getData and getACL retain their strict lookup behavior.

On the valid recovery path this does not suppress any client notification: local restore and learner synchronization replay transactions before the server starts serving clients, so the no-permissions ACL is passed to an empty watch manager. The fallback is observable only if a dangling reference has already survived into live state—for example, in a snapshot written by an older version after the repairing transaction fell outside the replay range. That member no longer has enough information to determine who can read the node, so failing closed is the only safe delivery policy.

Tests

Failing on current master, fixed by this patch:

• FileTxnSnapLogTest.testAclIdsReferencedBySnapshotAreNotReissuedDuringReplay — fix 1; fails with Failed to fetch acls for ... when resolving a node whose own txns replayed normally, i.e. the id-reuse corruption.
• DataTreeTest.testCreateTxnReplayOnExistingNodeKeepsAclRefCountExact — fix 2; fails with a leaked cache entry after the only node using the ACL is deleted.
• FileTxnSnapLogTest.testDeleteTxnReplayOnDanglingAclDoesNotCrash, testSetDataTxnReplayOnDanglingAclDoesNotCrash, testCreateTxnReplayUnderDanglingAclParentDoesNotCrash — fix 3; fail with Failed to fetch acls for ... from deleteNode / setData / createNode (the last is the same line as the stack trace on the ZOOKEEPER-4846 JIRA).

Security behavior of fix 3:

• FileTxnSnapLogTest.testWatchEventsForDanglingAclFailClosed — current master aborts at the strict ACL lookup. With the non-throwing lookup in place, the test additionally asserts that the ACL passed to watch delivery grants no READ; it fails against a variant which passes null, since checkACL treats null as unrestricted.

Guards against the known-bad #1997 approach (pass on current master and with this patch):

• FileTxnSnapLogTest.testAclRefCountDuringFuzzySnapshotSync — catches under-counting when multiple snapshot nodes share the same missing ACL.
• FileTxnSnapLogTest.testAclValuesMatchAfterFuzzySnapshotSyncReplay — catches repair keyed by the stale snapshot id instead of the ACL value carried by the transaction.

The fuzzy-snapshot tests share small helpers (FuzzySnapshot, ReplayTxn), and the pre-existing testACLCreatedDuringFuzzySnapshotSync is rewritten on top of them with its assertions preserved, except one correction: it used to resolve a DataNode taken from the leader tree against the restored tree's ACL cache. ACL ids are server-local, so it now resolves the restored tree's own node.

Testing performed:

mvn -pl zookeeper-server test \
  -Dtest=FileTxnSnapLogTest,DataTreeTest,ReferenceCountedACLCacheTest,\
ACLCountTest,PersistentWatcherACLTest,SnapshotDigestTest,FuzzySnapshotRelatedTest
mvn -pl zookeeper-server checkstyle:check

We have carried the aclIndex reservation part of this fix in Jane Street's internal ZooKeeper 3.5-based fork since 2023.

[adamyi]

@anmolnar @ztzg @kezhuw, would any of you have time to review this? This supersedes #1997 and builds on the value-based repair from #2222. I’ve also addressed the refcount/id-reuse problems discussed on the old PR. Thanks! :)

[adamyi]

(From https://ci-hadoop.apache.org/job/zookeeper-precommit-github-pr/job/PR-2425/1/consoleFull , looks like the build actually succeeded but there's some problem with the hadoop16 node that marked it failing?)