You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by brettKK <gi...@git.apache.org> on 2018/03/28 09:07:27 UTC
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
GitHub user brettKK opened a pull request:
https://github.com/apache/zookeeper/pull/496
ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate
@LJ1043041006 found a potential NPE in ZK
----
callee :SecurityUtils#createSaslClient will return null while encounter exception
```
// code placeholder
catch (Exception e) {
LOG.error("Exception while trying to create SASL client", e);
return null;
}
```
but its caller has no null check just like:
-----
and caller ReferenceCountedACLCache#deserialize call it without null check
```
// code placeholder
sc = SecurityUtils.createSaslClient();
if (sc.hasInitialResponse()) {
responseToken = createSaslToken(new byte[0], sc, learnerLogin);
}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brettKK/zookeeper ZOOKEEPER-3008
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/496.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 #496
----
commit 7d8d5230c5a87faef94d038a258b159a322f3f5e
Author: gongleigl.gong <go...@...>
Date: 2018-03-26T13:16:06Z
d
commit 700dfb7f48f774dd215e5bf19340a4b61eda3397
Author: gongleigl.gong <go...@...>
Date: 2018-03-27T16:38:28Z
fix NPE bug
commit 1ad4da8fc0269378fb2f43975954b5553b0c00e5
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T08:58:24Z
NPE inZOOKEEPER-3008
commit 4458bb32d5813272e0bf0d34364b082e51cad3ed
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:01:10Z
del unuse
commit 7fad1997be2a0401582ab315d60943475ebe1ef1
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:02:32Z
keep up with master
commit 765180fd82a554a2da1c7324843bfe99b8d0a4ed
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:04:50Z
add NPE place
----
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r184218439
--- 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 --
Same feedback as #495
1) check the callers and see if it's handled properly. Likely it will be logged there as well. Verify/report.
2) No need to say exception in an exception. The text of LOG.error line seems like it would have been a good error string for the exception itself.
3) as previously noted, add a test.
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@brettKK do you still work on this patch?
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@lujiefsi Have you run the tests on your machine and was all green?
Maybe it's some flaky tests and you just need to trigger another build.
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by brettKK <gi...@git.apache.org>.
Github user brettKK closed the pull request at:
https://github.com/apache/zookeeper/pull/496
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by lujiefsi <gi...@git.apache.org>.
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...
Posted by LJ1043041006 <gi...@git.apache.org>.
Github user LJ1043041006 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r177685103
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java ---
@@ -94,7 +94,7 @@ public void authenticate(Socket sock, String hostName) throws IOException {
principalConfig,
QuorumAuth.QUORUM_SERVER_PROTOCOL_NAME,
QuorumAuth.QUORUM_SERVER_SASL_DIGEST, LOG, "QuorumLearner");
-
+ // may happen NPE at here
if (sc.hasInitialResponse()) {
responseToken = createSaslToken(new byte[0], sc, learnerLogin);
--- End diff --
just add sc!=null is ok?
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@brettKK You merged the master branch. Next time try rebase instead and you won't end up with so many commits. Now, you can try to squash them for a neat commit history.
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by lujiefsi <gi...@git.apache.org>.
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 pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by lujiefsi <gi...@git.apache.org>.
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 issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by brettKK <gi...@git.apache.org>.
Github user brettKK commented on the issue:
https://github.com/apache/zookeeper/pull/496
@anmolnar , thanks for your attention, I will work on it on this weekend
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r177697912
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java ---
@@ -134,6 +138,8 @@ public void authenticate(Socket sock, String hostName) throws IOException {
// Validate status code at the end of authentication exchange.
checkAuthStatus(sock, qpStatus);
+ } catch (RuntimeException e) {
--- End diff --
@brettKK What's the point of swallowing it here?
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@brettKK you don't need to close/reopen the pull request to trigger a build. Just amend your latest commit and do a force push.
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by lujiefsi <gi...@git.apache.org>.
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 pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by brettKK <gi...@git.apache.org>.
GitHub user brettKK reopened a pull request:
https://github.com/apache/zookeeper/pull/496
ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate
@LJ1043041006 found a potential NPE in ZK
----
callee :SecurityUtils#createSaslClient will return null while encounter exception
```
// code placeholder
catch (Exception e) {
LOG.error("Exception while trying to create SASL client", e);
return null;
}
```
but its caller has no null check just like:
-----
and caller SaslQuorumAuthLearner#authenticate call it without null check
```
// code placeholder
sc = SecurityUtils.createSaslClient();
if (sc.hasInitialResponse()) {
responseToken = createSaslToken(new byte[0], sc, learnerLogin);
}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brettKK/zookeeper ZOOKEEPER-3008
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/496.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 #496
----
commit 7d8d5230c5a87faef94d038a258b159a322f3f5e
Author: gongleigl.gong <go...@...>
Date: 2018-03-26T13:16:06Z
d
commit 700dfb7f48f774dd215e5bf19340a4b61eda3397
Author: gongleigl.gong <go...@...>
Date: 2018-03-27T16:38:28Z
fix NPE bug
commit 1ad4da8fc0269378fb2f43975954b5553b0c00e5
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T08:58:24Z
NPE inZOOKEEPER-3008
commit 4458bb32d5813272e0bf0d34364b082e51cad3ed
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:01:10Z
del unuse
commit 7fad1997be2a0401582ab315d60943475ebe1ef1
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:02:32Z
keep up with master
commit 765180fd82a554a2da1c7324843bfe99b8d0a4ed
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:04:50Z
add NPE place
commit cf611d1783525df308930bb4e3cb2a1cc397ca55
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:12:31Z
fix code
commit 5eec8762b985f31eeb5e607dfd078d197b5a9980
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:46:56Z
fix jenkins error
commit 925bfd2f279852c1898d0f493ccce4ea669d8f9c
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T11:25:19Z
del catch
commit c7879123134b7145ab102a862c11891cacca8298
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T14:11:28Z
fix code
----
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by pravsingh <gi...@git.apache.org>.
Github user pravsingh commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/496#discussion_r205904126
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java ---
@@ -66,8 +67,8 @@ public SaslQuorumAuthLearner(boolean quorumRequireSasl,
+ "section '" + loginContext
+ "' could not be found.");
}
- this.learnerLogin = new Login(loginContext,
- new SaslClientCallbackHandler(null, "QuorumLearner"), new ZKConfig());
+ this.learnerLogin = loginFactory.createLogin(loginContext,
+ new SaslClientCallbackHandler(null, "QuorumLearner"), new ZKConfig());
--- End diff --
this can be put on above line. makes it more readable.
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@brettKK Can we move on?
@phunt Do you approve this change?
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by lujiefsi <gi...@git.apache.org>.
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 #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by nkalmar <gi...@git.apache.org>.
Github user nkalmar commented on the issue:
https://github.com/apache/zookeeper/pull/496
+1
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by lujiefsi <gi...@git.apache.org>.
Github user lujiefsi commented on the issue:
https://github.com/apache/zookeeper/pull/496
Also ping @brettKK
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@brettKK sorry, I was on holiday in the last couple of days.
I'll take a look at it tomorrow.
---
[GitHub] zookeeper pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by brettKK <gi...@git.apache.org>.
GitHub user brettKK reopened a pull request:
https://github.com/apache/zookeeper/pull/496
ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate
@LJ1043041006 found a potential NPE in ZK
----
callee :SecurityUtils#createSaslClient will return null while encounter exception
```
// code placeholder
catch (Exception e) {
LOG.error("Exception while trying to create SASL client", e);
return null;
}
```
but its caller has no null check just like:
-----
and caller SaslQuorumAuthLearner#authenticate call it without null check
```
// code placeholder
sc = SecurityUtils.createSaslClient();
if (sc.hasInitialResponse()) {
responseToken = createSaslToken(new byte[0], sc, learnerLogin);
}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brettKK/zookeeper ZOOKEEPER-3008
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/496.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 #496
----
commit 7d8d5230c5a87faef94d038a258b159a322f3f5e
Author: gongleigl.gong <go...@...>
Date: 2018-03-26T13:16:06Z
d
commit 700dfb7f48f774dd215e5bf19340a4b61eda3397
Author: gongleigl.gong <go...@...>
Date: 2018-03-27T16:38:28Z
fix NPE bug
commit 1ad4da8fc0269378fb2f43975954b5553b0c00e5
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T08:58:24Z
NPE inZOOKEEPER-3008
commit 4458bb32d5813272e0bf0d34364b082e51cad3ed
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:01:10Z
del unuse
commit 7fad1997be2a0401582ab315d60943475ebe1ef1
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:02:32Z
keep up with master
commit 765180fd82a554a2da1c7324843bfe99b8d0a4ed
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:04:50Z
add NPE place
commit cf611d1783525df308930bb4e3cb2a1cc397ca55
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:12:31Z
fix code
commit 5eec8762b985f31eeb5e607dfd078d197b5a9980
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T09:46:56Z
fix jenkins error
commit 925bfd2f279852c1898d0f493ccce4ea669d8f9c
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T11:25:19Z
del catch
commit c7879123134b7145ab102a862c11891cacca8298
Author: gongleigl.gong <go...@...>
Date: 2018-03-28T14:11:28Z
fix code
----
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/496
@brettKK I had some trouble with sorting out everything, but managed to write a unit test.
Not perfect, but could worth to consider adding it.
[0001-ZOOKEEPER-3008.-Added-unit-test.txt](https://github.com/apache/zookeeper/files/1971970/0001-ZOOKEEPER-3008.-Added-unit-test.txt)
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by lujiefsi <gi...@git.apache.org>.
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 pull request #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuth...
Posted by brettKK <gi...@git.apache.org>.
Github user brettKK closed the pull request at:
https://github.com/apache/zookeeper/pull/496
---
[GitHub] zookeeper issue #496: ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner...
Posted by brettKK <gi...@git.apache.org>.
Github user brettKK commented on the issue:
https://github.com/apache/zookeeper/pull/496
I have some trouble to write the test.
Anyone else have any suggestions?
If not, I consider to close this PR.
---