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/26 13:18:22 UTC
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
GitHub user brettKK opened a pull request:
https://github.com/apache/zookeeper/pull/495
ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCache#deserialize
@LJ1043041006 found a potential NPE in ZK
----
callee BinaryInputArchive#startVector will return null:
```
// code placeholder
public Index startVector(String tag) throws IOException {
int len = readInt(tag);
if (len == -1) {
return null;
}
```
-----
and caller ReferenceCountedACLCache#deserialize call it without null check
```
// code placeholder
Index j = ia.startVector("acls");
while (!j.done()) {
ACL acl = new ACL();
acl.deserialize(ia, "acl");
}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brettKK/zookeeper master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/495.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 #495
----
----
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r177343134
--- Diff: src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,16 +109,18 @@ public synchronized void deserialize(InputArchive ia) throws IOException {
}
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
- while (!j.done()) {
- ACL acl = new ACL();
- acl.deserialize(ia, "acl");
- aclList.add(acl);
- j.incr();
+ if (j != null) {
+ while (!j.done()) {
+ ACL acl = new ACL();
+ acl.deserialize(ia, "acl");
+ aclList.add(acl);
+ j.incr();
+ }
+ longKeyMap.put(val, aclList);
--- End diff --
+1
Only the inner while-loop uses the `j` variable, so nothing else should be inside the check.
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by maoling <gi...@git.apache.org>.
Github user maoling commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r177294282
--- Diff: src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,16 +109,18 @@ public synchronized void deserialize(InputArchive ia) throws IOException {
}
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
- while (!j.done()) {
- ACL acl = new ACL();
- acl.deserialize(ia, "acl");
- aclList.add(acl);
- j.incr();
+ if (j != null) {
--- End diff --
- can we have a more elegant way to process this NPE ?
- BTW. incorrect commit message. `git commit --amend -m "your new message"` to modify it
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by LJ1043041006 <gi...@git.apache.org>.
Github user LJ1043041006 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r177089527
--- Diff: src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,16 +109,18 @@ public synchronized void deserialize(InputArchive ia) throws IOException {
}
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
- while (!j.done()) {
- ACL acl = new ACL();
- acl.deserialize(ia, "acl");
- aclList.add(acl);
- j.incr();
+ if (j != null) {
+ while (!j.done()) {
+ ACL acl = new ACL();
+ acl.deserialize(ia, "acl");
+ aclList.add(acl);
+ j.incr();
+ }
+ longKeyMap.put(val, aclList);
--- End diff --
should we move 199~122 out of if to avoid endless loop?
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by brettKK <gi...@git.apache.org>.
Github user brettKK commented on the issue:
https://github.com/apache/zookeeper/pull/495
yeah， I will fix it @anmolnar @maoling @LJ1043041006
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
Ok, that (call site analysis) makes sense.
I'm afraid I was unclear, when I said
"You don't need an ERROR in the text here or on the next line."
What I mean is that the text string should not start with "ERROR" given the error string is in an exception and the logging (from one of the callers) will determine the severity to assign. As such my recommendation would be something like:
> throw new RuntimeException("Incorrect format of InputArchive when deserialize DataTree - missing acls");
Notice: 1) the removal of "ERROR" and the addition of "missing acls" in order to give the person diagnosing the problem a bit more insight (otw they have to find the source line in order to get more insight into what they formatting issue might be).
If you clear this up (this one line) I think we should be good for commit.
Thanks!
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by LJ1043041006 <gi...@git.apache.org>.
Github user LJ1043041006 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r177351128
--- Diff: src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,16 +109,18 @@ public synchronized void deserialize(InputArchive ia) throws IOException {
}
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
- while (!j.done()) {
- ACL acl = new ACL();
- acl.deserialize(ia, "acl");
- aclList.add(acl);
- j.incr();
+ if (j != null) {
--- End diff --
just as @anmolnar review, we should throw exception if j == null, so add code just like
if (j == null){
throw Exception(error_message);
}
@maoling can this change be elegant?
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/495
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
+1. Yes, it looks like those failures are unrelated (tests passed for me). Thanks @lujiefsi and @brettKK
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r183918644
--- Diff: src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,6 +109,10 @@ public synchronized void deserialize(InputArchive ia) throws IOException {
}
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
+ if (j == null) {
+ LOG.error("ERROR: incorrent format of InputArchive" + ia);
--- End diff --
You don't need an ERROR in the text here or on the next line. It's already LOG'd as an error. Same for the next line - it's a RTE.
Also you need a space "... of InputArchive" -> ".... of InputArchive " (notice space at the end. Otw the text of ia is just appended w/o the space. Also notice that ia doesn't have a toString, so I'm not sure how helpful that is.... it's fine to leave I guess.
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/495
@LJ1043041006 Looking good.
RuntimeException might suit better, but I'm not sure.
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by brettKK <gi...@git.apache.org>.
GitHub user brettKK reopened a pull request:
https://github.com/apache/zookeeper/pull/495
ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCache#deserialize
@LJ1043041006 found a potential NPE in ZK
----
callee BinaryInputArchive#startVector will return null:
```
// code placeholder
public Index startVector(String tag) throws IOException {
int len = readInt(tag);
if (len == -1) {
return null;
}
```
-----
and caller ReferenceCountedACLCache#deserialize call it without null check
```
// code placeholder
Index j = ia.startVector("acls");
while (!j.done()) {
ACL acl = new ACL();
acl.deserialize(ia, "acl");
}
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brettKK/zookeeper master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/495.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 #495
----
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
----
---
[GitHub] zookeeper pull request #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by brettKK <gi...@git.apache.org>.
Github user brettKK closed the pull request at:
https://github.com/apache/zookeeper/pull/495
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
Understood on throwing the exception (1&2). I'm interested in 3 - when it is thrown is it handled correctly or some unexpected sideeffect. If we're going to try to fix we should really ensure we fix it.
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by LJ1043041006 <gi...@git.apache.org>.
Github user LJ1043041006 commented on the issue:
https://github.com/apache/zookeeper/pull/495
@brettKK
so if we change code like this :
`Index j = ia.startVector("acls");
if (j == null){
throw new IOException("errmessage");
}
while (!j.done)
..........
`
@maoling this change can be elegant to check null?
@anmolnar this change can be correct?
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by lujiefsi <gi...@git.apache.org>.
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 #495: ZOOKEEPER-3007:Potential NPE in ReferenceCounte...
Posted by LJ1043041006 <gi...@git.apache.org>.
Github user LJ1043041006 commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/495#discussion_r177296763
--- Diff: src/java/main/org/apache/zookeeper/server/ReferenceCountedACLCache.java ---
@@ -109,16 +109,18 @@ public synchronized void deserialize(InputArchive ia) throws IOException {
}
List<ACL> aclList = new ArrayList<ACL>();
Index j = ia.startVector("acls");
- while (!j.done()) {
- ACL acl = new ACL();
- acl.deserialize(ia, "acl");
- aclList.add(acl);
- j.incr();
+ if (j != null) {
+ while (!j.done()) {
+ ACL acl = new ACL();
+ acl.deserialize(ia, "acl");
+ aclList.add(acl);
+ j.incr();
+ }
+ longKeyMap.put(val, aclList);
--- End diff --
Can we move code at line 119~123 out of null-checker, because it may cause endless loop due to i > 0 may always hold
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by lujiefsi <gi...@git.apache.org>.
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...
Posted by lujiefsi <gi...@git.apache.org>.
Github user lujiefsi commented on the issue:
https://github.com/apache/zookeeper/pull/495
Can we close it???
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/495
@brettKK Thanks for the fix. It'd be nice to add at least a unit test to cover the issue.
I think adding the check alone is not enough here. Looking at the `serialize()` method, if `map` field is greater than 0, both `long` and `acls` fields must also be present.
In other words, in `deserialize()` if (i>0) then both `long` and `acls` are mandatory. As a consequence the else branch of the check should also be implemented and an exception should be thrown indicating that the archive cannot be deserialise, because the format is incorrect.
Does it make sense?
---
[GitHub] zookeeper issue #495: ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCac...
Posted by lujiefsi <gi...@git.apache.org>.
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...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/495
I ran out of time to answer this question, perhaps you can tell me - what happens when the RTE is thrown? Is the caller handling it appropriately/reasonably, or are we just pushing the problem somewhere else?
---