ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate #496

brettKK · 2018-03-28T09:07:24Z

@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);
}

lujiefsi · 2018-03-28T09:10:51Z

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);


just add sc!=null is ok?

anmolnar

+1 nice catch, just a nitpick

anmolnar · 2018-03-28T09:58:46Z

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) {


@brettKK What's the point of swallowing it here?

anmolnar · 2018-03-28T15:55:10Z

@brettKK you don't need to close/reopen the pull request to trigger the build. Just amend your latest commit and do a force push.

nkalmar · 2018-04-18T07:57:16Z

+1

anmolnar · 2018-04-19T14:41:33Z

@brettKK do you still work on this patch?

lujiefsi · 2018-04-19T15:12:25Z

Also ping @brettKK

anmolnar

I think only a unit test is needed to get this patch done.

phunt · 2018-04-25T21:58:21Z

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) {


Same feedback as #495

check the callers and see if it's handled properly. Likely it will be logged there as well. Verify/report.

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.

as previously noted, add a test.

For #1:
Follower#77,Observer#69,QuorumCnxManager#333 all have same patern:
try { //root caller } catch (IOException e) {//handler code}
#2 and #3, @brettKK .

Hum, it is hard for me to write a unit test for this bug, any suggestion? @brettKK @anmolnar @phunt @nkalmar @Others

I will try unit test written by @brettKK ~~

…into ZOOKEEPER-3008

del unuse keep up with master add NPE place fix code fix jenkins error del catch fix code delete annotation in code del test file recover code change ZOOK3007 to compare with apache master fix format del logger error and fix error message fix RTE message in ReferenceCountedACLCache class fix test add a test , and need to do del test file apply anmolnar's patch

…into ZOOKEEPER-3008

recover code change ZOOK3007 to compare with apache master

del logger error and fix error message fix test apply anmolnar's patch

…ocSize() In my latest commit regarding TxnLogToolkit there's a refactor to outsource file padding logic from FileTxnLog to a separate class: 126fb0f#diff-89717124564925d61d29dd817bcdd915L147 Unfortunately public static method setPreallocSize(int) has also been moved to the new class, but it's being actively used by hadoop-common project too: https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/ha/ClientBaseWithFixes.java#L384 I'd like to submit a patch to revert the deleted method which is going to call the new one, but will keep backward compatibility with Hadoop. Author: Andor Molnar <[email protected]> Reviewers: [email protected] Closes #509 from anmolnar/ZOOKEEPER-3027 Change-Id: I7333b5e24c2a78d10a5c5a74c181633050bd225d del unuse keep up with master fix jenkins error del catch fix code recover zookeeper master same with apache:master recover code change ZOOK3007 to compare with apache master fix conficit del logger error and fix error message fix test apply anmolnar's patch

del unuse keep up with master add NPE place fix code fix jenkins error del catch fix code delete annotation in code del test file recover code change ZOOK3007 to compare with apache master fix format del logger error and fix error message fix RTE message in ReferenceCountedACLCache class fix test add a test , and need to do del test file apply anmolnar's patch

# This is the 1st commit message: fix NPE bug # This is the commit message #2: NPE inZOOKEEPER-3008 # This is the commit message #3: del unuse # This is the commit message #4: keep up with master # This is the commit message #5: add NPE place # This is the commit message #6: fix code # This is the commit message #7: fix jenkins error # This is the commit message #8: del catch # This is the commit message #9: fix code # This is the commit message #10: delete annotation in code # This is the commit message #11: del test file

recover code change ZOOK3007 to compare with apache master fix format del logger error and fix error message fix RTE message in ReferenceCountedACLCache class

NPE inZOOKEEPER-3008 del unuse keep up with master add NPE place fix code fix jenkins error del catch fix code delete annotation in code del test file merge info recover code change ZOOK3007 to compare with apache master fix format del logger error and fix error message fix RTE message in ReferenceCountedACLCache class NPE inZOOKEEPER-3008 del unuse keep up with master fix jenkins error del catch fix code delete annotation in code del test file

pravsingh

minor comment about styling.

Do we use some sort of Checkstyle for consistent code formatting?

If not, I can create ticket and work on it.

pravsingh · 2018-07-27T21:28:50Z

src/java/main/org/apache/zookeeper/server/quorum/auth/SaslQuorumAuthLearner.java

-            this.learnerLogin = new Login(loginContext,
-                                    new SaslClientCallbackHandler(null, "QuorumLearner"), new ZKConfig());
+            this.learnerLogin = loginFactory.createLogin(loginContext,
+                    new SaslClientCallbackHandler(null, "QuorumLearner"), new ZKConfig());


this can be put on above line. makes it more readable.

anmolnar · 2018-08-08T12:56:58Z

@brettKK Can we move on?
@phunt Do you approve this change?

brettKK · 2018-08-08T14:14:39Z

@anmolnar , thanks for your attention, I will work on it this weekend

gongleigl.gong added 6 commits March 26, 2018 21:16

d

7d8d523

fix NPE bug

700dfb7

NPE inZOOKEEPER-3008

1ad4da8

del unuse

4458bb3

keep up with master

7fad199

add NPE place

765180f

lujiefsi reviewed Mar 28, 2018

View reviewed changes

gongleigl.gong added 2 commits March 28, 2018 17:12

fix code

cf611d1

fix jenkins error

5eec876

anmolnar requested changes Mar 28, 2018

View reviewed changes

gongleigl.gong added 2 commits March 28, 2018 19:25

del catch

925bfd2

fix code

c787912

brettKK closed this Mar 28, 2018

brettKK reopened this Mar 28, 2018

brettKK closed this Mar 28, 2018

brettKK reopened this Mar 28, 2018

gongleigl.gong added 2 commits April 20, 2018 10:05

delete annotation in code

5c9b577

del test file

e967e0f

anmolnar requested changes Apr 20, 2018

View reviewed changes

brettKK and others added 5 commits April 22, 2018 14:50

recover zookeeper master same with apache:master

c4db5e2

recover code

cf9fb5f

change ZOOK3007 to compare with apache master

f7da9b9

fix format

a12b13f

del logger error and fix error message

0b85882

phunt reviewed Apr 25, 2018

View reviewed changes

fix RTE message in ReferenceCountedACLCache class

7eb9e1c

brettKK and others added 25 commits June 2, 2018 07:41

Merge branch 'ZOOKEEPER-3008' of https://github.com/brettKK/zookeeper …

841e582

…into ZOOKEEPER-3008

Merge remote-tracking branch 'upstream/master'

6f1a618

Merge branch 'master' into ZOOKEEPER-3008

32baa8b

Merge branch 'ZOOKEEPER-3008' of https://github.com/brettKK/zookeeper …

5f96907

…into ZOOKEEPER-3008

del unuse

acf8d22

keep up with master

490efd7

fix jenkins error

76b56e7

del catch

7517635

fix code

521b441

recover zookeeper master same with apache:master

1723a27

recover code change ZOOK3007 to compare with apache master

fix conficit

ca7ba07

del logger error and fix error message fix test apply anmolnar's patch

conflict

712151d

conflict

1193ecc

fix conflict

c451947

merge info

0fd753a

recover code change ZOOK3007 to compare with apache master fix format del logger error and fix error message fix RTE message in ReferenceCountedACLCache class

merge

b83abd7

merge info

72d3ded

fix conflict

3fc2f21

recover build.xml

464f002

pravsingh reviewed Jul 27, 2018

View reviewed changes

brettKK added 2 commits August 12, 2018 11:25

mock

ac95ac3

mock object

47df0e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate #496

ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate #496

brettKK commented Mar 28, 2018 •

edited

Loading

lujiefsi Mar 28, 2018

anmolnar left a comment

anmolnar Mar 28, 2018

anmolnar commented Mar 28, 2018 •

edited

Loading

nkalmar commented Apr 18, 2018

anmolnar commented Apr 19, 2018

lujiefsi commented Apr 19, 2018

anmolnar left a comment

phunt Apr 25, 2018

lujiefsi Apr 26, 2018

lujiefsi Apr 27, 2018

lujiefsi Apr 27, 2018

pravsingh left a comment •

edited

Loading

pravsingh Jul 27, 2018

anmolnar commented Aug 8, 2018

brettKK commented Aug 8, 2018 •

edited

Loading

ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate #496

Are you sure you want to change the base?

ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate #496

Conversation

brettKK commented Mar 28, 2018 • edited Loading

@LJ1043041006 found a potential NPE in ZK

but its caller has no null check just like:

lujiefsi Mar 28, 2018

Choose a reason for hiding this comment

anmolnar left a comment

Choose a reason for hiding this comment

anmolnar Mar 28, 2018

Choose a reason for hiding this comment

anmolnar commented Mar 28, 2018 • edited Loading

nkalmar commented Apr 18, 2018

anmolnar commented Apr 19, 2018

lujiefsi commented Apr 19, 2018

anmolnar left a comment

Choose a reason for hiding this comment

phunt Apr 25, 2018

Choose a reason for hiding this comment

lujiefsi Apr 26, 2018

Choose a reason for hiding this comment

lujiefsi Apr 27, 2018

Choose a reason for hiding this comment

lujiefsi Apr 27, 2018

Choose a reason for hiding this comment

pravsingh left a comment • edited Loading

Choose a reason for hiding this comment

pravsingh Jul 27, 2018

Choose a reason for hiding this comment

anmolnar commented Aug 8, 2018

brettKK commented Aug 8, 2018 • edited Loading

brettKK commented Mar 28, 2018 •

edited

Loading

anmolnar commented Mar 28, 2018 •

edited

Loading

pravsingh left a comment •

edited

Loading

brettKK commented Aug 8, 2018 •

edited

Loading