Skip to content

ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index after SNAP sync #1997

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
adamyi wants to merge 2 commits into from
+51 −21
Closed

ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index after SNAP sync #1997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb94cfb
ZOOKEEPER-4689: Add failing test
adamyi Apr 20, 2023
7b12742
ZOOKEEPER-4689: Fix inaccessible node due to inconsistent ACL index a…
adamyi Apr 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 18 additions & 13 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,21 +446,25 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
throw new NoNodeException();
}
synchronized (parent) {
// 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);

Set<String> children = parent.getChildren();
if (children.contains(childName)) {
DataNode child = nodes.get(path);
if (child == null) {
LOG.error("Parent claims child exists but it does not: " + path);
} else {
synchronized (child) {
// Due to fuzzy snapshot, it's possible that a node
// exists but its ACL is missing in the cache. This is
// because they serialized ACL cache before node is
// created but serialized nodes after creation. In this
// case, we'll replay node creation transaction. We
// should add it to cache and reset the ACL if the
// previous longval does not exist.
if (!aclCache.isLongCached(child.acl)) {

@kezhuw kezhuw Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the cause to bug#2 in this patch ?

child.acl = aclCache.convertAcls(acl);
}
}
}
throw new NodeExistsException();
}

Expand All @@ -478,6 +482,7 @@ public void createNode(final String path, byte[] data, List<ACL> acl, long ephem
parent.stat.setCversion(parentCVersion);
parent.stat.setPzxid(zxid);
}
Long acls = aclCache.convertAcls(acl);

@kezhuw kezhuw Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this patch, how bug#1 could happen ?

We would create ACL 6 with a refcount of 1 at step 2 and leave it unchanged at step 3 because ACL 6 already exists. Deleting /a1 at step 4 would decrement the refcount and consequently garbage-collect the ACL entry, leaving /a2 pointing to a non-existent ACL entry.

step#3 creates a new node with existing ACL, so that ACL will get ref count incremented. step#4 will not delete ACL 6.

DataNode child = new DataNode(data, acls, stat);
parent.addChild(childName);
nodes.postChange(parentName, parent);
Expand Down
13 changes: 13 additions & 0 deletions zookeeper-server/src/main/java/org/apache/zookeeper/server/ReferenceCountedACLCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,19 @@ public synchronized List<ACL> convertLong(Long longVal) {
return acls;
}

/**
* checks if longval is cached (convertLong can convert it to ACLs).
*
* @param longVal

@sonatype-lift sonatype-lift Bot Apr 20, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9% of developers fix this issue

EmptyBlockTag: A block tag (@param, @return, @throws, @deprecated) has an empty description. Block tags without descriptions don't add much value for future readers of the code; consider removing the tag entirely or adding a description.

Suggested change
* @param longVal
*
ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

* @return true if it's cached.
*/
public synchronized boolean isLongCached(long longVal) {
if (longVal == OPEN_UNSAFE_ACL_ID) {
return true;
}
return longKeyMap.containsKey(longVal);

@sonatype-lift sonatype-lift Bot Apr 20, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7% of developers fix this issue

THREAD_SAFETY_VIOLATION: Read/Write race. Non-private method ReferenceCountedACLCache.isLongCached(...) reads with synchronization from container this.longKeyMap via call to Map.containsKey(...). Potentially races with unsynchronized write in method ReferenceCountedACLCache.deserialize(...).
Reporting because this access may occur on a background thread.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

}

private long incrementIndex() {
return ++aclIndex;
}
Expand Down
28 changes: 20 additions & 8 deletions ...eper-server/src/test/java/org/apache/zookeeper/server/persistence/FileTxnSnapLogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
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;
Expand Down Expand Up @@ -333,6 +334,14 @@ public void testDirCheckWithLogFilesInSnapDir() throws IOException {
@Test
public void testACLCreatedDuringFuzzySnapshotSync() throws IOException {
DataTree leaderDataTree = new DataTree();
// Populate the initial ACL cache with some entries before snapshotting.
TxnHeader hdr1 = new TxnHeader(1, 2, 2, 2, ZooDefs.OpCode.create);
Record txn1 = new CreateTxn("/a1", "foo".getBytes(), ZooDefs.Ids.OPEN_ACL_UNSAFE, false, -1);
leaderDataTree.processTxn(hdr1, txn1);

TxnHeader hdr2 = new TxnHeader(1, 2, 3, 2, ZooDefs.OpCode.delete);
Record txn2 = new DeleteTxn("/a1");
leaderDataTree.processTxn(hdr2, txn2);

// Start the simulated snap-sync by serializing ACL cache.
File file = File.createTempFile("snapshot", "zk");
Expand All @@ -341,9 +350,9 @@ public void testACLCreatedDuringFuzzySnapshotSync() throws IOException {
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);
TxnHeader hdr3 = new TxnHeader(1, 2, 4, 2, ZooDefs.OpCode.create);
Record txn3 = new CreateTxn("/a1", "foo".getBytes(), ZooDefs.Ids.CREATOR_ALL_ACL, false, -1);
leaderDataTree.processTxn(hdr3, txn3);

// Finish the snapshot.
leaderDataTree.serializeNodes(oa);
Expand All @@ -354,13 +363,16 @@ public void testACLCreatedDuringFuzzySnapshotSync() throws IOException {
InputArchive ia = BinaryInputArchive.getArchive(is);
DataTree followerDataTree = new DataTree();
followerDataTree.deserialize(ia, "tree");
followerDataTree.processTxn(hdr1, txn1);
is.close();
followerDataTree.processTxn(hdr3, txn3);

DataNode a1 = leaderDataTree.getNode("/a1");
assertNotNull(a1);
assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, leaderDataTree.getACL(a1));
DataNode a1FromLeader = leaderDataTree.getNode("/a1");
assertNotNull(a1FromLeader);
assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, leaderDataTree.getACL(a1FromLeader));

assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, followerDataTree.getACL(a1));
DataNode a1FromFollower = followerDataTree.getNode("/a1");
assertNotNull(a1FromFollower);
assertEquals(ZooDefs.Ids.CREATOR_ALL_ACL, followerDataTree.getACL(a1FromFollower));
}

@Test
Expand Down