Skip to content
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-3009] Potential NPE in NIOServerCnxnFactory #525

Closed
wants to merge 3 commits into from

Conversation

@lujiefsi
Copy link

lujiefsi commented May 21, 2018

We have developed a static analysis tool NPEDetector to find some potential NPE. Our analysis shows that NPE reason can be simple:some callees may return null directly in corner case(e.g. node crash , IO exception), some of their callers have !=null check but some do not have.

Bug:

Callee getSocketAddress can return null, may caused by node crash, network exception....

    public InetAddress getSocketAddress() {
        if (sock.isOpen() == false) {
            return null;
        }
        return sock.socket().getInetAddress();
    }

getSocketAddress has two callers: removeCnxn and removeCnxn

caller removeCnxn has null check

 public boolean removeCnxn(NIOServerCnxn cnxn) {
        //other code
        InetAddress addr = cnxn.getSocketAddress();
        if (addr != null) {
            Set set = ipMap.get(addr);
            //other code
        }
     //other code
    }

but caller addCnxn has the similar code, but don't have the null check:

     private void addCnxn(NIOServerCnxn cnxn) {
         InetAddress addr = cnxn.getSocketAddress();
         Set set = ipMap.get(addr);
        //other code
      }

I commit a patch to fix it: adding an null checker in addCnxn, and throws a semantics IOException rather than the ugly NPE. I think the IOException is ok, because the caller of addCnxn has the handler code:

  private void processAcceptedConnections() {
               //other code
               try {
                    addCnxn(cnxn);
               } catch (IOException e) {
                    // register, createConnection
                    cleanupSelectionKey(key);
                    fastCloseSock(accepted);
               }
             }

Maybe i am wrong, please point out my error.

LJ1043041006 added 2 commits May 21, 2018
InetAddress addr = cnxn.getSocketAddress();
if (addr == null) {
throw new IOException("Scoket of " + cnxn + " has been closed");

This comment has been minimized.

Copy link
@phunt

phunt May 21, 2018

Contributor

fix the spelling.

@phunt

This comment has been minimized.

Copy link
Contributor

phunt commented May 21, 2018

These are useful fixes. However one of the burdens of recommending such a fix is to also prove that the fix really addresses the issue, rather than just moving the problem somewhere else. In particular it would be good if the description of this fix (and any such fix) included details on whether all of the callers to addCnxn handle the new error handling code properly. In this case throwing the IOException in addCnxn. "some of their callers have !=null check but some do not have" One of the things any committer will need to do is also verify, and having this thought out ahead of time will help.

Another thing - are the null checks in the caller code still necessary? Should they be removed? If yes any such patch should include changes there as well.

LJ1043041006
@lujiefsi

This comment has been minimized.

Copy link
Author

lujiefsi commented May 22, 2018

Hi: @phunt
Great suggestions for me!!!! I have update the bug description:
(1) getSocketAddress only has two callers, just as shown in description.
(2)addCnxn only has one caller, and has the exception handler.

@lvfangmin

This comment has been minimized.

Copy link
Contributor

lvfangmin commented Jun 9, 2018

LGTM +1, the caller has handled the IOException properly.

@hanm
hanm approved these changes Jun 15, 2018
@asfgit asfgit closed this in 13dd5d0 Jun 15, 2018
asfgit pushed a commit that referenced this pull request Jun 15, 2018
We have developed a static analysis tool [NPEDetector](https://github.com/lujiefsi/NPEDetector) to find some potential NPE. Our analysis shows that NPE reason can be simple:some callees may return null directly in corner case(e.g. node crash , IO exception), some of their callers have  !=null check but some do not have.
### Bug:
Callee getSocketAddress can return null, may caused by node crash, network exception....
<pre>
    public InetAddress getSocketAddress() {
        if (sock.isOpen() == false) {
            return null;
        }
        return sock.socket().getInetAddress();
    }
</pre>
getSocketAddress has two callers:  removeCnxn and removeCnxn

caller removeCnxn has null check
<pre>
 public boolean removeCnxn(NIOServerCnxn cnxn) {
        //other code
        InetAddress addr = cnxn.getSocketAddress();
        if (addr != null) {
            Set<NIOServerCnxn> set = ipMap.get(addr);
            //other code
        }
     //other code
    }
</pre>

but caller addCnxn has the similar code, but don't have the null check:
<pre>
     private void addCnxn(NIOServerCnxn cnxn) {
         InetAddress addr = cnxn.getSocketAddress();
         Set<NIOServerCnxn> set = ipMap.get(addr);
        //other code
      }
</pre>
I commit a patch to fix it: adding an null checker in addCnxn, and throws a semantics IOException rather than the ugly NPE.  I think the IOException  is ok, because the caller of addCnxn has the handler code:
<pre>
  private void processAcceptedConnections() {
               //other code
               try {
                    addCnxn(cnxn);
               } catch (IOException e) {
                    // register, createConnection
                    cleanupSelectionKey(key);
                    fastCloseSock(accepted);
               }
             }
</pre>
Maybe i am wrong, please point out my error.

Author: LJ1043041006 <1239497420@qq.com>

Reviewers: Patrick Hunt <phunt@apache.org>, Allan Lyu <fangmin@apache.org>, Michael Han <hanm@apache.org>

Closes #525 from lujiefsi/ZOOKEEPER-3009

(cherry picked from commit 13dd5d0)
Signed-off-by: Michael Han <hanm@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.