Return-Path: X-Original-To: archive-asf-public-internal@cust-asf2.ponee.io Delivered-To: archive-asf-public-internal@cust-asf2.ponee.io Received: from cust-asf.ponee.io (cust-asf.ponee.io [163.172.22.183]) by cust-asf2.ponee.io (Postfix) with ESMTP id 9FFB9200CED for ; Fri, 18 Aug 2017 23:35:31 +0200 (CEST) Received: by cust-asf.ponee.io (Postfix) id 9E51C16D78F; Fri, 18 Aug 2017 21:35:31 +0000 (UTC) Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by cust-asf.ponee.io (Postfix) with SMTP id BD95916D786 for ; Fri, 18 Aug 2017 23:35:30 +0200 (CEST) Received: (qmail 59841 invoked by uid 500); 18 Aug 2017 21:35:29 -0000 Mailing-List: contact commits-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list commits@zookeeper.apache.org Received: (qmail 59830 invoked by uid 99); 18 Aug 2017 21:35:29 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 18 Aug 2017 21:35:29 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 3D874E02FD; Fri, 18 Aug 2017 21:35:27 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: hanm@apache.org To: commits@zookeeper.apache.org Message-Id: <02598ce6db33459889d80a646f2b7cb8@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: zookeeper git commit: ZOOKEEPER-2804: Node creation fails with NPE if ACLs are null Date: Fri, 18 Aug 2017 21:35:27 +0000 (UTC) archived-at: Fri, 18 Aug 2017 21:35:31 -0000 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 Reviewers: Mohammad Arshad , Michael Han 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 Authored: Fri Aug 18 14:35:24 2017 -0700 Committer: Michael Han 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) 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 removeDuplicates(List acl) { LinkedList retval = new LinkedList(); - 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 acls = new ArrayList(); + 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)); + } + } }