[
https://issues.apache.org/jira/browse/ZOOKEEPER-4689?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18096562#comment-18096562
]
Adam Yi commented on ZOOKEEPER-4689:
------------------------------------
I revisited this against current master and prepared a replacement for the old
[PR #1997|https://github.com/apache/zookeeper/pull/1997]:
[#2425|https://github.com/apache/zookeeper/pull/2425]
ZOOKEEPER-4846 has since fixed the basic repair path: when create replay finds
a node already present in a fuzzy snapshot, it moves that node to the ACL id
obtained from {{{}convertAcls(acl){}}}. This value-based repair is the right
approach; numeric ACL ids are local cache details and need not remain equal
across servers or restarts.
Three issues remain:
# *An id referenced only by snapshot nodes can be reissued to another ACL.* On
a fresh restore, {{aclIndex}} advances only from entries in the serialized ACL
cache, which is written before the nodes. Replay can therefore assign a stale
node id to a different ACL. A delete transaction for a node still carrying that
id can then decrement and garbage-collect an unrelated live ACL entry, leaving
a live node with a dangling ACL after its repairing transaction has already
replayed. The patch advances {{aclIndex}} past every id referenced by the
loaded tree, including ids absent from the serialized cache.
# *Create replay inflates reference counts.* {{convertAcls}} adds a reference
before the existing node is updated, but the node's previous reference is never
released. The patch makes this an exact reference transfer under the node lock.
# *ACL lookup for watch filtering can abort replay.* ZOOKEEPER-4799 made
{{createNode}}, {{deleteNode}}, and {{setData}} resolve ACLs before triggering
watches. Restore and learner synchronization replay transactions before the
server starts serving clients, so the watch tables are normally empty;
nevertheless, a temporarily missing ACL id throws before the watch manager is
reached and aborts recovery. The patch uses a non-throwing lookup only for
watch-event filtering. Missing ACLs are represented by a no-permissions ACL, so
normal replay proceeds and any unexpected ordinary-client delivery fails closed
(super sessions retain their normal bypass). This 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 its repairing transaction fell
outside the replay range. That member no longer has enough information to
determine who may read the node. Null or an empty ACL would pass {{checkACL}}
and would reintroduce CVE-2024-23944 for these nodes, so neither is used.
Client ACL lookups remain strict.
The PR includes tests which fail on current master for ACL-id reuse,
reference-count leakage, and the delete/setData/child-create replay crashes,
plus regressions for both bugs found in the original #1997 approach.
This deliberately does not require cross-server ACL-id consistency or a
snapshot-format change. It enforces the local invariant that an id mentioned by
the loaded tree cannot be assigned a different ACL before replay repairs the
node. We have carried that id-reservation change in Jane Street's internal
ZooKeeper 3.5-based fork since 2023.
[~kezhuw] suggested on PR #1997 to never purge the entry {{aclIndex}} points
to, preserving the allocator's high-water mark so replay can reproduce the ids
recorded in the snapshot. This patch takes a different route: ids need not
match across restarts, but every id referenced by the loaded tree is reserved
so it cannot be reassigned to another ACL, and replay repairs each node from
the ACL value in its transaction. The reference-count drift and replay crash
need fixing either way.
[~ztzg]'s earlier idea of an optional post-replay "fsck" pass would complement
this nicely as a follow-up: it could detect references left dangling by older
versions or by missing transaction logs, which no replay-based repair can
reconstruct. I kept it out of this patch's scope.
> Node may not accessible due the the inconsistent ACL reference map after SNAP
> sync (again)
> ------------------------------------------------------------------------------------------
>
> Key: ZOOKEEPER-4689
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-4689
> Project: ZooKeeper
> Issue Type: Bug
> Components: server
> Affects Versions: 3.6.0, 3.7.0, 3.8.0
> Reporter: Adam Yi
> Priority: Critical
> Labels: pull-request-available
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> In Zookeeper, we do a "fuzzy snapshot". It means that we don't make a copy of
> the DataTree or grab a lock or anything when serializing a DataTree. Instead,
> we note down the zxid when we start serializing DataTree. We serialize the
> DataTree while it's getting mutated and replay the \{transactions after
> starting to take snapshot} after deserializing the DataTree. The idea is that
> those transactions should be idempotent.
> Zookeeper also implements its own interned ACL. It keeps a [long -> ACL] map
> and store the `long` in each node as nodes tend to share the same ACL.
> When serializing DataTree, we first serialize the ACL cache and then
> serialize the nodes. It's possible that with the following sequence, a node
> points to an invalid ACL entry:
> 1. Serialize ACL
> 2. Create node with new ACL
> 3. Serialize node
> ZOOKEEPER-3306 fixes this by making sure to insert the ACL to cache upon
> calling `DataTree.createNode` when replaying transactions and when the node
> already exists. However, we only insert it to the cache, but do not set the
> interned ACL in the node to point to the new entry.
> It's possible that the longval we get for the ACL is inconsistent, even
> though we follow the same zxid ordering of events. Specifically, we keep a
> [aclIndex] pointing to the max entry that currently exists and increment that
> whenever we need to intern a new ACL we've never seen before.
> With ZOOKEEPER-2214, we started to do reference counting in ACL cache and
> remove no-longer used entries from the cache.
> Say the current aclIndex is 10. If we create a node with ACL unseen before
> and delete that node, aclIndex will increment to 11. However, when we
> deserialize the tree, we'll set aclIndex to the max existent ACL cache entry,
> so it's reverted back to 10. aclIndex inconsistency on its own is fine but it
> causes problem to the ZOOKEEPER-3306 patch.
> Now if we follow the same scenario mentioned in ZOOKEEPER-3306:
> # Leader creates ACL entry 11 and delete it due to node deletion
> # Server A starts to have snap sync with leader
> # After serializing the ACL map to Server A, there is a txn T1 to create a
> node N1 with new ACL_1 which was not exist in ACL map
> # On leader, after this txn, the ACL map will be 12 -> (ACL_1, COUNT: 1),
> and data tree N1 -> 12
> # On server A, it will be ACL map with max ID 10, and N1 -> 12 in fuzzy
> snapshot
> # When replaying the txn T1, it will add 11 -> (ACL_1, COUNT: 1) to the ACL
> cache but the node N1 still points to 12.
> N1 still points to invalid ACL entry.
> There are two ways to fix this:
> # Make aclIndex consistent upon re-deserialization (by either serializing it
> in snapshot or paying special attention to decrement it when removing cache)
> # Fix ZOOKEEPER-3306 patch so that we also override the ACL of node to new
> key if previous entry does not exist in the ACL table.
>
> I think solution 2 is nicer as aclIndex inconsistency itself is not a
> problem. With solution 1, we're still implicitly depending on aclIndex
> consistency and ordering of events. It's harder to reason about and it seems
> more fragile than solution 1.
> I'm going to send a patch for solution 2 but please let me know if you
> disagree and I'm happy to go with solution 1 instead.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
