Re: trafficserver git commit: TS-2709: ATS does not send close-notify on shutdown.

[Susan Hinrichs]

On 3/26/2015 3:23 PM, James Peach wrote:
On Mar 26, 2015, at 1:18 PM, shinr...@apache.org wrote:

[snip]
----------------------------------------------------------------------
diff --git a/iocore/net/SSLNetVConnection.cc b/iocore/net/SSLNetVConnection.cc
index 3d58072..a1988f2 100644
--- a/iocore/net/SSLNetVConnection.cc
+++ b/iocore/net/SSLNetVConnection.cc
@@ -762,6 +762,43 @@ SSLNetVConnection::SSLNetVConnection()
{
}

+void
+SSLNetVConnection::do_io_close(int lerrno)
+{
+  if (this->ssl != NULL && sslHandShakeComplete) {
+    int new_shutdown_mode = 0, shutdown_mode = 0;
+    if (this->lerrno < 0) {
Why do you look at VConnection::lerrno instead of the lerrno argument?

Hmm.  Good catch.  I started with someone else's patch.  They had the 
code in a UnixNetVConnection free close function.  Seemed more 
appropriate to move it back to a virtual override of do_io_close on 
SSLNetVConnection.  But when I did that, I should have changed the logic 
to use the lerrno argument rather than the lerrno member (which would 
have already been set to the calling method's lerrno argument values in 
the original patch).

I'll tidy that up.


+      new_shutdown_mode = SSL_SENT_SHUTDOWN|SSL_RECEIVED_SHUTDOWN;
+    } else {
+      shutdown_mode = SSL_get_shutdown(ssl);
+      Debug("ssl-shutdown", "previous shutdown state 0x%x", shutdown_mode);
+      new_shutdown_mode = shutdown_mode | SSL_RECEIVED_SHUTDOWN;
+    }
+    if (new_shutdown_mode != shutdown_mode) {
+      // We do not need to sit around and wait for the client's close-notify if
+      // they have not already sent it.  We will still be standards compliant
+      Debug("ssl-shutdown", "new SSL_set_shutdown 0x%x", new_shutdown_mode);
+      SSL_set_shutdown(ssl, new_shutdown_mode);
+    }
+
+    // If the peer has already sent a FIN, don't bother with the shutdown
+    // They will just send us a RST for our troubles
+    // This test is not foolproof.  The client's fin could be on the wire
+    // at the same time we send the close-notify.  If so, the client will 
likely
+    // send RST anyway
+    char c;
+    ssize_t x = recv(this->con.fd, &c, 1, MSG_PEEK);
+    // x < 0 means error.  x == 0 means fin sent
+    if (x != 0) {
+      // Send the close-notify
+      int ret = SSL_shutdown(ssl);
+      Debug("ssl-shutdown", "SSL_shutdown %s", (ret)?"success":"failed");
+    }
+  }
+  // Go on and do the unix socket cleanups
+  super::do_io_close(lerrno);
+}
+
void
SSLNetVConnection::free(EThread *t)
{
@@ -780,8 +817,6 @@ SSLNetVConnection::free(EThread *t)
   closed = 0;
   ink_assert(con.fd == NO_FD);
   if (ssl != NULL) {
-    /*if (sslHandShakeComplete)
-       SSL_set_shutdown(ssl, SSL_SENT_SHUTDOWN|SSL_RECEIVED_SHUTDOWN); */
     SSL_free(ssl);
     ssl = NULL;
   }

http://git-wip-us.apache.org/repos/asf/trafficserver/blob/03734d05/iocore/net/SSLUtils.cc
----------------------------------------------------------------------
diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc
index b813aee..75a44a7 100644
--- a/iocore/net/SSLUtils.cc
+++ b/iocore/net/SSLUtils.cc
@@ -1238,8 +1238,6 @@ SSLInitServerContext(const SSLConfigParams *params, const 
ssl_user_config &sslMu
   SSL_CTX_set_options(ctx, SSL_OP_SAFARI_ECDHE_ECDSA_BUG);
#endif

-  SSL_CTX_set_quiet_shutdown(ctx, 1);
-
   // pass phrase dialog configuration
   passphrase_cb_userdata ud(params, sslMultCertSettings.dialog, 
sslMultCertSettings.first_cert, sslMultCertSettings.key);