-
Notifications
You must be signed in to change notification settings - Fork 7.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZOOKEEPER-3008: Potential NPE in SaslQuorumAuthLearner#authenticate and SaslQuorumAuthServer#authenticate #496
Open
brettKK
wants to merge
70
commits into
apache:master
Choose a base branch
from
brettKK:ZOOKEEPER-3008
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+149
−5
Open
Changes from 8 commits
Commits
Show all changes
70 commits
Select commit
Hold shift + click to select a range
7d8d523
d
700dfb7
fix NPE bug
1ad4da8
NPE inZOOKEEPER-3008
4458bb3
del unuse
7fad199
keep up with master
765180f
add NPE place
cf611d1
fix code
5eec876
fix jenkins error
925bfd2
del catch
c787912
fix code
5c9b577
delete annotation in code
e967e0f
del test file
c4db5e2
recover zookeeper master same with apache:master
brettKK cf9fb5f
recover code
brettKK f7da9b9
change ZOOK3007 to compare with apache master
brettKK a12b13f
fix format
brettKK 0b85882
del logger error and fix error message
7eb9e1c
fix RTE message in ReferenceCountedACLCache class
fb36cf8
Merge remote-tracking branch 'upstream/master'
brettKK 4df1044
merge with new apache master
brettKK 841cc4f
fix test
brettKK b1c4856
add a test , and need to do
ddf1c6c
del test file
brettKK 86910c6
apply anmolnar's patch
brettKK 843e3db
Merge remote-tracking branch 'upstream/master' into ZOOKEEPER-3008
brettKK 974d8b5
ZOOKEEPER-3027: Accidently removed public API of FileTxnLog.setPreall…
anmolnar 4d07262
ZOOKEEPER-2988: NPE triggered if server receives a vote for a server …
aa6d016
ZOOKEEPER-2982: Re-try DNS hostname -> IP resolution if node connecti…
d0536b3
ZOOKEEPER-2901: TTL Nodes don't work with Server IDs > 127
Randgalt 989a35a
ZOOKEEPER-3012: Fix unit test: testDataDirAndDataLogDir should not us…
anmolnar 60e592f
ZOOKEEPER-2959: ignore accepted epoch and LEADERINFO ack from observers
fe31819
ZOOKEEPER-3038: Cleanup some nitpicks in TTL implementation
anmolnar 8bfbd4a
ZOOKEEPER-3039: TxnLogToolkit uses Scanner badly
anmolnar 2234630
ZOOKEEPER-3041: Typo in error message, affects log analysis; charater…
hughobrienjet 4f9da0d
ZOOKEEPER-3046: added junit timeout to precede 'ant' timeout
lavacat 97ee54c
ZOOKEEPER-3050: update to the newest version of Jetty
phunt a56d309
ZOOKEEPER-3051: Updated jackson to the latest version
phunt 13f0efa
ZOOKEEPER-2993: Removed 'generated' line from .gitignore
dc8e7dd
ZOOKEEPER-1919 Update the C implementation of removeWatches to have i…
anmolnar 3486086
ZOOKEEPER-3043: QuorumKerberosHostBasedAuthTest fails on Linux box: U…
eolivelli d010be3
ZOOKEEPER-2317: Ensure that OSGi versions are valid
timothyjward 78ff7f0
ZOOKEEPER-2920: Upgrade OWASP Dependency Check to 3.2.1
phunt e08f199
ZOOKEEPER-2968: Add C client code coverage tests
841e582
Merge branch 'ZOOKEEPER-3008' of https://github.com/brettKK/zookeeper…
brettKK 6f1a618
Merge remote-tracking branch 'upstream/master'
brettKK 32baa8b
Merge branch 'master' into ZOOKEEPER-3008
brettKK ad6245d
NPE inZOOKEEPER-3008
5f96907
Merge branch 'ZOOKEEPER-3008' of https://github.com/brettKK/zookeeper…
brettKK acf8d22
del unuse
490efd7
keep up with master
76b56e7
fix jenkins error
7517635
del catch
521b441
fix code
1723a27
recover zookeeper master same with apache:master
brettKK ca7ba07
fix conficit
brettKK 7a6894a
ZOOKEEPER-3027: Accidently removed public API of FileTxnLog.setPreall…
anmolnar 712151d
conflict
brettKK 890addd
ZOOKEEPER-3027: Accidently removed public API of FileTxnLog.setPreall…
anmolnar 3dfa3f6
NPE inZOOKEEPER-3008
1193ecc
conflict
brettKK c451947
fix conflict
brettKK 1cfbe2b
# This is a combination of 11 commits.
0fd753a
merge info
brettKK b83abd7
merge
brettKK eae2702
fix NPE bug
72d3ded
merge info
brettKK 3fc2f21
fix conflict
brettKK 464f002
recover build.xml
brettKK ac95ac3
mock
brettKK 47df0e0
mock object
brettKK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,7 +94,11 @@ 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 == null) { | ||
LOG.error("SaslClient object is null while trying to create SASL client"); | ||
throw new RuntimeException("Exception while trying to create SASL client"); | ||
} | ||
if (sc.hasInitialResponse()) { | ||
responseToken = createSaslToken(new byte[0], sc, learnerLogin); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just add sc!=null is ok? |
||
} | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brettKK What's the point of swallowing it here? |
||
// do nothing | ||
} finally { | ||
if (sc != null) { | ||
try { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as #495
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For #1:
Follower#77,Observer#69,QuorumCnxManager#333 all have same patern:
try { //root caller } catch (IOException e) {//handler code}
#2 and #3, @brettKK .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, it is hard for me to write a unit test for this bug, any suggestion? @brettKK @anmolnar @phunt @nkalmar @Others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try unit test written by @brettKK ~~