elecharny commented on code in PR #77: URL: https://github.com/apache/mina/pull/77#discussion_r4236271027
########## mina-core/src/main/java/org/apache/mina/filter/ssl/SSLHandlerG1.java: ########## Review Comment: I have tracked down the root of this bad code. We have had a discussion 4 years ago about some corner case where it's needed to send a close_notification alert message to the remote peer before shutting down the connection. That was what the code was about. The import mail is this one: https://lists.apache.org/thread.html/2dvnzz429d0d5xfg0bypvjnzmsdkfj8f So when we deal with pending errors we suppose the inbound has been closed, thus we don't get up to the point the message is used causing a NPE. This is a brittle assumption, typically I'm not sure the inbound has been closed (it *seems* to be the case). Let's assume it's not, we really need to check for the message nullity before going any further. Something like: ``` if (message==null) { if ( mPendingError != null ) { throw mPendingError; } else { throw new IllegalStateException("closed"); } } ``` added in the receive_loop just after the `if (mEngine.isInboundDone()) {` block would do the trick (in both G0 and G1 handlers) This is of course also assuming the message will *only* be null when we are processing an error. There is another option: don't call receive_loop at all, and deal with the error in the `throw_pending_error` method: ``` synchronized protected void throw_pending_error(NextFilter next) throws SSLException { SSLException sslException = mPendingError; if (sslException != null) { // Send back the alert messages if (mEngine.isInboundDone() && mEngine.getHandshakeStatus() == HandshakeStatus.NEED_WRAP) { if (LOGGER.isDebugEnabled()) { LOGGER.debug("{} throw_pending_error() - handshake needs wrap, invoking write", toString()); } write_handshake(next); } mPendingError = null; throw sslException; } } ``` The second solution seems much clearer. Do we agree with that? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
