Repository: zookeeper Updated Branches: refs/heads/master d6490d590 -> 13dd5d0db
ZOOKEEPER-3009: Potential NPE in NIOServerCnxnFactory We have developed a static analysis tool [NPEDetector](https://github.com/lujiefsi/NPEDetector) to find some potential NPE. Our analysis shows that NPE reason can be simple:some callees may return null directly in corner case(e.g. node crash , IO exception), some of their callers have !=null check but some do not have. ### Bug: Callee getSocketAddress can return null, may caused by node crash, network exception.... <pre> public InetAddress getSocketAddress() { if (sock.isOpen() == false) { return null; } return sock.socket().getInetAddress(); } </pre> getSocketAddress has two callers: removeCnxn and removeCnxn caller removeCnxn has null check <pre> public boolean removeCnxn(NIOServerCnxn cnxn) { //other code InetAddress addr = cnxn.getSocketAddress(); if (addr != null) { Set<NIOServerCnxn> set = ipMap.get(addr); //other code } //other code } </pre> but caller addCnxn has the similar code, but don't have the null check: <pre> private void addCnxn(NIOServerCnxn cnxn) { InetAddress addr = cnxn.getSocketAddress(); Set<NIOServerCnxn> set = ipMap.get(addr); //other code } </pre> I commit a patch to fix it: adding an null checker in addCnxn, and throws a semantics IOException rather than the ugly NPE. I think the IOException is ok, because the caller of addCnxn has the handler code: <pre> private void processAcceptedConnections() { //other code try { addCnxn(cnxn); } catch (IOException e) { // register, createConnection cleanupSelectionKey(key); fastCloseSock(accepted); } } </pre> Maybe i am wrong, please point out my error. Author: LJ1043041006 <[email protected]> Reviewers: Patrick Hunt <[email protected]>, Allan Lyu <[email protected]>, Michael Han <[email protected]> Closes #525 from lujiefsi/ZOOKEEPER-3009 Project: http://git-wip-us.apache.org/repos/asf/zookeeper/repo Commit: http://git-wip-us.apache.org/repos/asf/zookeeper/commit/13dd5d0d Tree: http://git-wip-us.apache.org/repos/asf/zookeeper/tree/13dd5d0d Diff: http://git-wip-us.apache.org/repos/asf/zookeeper/diff/13dd5d0d Branch: refs/heads/master Commit: 13dd5d0db7a5c4fa926d1e44fc2047c24d5d012c Parents: d6490d5 Author: LJ1043041006 <[email protected]> Authored: Thu Jun 14 23:02:41 2018 -0700 Committer: Michael Han <[email protected]> Committed: Thu Jun 14 23:02:41 2018 -0700 ---------------------------------------------------------------------- .../main/org/apache/zookeeper/server/NIOServerCnxnFactory.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/zookeeper/blob/13dd5d0d/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---------------------------------------------------------------------- diff --git a/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java b/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java index 7a72757..e343bc0 100644 --- a/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java +++ b/src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java @@ -815,8 +815,11 @@ public class NIOServerCnxnFactory extends ServerCnxnFactory { cnxnExpiryQueue.update(cnxn, cnxn.getSessionTimeout()); } - private void addCnxn(NIOServerCnxn cnxn) { + private void addCnxn(NIOServerCnxn cnxn) throws IOException { InetAddress addr = cnxn.getSocketAddress(); + if (addr == null) { + throw new IOException("Socket of " + cnxn + " has been closed"); + } Set<NIOServerCnxn> set = ipMap.get(addr); if (set == null) { // in general we will see 1 connection from each
