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-3009: Potential NPE in NIOServerCnxnFactory
Date Fri, 15 Jun 2018 06:02:53 GMT
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 <1239497420@qq.com>

Reviewers: Patrick Hunt <phunt@apache.org>, Allan Lyu <fangmin@apache.org>, Michael
Han <hanm@apache.org>

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 <1239497420@qq.com>
Authored: Thu Jun 14 23:02:41 2018 -0700
Committer: Michael Han <hanm@apache.org>
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


Mime
View raw message