From 2b079498fb633eee63c198c9863c10ff134b84ba Mon Sep 17 00:00:00 2001
From: Scott Mitchell <scott_mitchell@apple.com>
Date: Tue, 14 Feb 2017 19:22:13 -0800
Subject: [PATCH] OpenSslEngine may lose data if the non-application buffer is
 small/full

Motivation:
If an event occurs which generates non-application data (shutdown, handshake failure, alert generation, etc...) and the non-application buffer in the ByteBuffer BIO is full (or sufficiently small) we may not propagate all data to our peer before tearing down the socket.

Modifications:
- when wrap() detects the outbound is closed, but there is more data pending in the non-application buffers, we must also check if OpenSSL will generate more data from calling SSL_shutdown again
- when wrap() detects a handshakeExcpetion during failure we should check if OpenSSL has any pending data (in addition to the non-application buff) before throwing the handshake exception

Result:
OpenSslEngine more reliably transmits data to the peer before closing the socket.
---
 .../ssl/ReferenceCountedOpenSslEngine.java    | 61 ++++++++++++-------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
index 61d826d8a8..0d5c8d2e54 100644
--- a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
+++ b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -447,7 +447,15 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
                     // There is something left to drain.
                     // See https://github.com/netty/netty/issues/6260
                     bytesProduced = SSL.bioFlushByteBuffer(networkBIO);
-                    return newResultMayFinishHandshake(NOT_HANDSHAKING, 0, bytesProduced);
+                    if (bytesProduced <= 0) {
+                        return newResultMayFinishHandshake(NOT_HANDSHAKING, 0, 0);
+                    }
+                    // It is possible when the outbound was closed there was not enough room in the non-application
+                    // buffers to hold the close_notify. We should keep trying to close until we consume all the data
+                    // OpenSSL can give us.
+                    boolean didShutdown = doSSLShutdown();
+                    bytesProduced = bioLengthBefore - SSL.bioLengthByteBuffer(networkBIO);
+                    return newResultMayFinishHandshake(didShutdown ? NEED_WRAP : NOT_HANDSHAKING, 0, bytesProduced);
                 }
 
                 // Flush any data that may be implicitly generated by OpenSSL (handshake, close, etc..).
@@ -463,6 +471,11 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
                     bytesProduced = SSL.bioFlushByteBuffer(networkBIO);
 
                     if (bytesProduced > 0 && handshakeException != null) {
+                        // TODO(scott): It is possible that when the handshake failed there was not enough room in the
+                        // non-application buffers to hold the alert. We should get all the data before progressing on.
+                        // However I'm not aware of a way to do this with the OpenSSL APIs.
+                        // See https://github.com/netty/netty/issues/6385.
+
                         // We produced / consumed some data during the handshake, signal back to the caller.
                         // If there is a handshake exception and we have produced data, we should send the data before
                         // we allow handshake() to throw the handshake exception.
@@ -995,29 +1008,9 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
         outboundClosed = true;
 
         if (handshakeState != HandshakeState.NOT_STARTED && !isDestroyed()) {
-            if (SSL.isInInit(ssl) != 0) {
-                // Only try to call SSL_shutdown if we are not in the init state anymore.
-                // Otherwise we will see 'error:140E0197:SSL routines:SSL_shutdown:shutdown while in init' in our logs.
-                //
-                // See also http://hg.nginx.org/nginx/rev/062c189fee20
-                return;
-            }
-
             int mode = SSL.getShutdown(ssl);
             if ((mode & SSL.SSL_SENT_SHUTDOWN) != SSL.SSL_SENT_SHUTDOWN) {
-                int err = SSL.shutdownSSL(ssl);
-                if (err < 0) {
-                    int sslErr = SSL.getError(ssl, err);
-                    if (sslErr == SSL.SSL_ERROR_SYSCALL || sslErr == SSL.SSL_ERROR_SSL) {
-                        if (logger.isDebugEnabled()) {
-                            logger.debug("SSL_shutdown failed: OpenSSL error: {}", SSL.getLastError());
-                        }
-                        // There was an internal error -- shutdown
-                        shutdown();
-                    } else {
-                        SSL.clearError();
-                    }
-                }
+                doSSLShutdown();
             }
         } else {
             // engine closing before initial handshake
@@ -1025,6 +1018,30 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
         }
     }
 
+    private boolean doSSLShutdown() {
+        if (SSL.isInInit(ssl) != 0) {
+            // Only try to call SSL_shutdown if we are not in the init state anymore.
+            // Otherwise we will see 'error:140E0197:SSL routines:SSL_shutdown:shutdown while in init' in our logs.
+            //
+            // See also http://hg.nginx.org/nginx/rev/062c189fee20
+            return false;
+        }
+        int err = SSL.shutdownSSL(ssl);
+        if (err < 0) {
+            int sslErr = SSL.getError(ssl, err);
+            if (sslErr == SSL.SSL_ERROR_SYSCALL || sslErr == SSL.SSL_ERROR_SSL) {
+                if (logger.isDebugEnabled()) {
+                    logger.debug("SSL_shutdown failed: OpenSSL error: {}", SSL.getLastError());
+                }
+                // There was an internal error -- shutdown
+                shutdown();
+                return false;
+            }
+            SSL.clearError();
+        }
+        return true;
+    }
+
     @Override
     public final synchronized boolean isOutboundDone() {
         // Check if there is anything left in the outbound buffer.