You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zookeeper.apache.org by ha...@apache.org on 2018/06/15 06:02:53 UTC
zookeeper git commit: ZOOKEEPER-3009: Potential NPE in
NIOServerCnxnFactory
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 <12...@qq.com>
Reviewers: Patrick Hunt <ph...@apache.org>, Allan Lyu <fa...@apache.org>, Michael Han <ha...@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 <12...@qq.com>
Authored: Thu Jun 14 23:02:41 2018 -0700
Committer: Michael Han <ha...@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