7096: Synchronize Server Session Management and Network I/O

iocore/net/I_NetVConnection.h

@@ -387,6 +387,12 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
   */
   VIO *do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) override = 0;
 
+  virtual Continuation *
+  read_vio_cont()
+  {
+    return nullptr;
+  }
+
   /**
     Initiates write. Thread-safe, may be called when not handling
     an event from the NetVConnection, or the NetVConnection creation
@@ -423,6 +429,11 @@ class NetVConnection : public VConnection, public PluginUserArgs<TS_USER_ARGS_VC
   */
   VIO *do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *buf, bool owner = false) override = 0;
 
+  virtual Continuation *
+  write_vio_cont()
+  {
+    return nullptr;
+  }
   /**
     Closes the vconnection. A state machine MUST call do_io_close()
     when it has finished with a VConnection. do_io_close() indicates

iocore/net/P_UnixNetVConnection.h

@@ -111,6 +111,9 @@ class UnixNetVConnection : public NetVConnection, public NetEvent
   VIO *do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf) override;
   VIO *do_io_write(Continuation *c, int64_t nbytes, IOBufferReader *buf, bool owner = false) override;
 
+  Continuation *read_vio_cont() override;
+  Continuation *write_vio_cont() override;
+
   bool get_data(int id, void *data) override;
 
   Action *send_OOB(Continuation *cont, char *buf, int len) override;

iocore/net/SSLNetVConnection.cc

@@ -212,7 +212,6 @@ make_ssl_connection(SSL_CTX *ctx, SSLNetVConnection *netvc)
     }
 
     SSLNetVCAttach(ssl, netvc);
-    TLSSessionResumptionSupport::bind(ssl, netvc);
   }
 
   return ssl;
@@ -1821,7 +1820,6 @@ SSLNetVConnection::populate(Connection &con, Continuation *c, void *arg)
 
   sslHandshakeStatus = SSL_HANDSHAKE_DONE;
   SSLNetVCAttach(this->ssl, this);
-  TLSSessionResumptionSupport::bind(this->ssl, this);
   return EVENT_DONE;
 }
 

iocore/net/SSLUtils.cc

@@ -1700,12 +1700,14 @@ void
 SSLNetVCAttach(SSL *ssl, SSLNetVConnection *vc)
 {
   SSL_set_ex_data(ssl, ssl_vc_index, vc);
+  TLSSessionResumptionSupport::bind(ssl, vc);
 }
 
 void
 SSLNetVCDetach(SSL *ssl)
 {
   SSL_set_ex_data(ssl, ssl_vc_index, nullptr);
+  TLSSessionResumptionSupport::unbind(ssl);
 }
 
 SSLNetVConnection *

iocore/net/TLSSessionResumptionSupport.cc

@@ -75,6 +75,12 @@ TLSSessionResumptionSupport::bind(SSL *ssl, TLSSessionResumptionSupport *srs)
   SSL_set_ex_data(ssl, _ex_data_index, srs);
 }
 
+void
+TLSSessionResumptionSupport::unbind(SSL *ssl)
+{
+  SSL_set_ex_data(ssl, _ex_data_index, nullptr);
+}
+
 int
 TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyname, unsigned char *iv, EVP_CIPHER_CTX *cipher_ctx,
                                                   HMAC_CTX *hctx, int enc)

iocore/net/TLSSessionResumptionSupport.h

@@ -38,6 +38,7 @@ class TLSSessionResumptionSupport
   static void initialize();
   static TLSSessionResumptionSupport *getInstance(SSL *ssl);
   static void bind(SSL *ssl, TLSSessionResumptionSupport *srs);
+  static void unbind(SSL *ssl);
 
   int processSessionTicket(SSL *ssl, unsigned char *keyname, unsigned char *iv, EVP_CIPHER_CTX *cipher_ctx, HMAC_CTX *hctx,
                            int enc);

iocore/net/UnixNetVConnection.cc

@@ -79,9 +79,12 @@ static inline int
 read_signal_and_update(int event, UnixNetVConnection *vc)
 {
   vc->recursion++;
-  if (vc->read.vio.cont) {
+  if (vc->read.vio.cont && vc->read.vio.mutex == vc->read.vio.cont->mutex) {
     vc->read.vio.cont->handleEvent(event, &vc->read.vio);
   } else {
+    if (vc->read.vio.cont) {
+      Note("read_signal_and_update: mutexes are different? vc=%p, event=%d", vc, event);
+    }
     switch (event) {
     case VC_EVENT_EOS:
     case VC_EVENT_ERROR:
@@ -111,9 +114,12 @@ static inline int
 write_signal_and_update(int event, UnixNetVConnection *vc)
 {
   vc->recursion++;
-  if (vc->write.vio.cont) {
+  if (vc->write.vio.cont && vc->write.vio.mutex == vc->write.vio.cont->mutex) {
     vc->write.vio.cont->handleEvent(event, &vc->write.vio);
   } else {
+    if (vc->write.vio.cont) {
+      Note("write_signal_and_update: mutexes are different? vc=%p, event=%d", vc, event);
+    }
     switch (event) {
     case VC_EVENT_EOS:
     case VC_EVENT_ERROR:
@@ -628,19 +634,46 @@ UnixNetVConnection::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader
   return &write.vio;
 }
 
+Continuation *
+UnixNetVConnection::read_vio_cont()
+{
+  return read.vio.cont;
+}
+
+Continuation *
+UnixNetVConnection::write_vio_cont()
+{
+  return write.vio.cont;
+}
+
 void
 UnixNetVConnection::do_io_close(int alerrno /* = -1 */)
 {
   // FIXME: the nh must not nullptr.
   ink_assert(nh);
 
-  read.enabled  = 0;
-  write.enabled = 0;
-  read.vio.buffer.clear();
+  // mark it closed first
+  if (alerrno == -1) {
+    closed = 1;
+  } else {
+    closed = -1;
+  }
+  read.enabled    = 0;
+  write.enabled   = 0;
   read.vio.nbytes = 0;
   read.vio.op     = VIO::NONE;
   read.vio.cont   = nullptr;
-  write.vio.buffer.clear();
+
+  if (netvc_context == NET_VCONNECTION_OUT) {
+    // do not clear the iobufs yet to guard
+    // against race condition with session pool closing
+    Debug("iocore_net", "delay vio buffer clear to protect against  race for vc %p", this);
+  } else {
+    // may be okay to delay for all VCs?
+    read.vio.buffer.clear();
+    write.vio.buffer.clear();
+  }
+
   write.vio.nbytes = 0;
   write.vio.op     = VIO::NONE;
   write.vio.cont   = nullptr;
@@ -652,11 +685,6 @@ UnixNetVConnection::do_io_close(int alerrno /* = -1 */)
   if (alerrno && alerrno != -1) {
     this->lerrno = alerrno;
   }
-  if (alerrno == -1) {
-    closed = 1;
-  } else {
-    closed = -1;
-  }
 
   if (close_inline) {
     if (nh) {
@@ -1316,6 +1344,10 @@ UnixNetVConnection::clear()
   read.vio.vc_server  = nullptr;
   write.vio.vc_server = nullptr;
   options.reset();
+  if (netvc_context == NET_VCONNECTION_OUT) {
+    read.vio.buffer.clear();
+    write.vio.buffer.clear();
+  }
   closed        = 0;
   netvc_context = NET_VCONNECTION_UNSET;
   ink_assert(!read.ready_link.prev && !read.ready_link.next);

proxy/http/Http1ServerSession.cc

@@ -185,9 +185,10 @@ Http1ServerSession::release()
     return;
   }
 
-  // Make sure the vios for the current SM are cleared
-  server_vc->do_io_read(nullptr, 0, nullptr);
-  server_vc->do_io_write(nullptr, 0, nullptr);
+  // do not change the read/write cont and mutex yet
+  // release_session() will either swap them with the
+  // pool continuation with a valid read buffer or if
+  // it fails, do_io_close() will clear the cont anyway
 
   HSMresult_t r = httpSessionManager.release_session(this);
 

proxy/http/HttpSM.cc

@@ -433,7 +433,10 @@ HttpSM::state_add_to_list(int event, void * /* data ATS_UNUSED */)
       // Seems like ua_entry->read_vio->disable(); should work, but that was
       // not sufficient to stop the state machine from processing IO events until the
       // TXN_START hooks had completed
-      ua_entry->read_vio = ua_entry->vc->do_io_read(nullptr, 0, nullptr);
+      // Preserve the current read cont and mutex
+      NetVConnection *netvc = ((ProxyTransaction *)ua_entry->vc)->get_netvc();
+      ink_assert(netvc != nullptr);
+      ua_entry->read_vio = ua_entry->vc->do_io_read(netvc->read_vio_cont(), 0, nullptr);
     }
     return EVENT_CONT;
   }
@@ -577,7 +580,8 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc, IOBufferReader *buffe
   //  this hook maybe asynchronous, we need to disable IO on
   //  client but set the continuation to be the state machine
   //  so if we get an timeout events the sm handles them
-  ua_entry->read_vio  = client_vc->do_io_read(this, 0, buffer_reader->mbuf);
+  //  hold onto enabling read until setup_client_read_request_header
+  ua_entry->read_vio  = client_vc->do_io_read(this, 0, nullptr);
   ua_entry->write_vio = client_vc->do_io_write(this, 0, nullptr);
 
   /////////////////////////
@@ -6107,7 +6111,8 @@ HttpSM::attach_server_session(Http1ServerSession *s)
   // first tunnel instead of the producer of the second tunnel.
   // The real read is setup in setup_server_read_response_header()
   //
-  server_entry->read_vio = server_session->do_io_read(this, 0, server_session->read_buffer);
+  // Keep the read disabled until setup_server_read_response_header
+  server_entry->read_vio = server_session->do_io_read(this, 0, nullptr);
 
   // Transfer control of the write side as well
   server_entry->write_vio = server_session->do_io_write(this, 0, nullptr);

proxy/http/HttpSessionManager.cc

@@ -370,12 +370,9 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
     to_return = nullptr;
   }
 
-  // TS-3797 Adding another scope so the pool lock is dropped after it is removed from the pool and
-  // potentially moved to the current thread.  At the end of this scope, either the original
-  // pool selected VC is on the current thread or its content has been moved to a new VC on the
-  // current thread and the original has been deleted. This should adequately cover TS-3266 so we
-  // don't have to continue to hold the pool thread while we initialize the server session in the
-  // client session
+  // Extend the mutex window until the acquired Server session is attached
+  // to the SM. Releasing the mutex before that results in race conditions
+  // due to a potential parallel network read on the VC with no mutex guarding
   {
     // Now check to see if we have a connection in our shared connection pool
     EThread *ethread = this_ethread();
@@ -396,6 +393,10 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
         if (to_return) {
           UnixNetVConnection *server_vc = dynamic_cast<UnixNetVConnection *>(to_return->get_netvc());
           if (server_vc) {
+            // Disable i/o on this vc now, but, hold onto the g_pool cont
+            // and the mutex to stop any stray events from getting in
+            server_vc->do_io_read(m_g_pool, 0, nullptr);
+            server_vc->do_io_write(m_g_pool, 0, nullptr);
             UnixNetVConnection *new_vc = server_vc->migrateToCurrentThread(sm, ethread);
             // The VC moved, free up the original one
             if (new_vc != server_vc) {
@@ -421,14 +422,14 @@ HttpSessionManager::acquire_session(Continuation * /* cont ATS_UNUSED */, sockad
     } else { // Didn't get the lock.  to_return is still NULL
       retval = HSM_RETRY;
     }
-  }
 
-  if (to_return) {
-    Debug("http_ss", "[%" PRId64 "] [acquire session] return session from shared pool", to_return->con_id);
-    to_return->state = HSS_ACTIVE;
-    // the attach_server_session will issue the do_io_read under the sm lock
-    sm->attach_server_session(to_return);
-    retval = HSM_DONE;
+    if (to_return) {
+      Debug("http_ss", "[%" PRId64 "] [acquire session] return session from shared pool", to_return->con_id);
+      to_return->state = HSS_ACTIVE;
+      // the attach_server_session will issue the do_io_read under the sm lock
+      sm->attach_server_session(to_return);
+      retval = HSM_DONE;
+    }
   }
   return retval;
 }