ZOOKEEPER-3007:Potential NPE in ReferenceCountedACLCache#deserialize

[brettKK]

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

[maoling]

• 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

[lujiefsi]

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?

[lujiefsi]

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

[anmolnar]

+1
Only the inner while-loop uses the j variable, so nothing else should be inside the check.

[anmolnar]

@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 deserialised, because the format is incorrect.

Does it make sense?

[lujiefsi]

@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?

[anmolnar]

@LJ1043041006 Looking good.
RuntimeException might suit better, but I'm not sure.

[lujiefsi]

I have run the test, +1 LGTM

[lujiefsi]

Can we close it???

[phunt]

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.

[phunt]

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?

[lujiefsi]

@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) .

[phunt]

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.

[lujiefsi]

@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.

[phunt]

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 2) 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!

[lujiefsi]

Seems that unit test error is not caused by this patch?

[phunt]

+1. Yes, it looks like those failures are unrelated (tests passed for me). Thanks @lujiefsi and @brettKK