zookeeper-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From h...@apache.org
Subject zookeeper git commit: ZOOKEEPER-2804: Node creation fails with NPE if ACLs are null
Date Fri, 18 Aug 2017 21:35:27 GMT
Repository: zookeeper
Updated Branches:
  refs/heads/master 0706b40af -> d7c192c18


ZOOKEEPER-2804: Node creation fails with NPE if ACLs are null

1) Handled Null case in server. Client will get InvalidACLException
2) Handled null check in create and setACL APIs in client side as mentioned in their javadoc
throws KeeperException.InvalidACLException if the ACL is invalid, null, or empty
3) Not handling any validation for async API of create and setACL in this JIRA because these
API doesn't throw KeeperException explicitly. So can not throw InvalidACLExceptin from Client.
 If we throw IllegalArgumentException then it will not be consistent with other sync APIs.
 So Let server throw InvalidACLException for async API.

Please review and provide suggestion.

Author: bhupendra jain <bhupendra.jain@huawei.com>

Reviewers: Mohammad Arshad <arshad@apache.org>, Michael Han <hanm@apache.org>

Closes #279 from jainbhupendra24/ZOOKEEPER-2804-new


Project: http://git-wip-us.apache.org/repos/asf/zookeeper/repo
Commit: http://git-wip-us.apache.org/repos/asf/zookeeper/commit/d7c192c1
Tree: http://git-wip-us.apache.org/repos/asf/zookeeper/tree/d7c192c1
Diff: http://git-wip-us.apache.org/repos/asf/zookeeper/diff/d7c192c1

Branch: refs/heads/master
Commit: d7c192c1829cc657d3312f47ecb5b97519d1b30d
Parents: 0706b40
Author: bhupendra jain <bhupendra.jain@huawei.com>
Authored: Fri Aug 18 14:35:24 2017 -0700
Committer: Michael Han <hanm@apache.org>
Committed: Fri Aug 18 14:35:24 2017 -0700

----------------------------------------------------------------------
 .../main/org/apache/zookeeper/ZooKeeper.java    | 26 ++++--
 .../zookeeper/server/PrepRequestProcessor.java  |  8 +-
 .../test/org/apache/zookeeper/test/ACLTest.java | 90 ++++++++++++++++++++
 3 files changed, 112 insertions(+), 12 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/zookeeper/blob/d7c192c1/src/java/main/org/apache/zookeeper/ZooKeeper.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/ZooKeeper.java b/src/java/main/org/apache/zookeeper/ZooKeeper.java
index 3218909..d24404f 100644
--- a/src/java/main/org/apache/zookeeper/ZooKeeper.java
+++ b/src/java/main/org/apache/zookeeper/ZooKeeper.java
@@ -1429,6 +1429,7 @@ public class ZooKeeper implements AutoCloseable {
         final String clientPath = path;
         PathUtils.validatePath(clientPath, createMode.isSequential());
         EphemeralType.validateTTL(createMode, -1);
+        validateACL(acl);
 
         final String serverPath = prependChroot(clientPath);
 
@@ -1439,9 +1440,6 @@ public class ZooKeeper implements AutoCloseable {
         request.setData(data);
         request.setFlags(createMode.toFlag());
         request.setPath(serverPath);
-        if (acl != null && acl.size() == 0) {
-            throw new KeeperException.InvalidACLException();
-        }
         request.setAcl(acl);
         ReplyHeader r = cnxn.submitRequest(h, request, response, null);
         if (r.getErr() != 0) {
@@ -1532,15 +1530,13 @@ public class ZooKeeper implements AutoCloseable {
         final String clientPath = path;
         PathUtils.validatePath(clientPath, createMode.isSequential());
         EphemeralType.validateTTL(createMode, ttl);
+        validateACL(acl);
 
         final String serverPath = prependChroot(clientPath);
 
         RequestHeader h = new RequestHeader();
         setCreateHeader(createMode, h);
         Create2Response response = new Create2Response();
-        if (acl != null && acl.size() == 0) {
-            throw new KeeperException.InvalidACLException();
-        }
         Record record = makeCreateRecord(createMode, serverPath, data, acl, ttl);
         ReplyHeader r = cnxn.submitRequest(h, record, response, null);
         if (r.getErr() != 0) {
@@ -2373,6 +2369,7 @@ public class ZooKeeper implements AutoCloseable {
     {
         final String clientPath = path;
         PathUtils.validatePath(clientPath);
+        validateACL(acl);
 
         final String serverPath = prependChroot(clientPath);
 
@@ -2380,9 +2377,6 @@ public class ZooKeeper implements AutoCloseable {
         h.setType(ZooDefs.OpCode.setACL);
         SetACLRequest request = new SetACLRequest();
         request.setPath(serverPath);
-        if (acl != null && acl.size() == 0) {
-            throw new KeeperException.InvalidACLException(clientPath);
-        }
         request.setAcl(acl);
         request.setVersion(aclVersion);
         SetACLResponse response = new SetACLResponse();
@@ -2945,4 +2939,18 @@ public class ZooKeeper implements AutoCloseable {
             throw ioe;
         }
     }
+
+    /**
+     * Validates the provided ACL list for null, empty or null value in it.
+     * 
+     * @param acl
+     *            ACL list
+     * @throws InvalidACLException
+     *             if ACL list is not valid
+     */
+    private void validateACL(List<ACL> acl) throws KeeperException.InvalidACLException
{
+        if (acl == null || acl.isEmpty() || acl.contains(null)) {
+            throw new KeeperException.InvalidACLException();
+        }
+    }
 }

http://git-wip-us.apache.org/repos/asf/zookeeper/blob/d7c192c1/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
----------------------------------------------------------------------
diff --git a/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java b/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
index 63b35ab..deda6b9 100644
--- a/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
+++ b/src/java/main/org/apache/zookeeper/server/PrepRequestProcessor.java
@@ -912,9 +912,11 @@ public class PrepRequestProcessor extends ZooKeeperCriticalThread implements
     private List<ACL> removeDuplicates(List<ACL> acl) {
 
         LinkedList<ACL> retval = new LinkedList<ACL>();
-        for (ACL a : acl) {
-            if (!retval.contains(a)) {
-                retval.add(a);
+        if (acl != null) {
+            for (ACL a : acl) {
+                if (!retval.contains(a)) {
+                    retval.add(a);
+                }
             }
         }
         return retval;

http://git-wip-us.apache.org/repos/asf/zookeeper/blob/d7c192c1/src/java/test/org/apache/zookeeper/test/ACLTest.java
----------------------------------------------------------------------
diff --git a/src/java/test/org/apache/zookeeper/test/ACLTest.java b/src/java/test/org/apache/zookeeper/test/ACLTest.java
index e88f7f4..b9c2c49 100644
--- a/src/java/test/org/apache/zookeeper/test/ACLTest.java
+++ b/src/java/test/org/apache/zookeeper/test/ACLTest.java
@@ -22,11 +22,13 @@ import static org.apache.zookeeper.test.ClientBase.CONNECTION_TIMEOUT;
 
 import java.io.File;
 import java.util.ArrayList;
+import java.util.List;
 import java.util.concurrent.CountDownLatch;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.apache.zookeeper.CreateMode;
+import org.apache.zookeeper.KeeperException.InvalidACLException;
 import org.apache.zookeeper.PortAssignment;
 import org.apache.zookeeper.WatchedEvent;
 import org.apache.zookeeper.Watcher;
@@ -189,4 +191,92 @@ public class ACLTest extends ZKTestCase implements Watcher {
             }
         }
     }
+    
+    @Test
+    public void testNullACL() throws Exception {
+        File tmpDir = ClientBase.createTmpDir();
+        ClientBase.setupTestEnv();
+        ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+        final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+        ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+        f.startup(zks);
+        ZooKeeper zk = ClientBase.createZKClient(HOSTPORT);
+        try {
+            // case 1 : null ACL with create
+            try {
+                zk.create("/foo", "foo".getBytes(), null, CreateMode.PERSISTENT);
+                Assert.fail("Expected InvalidACLException for null ACL parameter");
+            } catch (InvalidACLException e) {
+                // Expected. Do nothing
+            }
+
+            // case 2 : null ACL with other create API
+            try {
+                zk.create("/foo", "foo".getBytes(), null, CreateMode.PERSISTENT, null);
+                Assert.fail("Expected InvalidACLException for null ACL parameter");
+            } catch (InvalidACLException e) {
+                // Expected. Do nothing
+            }
+            
+            // case 3 : null ACL with setACL
+            try {
+                zk.setACL("/foo", null, 0);
+                Assert.fail("Expected InvalidACLException for null ACL parameter");
+            } catch (InvalidACLException e) {
+                // Expected. Do nothing
+            }
+        } finally {
+            zk.close();
+            f.shutdown();
+            zks.shutdown();
+            Assert.assertTrue("waiting for server down",
+                    ClientBase.waitForServerDown(HOSTPORT, ClientBase.CONNECTION_TIMEOUT));
+        }
+    }
+
+    @Test
+    public void testNullValueACL() throws Exception {
+        File tmpDir = ClientBase.createTmpDir();
+        ClientBase.setupTestEnv();
+        ZooKeeperServer zks = new ZooKeeperServer(tmpDir, tmpDir, 3000);
+        final int PORT = Integer.parseInt(HOSTPORT.split(":")[1]);
+        ServerCnxnFactory f = ServerCnxnFactory.createFactory(PORT, -1);
+        f.startup(zks);
+        ZooKeeper zk = ClientBase.createZKClient(HOSTPORT);
+        try {
+
+            List<ACL> acls = new ArrayList<ACL>();
+            acls.add(null);
+
+            // case 1 : null value in ACL list with create
+            try {
+                zk.create("/foo", "foo".getBytes(), acls, CreateMode.PERSISTENT);
+                Assert.fail("Expected InvalidACLException for null value in ACL List");
+            } catch (InvalidACLException e) {
+                // Expected. Do nothing
+            }
+
+            // case 2 : null value in ACL list with other create API
+            try {
+                zk.create("/foo", "foo".getBytes(), acls, CreateMode.PERSISTENT, null);
+                Assert.fail("Expected InvalidACLException for null value in ACL List");
+            } catch (InvalidACLException e) {
+                // Expected. Do nothing
+            }
+
+            // case 3 : null value in ACL list with setACL
+            try {
+                zk.setACL("/foo", acls, -1);
+                Assert.fail("Expected InvalidACLException for null value in ACL List");
+            } catch (InvalidACLException e) {
+                // Expected. Do nothing
+            }
+        } finally {
+            zk.close();
+            f.shutdown();
+            zks.shutdown();
+            Assert.assertTrue("waiting for server down",
+                    ClientBase.waitForServerDown(HOSTPORT, ClientBase.CONNECTION_TIMEOUT));
+        }
+    }
 }


Mime
View raw message