[GitHub] zookeeper pull request #544: ZOOKEEPER-3009 : fix the related bugs in branch...
Github user lujiefsi closed the pull request at: https://github.com/apache/zookeeper/pull/544 ---
[GitHub] zookeeper pull request #544: ZOOKEEPER-3009 : fix the related bugs in branch...
Github user lujiefsi commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/544#discussion_r195967959
--- Diff:
src/java/main/org/apache/zookeeper/server/NIOServerCnxnFactory.java ---
@@ -165,9 +168,13 @@ public void removeCnxn(NIOServerCnxn cnxn) {
}
synchronized (ipMap) {
-Set s =
-ipMap.get(cnxn.getSocketAddress());
-s.remove(cnxn);
+InetAddress addr = cnxn.getSocketAddress();
+ if (addr != null) {
--- End diff --
tabs led the bad coding style, I have removed it.
---
[GitHub] zookeeper pull request #544: ZOOKEEPER-3009 : fix the related bugs in branch...
GitHub user lujiefsi opened a pull request: https://github.com/apache/zookeeper/pull/544 ZOOKEEPER-3009 : fix the related bugs in branch-3.4 The same problem that described in ZOOKEEPER-3009 also exists in branch-3.4, just as @hanm said. I pull a new request to fix it. The patch just make the related code of 3.4 be same as 3.5. please check!!! You can merge this pull request into a Git repository by running: $ git pull https://github.com/lujiefsi/zookeeper ZOOKEEPER-3009-3.4 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/zookeeper/pull/544.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 #544 commit 82e310428268eedec58b0a88d1af9b9a265c999a Author: lujie Date: 2018-06-15T07:42:07Z fix the ZOOKEEPER-3009 in branch-3.4 ---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/496 hum, too many commits, can we merge them as one commit, @brettKK ---
[GitHub] zookeeper issue #525: [ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
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 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
public InetAddress getSocketAddress() {
if (sock.isOpen() == false) {
return null;
}
return sock.socket().getInetAddress();
}
caller removeCnxn has null check
public boolean removeCnxn(NIOServerCnxn cnxn) {
//other code
InetAddress addr = cnxn.getSocketAddress();
if (addr != null) {
Set set = ipMap.get(addr);
//other code
}
//other code
}
but caller addCnxn has them similar code, but don't have the null check:
private void addCnxn(NIOServerCnxn cnxn) {
InetAddress addr = cnxn.getSocketAddress();
Set set = ipMap.get(addr);
//other code
}
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 <1239497420@...>
Date: 2018-05-21T01:41:13Z
[ZOOKEEPER-3009] Potential NPE in NIOServerCnxnFactory
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/496 @anmolnar Local run is ok, @brettKK could you trigger another build ---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/496 are the failure related to this patch??? ---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Github user lujiefsi commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r184676603
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java
---
@@ -94,7 +94,10 @@ public void authenticate(Socket sock, String hostName)
throws IOException {
principalConfig,
QuorumAuth.QUORUM_SERVER_PROTOCOL_NAME,
QuorumAuth.QUORUM_SERVER_SASL_DIGEST, LOG,
"QuorumLearner");
-
+if (sc == null) {
--- End diff --
I will try unit test written by @brettKK ~~
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Github user lujiefsi commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r184675859
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java
---
@@ -94,7 +94,10 @@ public void authenticate(Socket sock, String hostName)
throws IOException {
principalConfig,
QuorumAuth.QUORUM_SERVER_PROTOCOL_NAME,
QuorumAuth.QUORUM_SERVER_SASL_DIGEST, LOG,
"QuorumLearner");
-
+if (sc == null) {
--- End diff --
Hum, it is hard for me to write a unit test for this bug, any suggestion?
@brettKK @anmolnar @phunt @nkalmar @others
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/495 Seems that unit test error is not caused by this patch? ---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Github user lujiefsi commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r184251756
--- Diff:
src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java
---
@@ -94,7 +94,10 @@ public void authenticate(Socket sock, String hostName)
throws IOException {
principalConfig,
QuorumAuth.QUORUM_SERVER_PROTOCOL_NAME,
QuorumAuth.QUORUM_SERVER_SASL_DIGEST, LOG,
"QuorumLearner");
-
+if (sc == null) {
--- End diff --
For #1:
Follower#77,Observer#69,QuorumCnxManager#333 all have same patern:
`try { //root caller } catch (IOException e) {//handler code}`
#2 and #3, @brettKK .
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Github user lujiefsi commented on the issue:
https://github.com/apache/zookeeper/pull/495
@phunt
I got it. I have found all "deserialize" root caller and callsite postion:
(1)QuorumPeer#1208,#1154,#1152,#1182,#1154,#1194,#1195: their code have
same format:
` try {
//root caller
} catch (Exception e) {
LOG.warn("Unexpected exception",e);
}
} `
So i think the RuntimeException in the patch is ok in here
(2)QuorumPeer#860,852: there is another "throw new RuntimeException" at
#520. So i think the RuntimeException in the patch is ok in here
(3)ZooKeeperServerMain#64 SnapshotFormatter#53 : these two caller are main
function, when run into RuntimeException , it will exit, I am not sure the
"RuntimeException" in the patch whether is ok in here.
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/495 @phunt : I think the reason that @brettKK use RuntimeException to replace NullPointerException is :(1)NullPointerException is subclass of RuntimeException (2) give the semantic reason instead ugly NPE.(3)@brettkk may dost not think over what happens when the RTE is thrown. He/She may just do it due to (2) . ---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/495 Can we close it??? ---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Github user lujiefsi commented on the issue: https://github.com/apache/zookeeper/pull/496 Also ping @brettKK ---
