You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by lujiefsi <gi...@git.apache.org> on 2018/05/21 01:52:04 UTC
[GitHub] zookeeper pull request #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxn...
GitHub user lujiefsi opened a pull request:
https://github.com/apache/zookeeper/pull/525
[ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
We have developed a static analysis tool 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>
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 them 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>
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lujiefsi/zookeeper ZOOKEEPER-3009
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/525.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #525
----
commit 8bfad14efaa885818dbf45a33c9ad8b55d3901e4
Author: LJ1043041006 <12...@...>
Date: 2018-05-21T01:41:13Z
[ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
----
---
[GitHub] zookeeper issue #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
Posted by lvfangmin <gi...@git.apache.org>.
Github user lvfangmin commented on the issue:
https://github.com/apache/zookeeper/pull/525
LGTM +1, the caller has handled the IOException properly.
---
[GitHub] zookeeper pull request #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxn...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/525
---
[GitHub] zookeeper issue #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
Posted by lujiefsi <gi...@git.apache.org>.
Github user lujiefsi commented on the issue:
https://github.com/apache/zookeeper/pull/525
Hi: @phunt
Great suggestions for me!!!! I have update the bug description:
(1) getSocketAddress only has two callers, just as shown in description.
(2)addCnxn only has one caller, and has the exception handler.
---
[GitHub] zookeeper issue #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/525
These are useful fixes. However one of the burdens of recommending such a fix is to also prove that the fix really addresses the issue, rather than just moving the problem somewhere else. In particular it would be good if the description of this fix (and any such fix) included details on whether all of the callers to addCnxn handle the new error handling code properly. In this case throwing the IOException in addCnxn. "some of their callers have !=null check but some do not have" One of the things any committer will need to do is also verify, and having this thought out ahead of time will help.
Another thing - are the null checks in the caller code still necessary? Should they be removed? If yes any such patch should include changes there as well.
---
[GitHub] zookeeper pull request #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxn...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/525#discussion_r189666458
--- Diff: src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -815,8 +815,11 @@ public void touchCnxn(NIOServerCnxn cnxn) {
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("Scoket of " + cnxn + " has been closed");
--- End diff --
fix the spelling.
---