diff --git a/iocore/net/I_NetVConnection.h b/iocore/net/I_NetVConnection.h index 0609e1857eb..fac5762f025 100644 --- a/iocore/net/I_NetVConnection.h +++ b/iocore/net/I_NetVConnection.h @@ -387,6 +387,12 @@ class NetVConnection : public VConnection, public PluginUserArgsssl, this); - TLSSessionResumptionSupport::bind(this->ssl, this); return EVENT_DONE; } diff --git a/iocore/net/SSLUtils.cc b/iocore/net/SSLUtils.cc index 442aec3c278..79be61410a6 100644 --- a/iocore/net/SSLUtils.cc +++ b/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 * diff --git a/iocore/net/TLSSessionResumptionSupport.cc b/iocore/net/TLSSessionResumptionSupport.cc index 8935c990705..ab6f38e5d14 100644 --- a/iocore/net/TLSSessionResumptionSupport.cc +++ b/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) diff --git a/iocore/net/TLSSessionResumptionSupport.h b/iocore/net/TLSSessionResumptionSupport.h index 466618b9e9b..f1e61c911ee 100644 --- a/iocore/net/TLSSessionResumptionSupport.h +++ b/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); diff --git a/iocore/net/UnixNetVConnection.cc b/iocore/net/UnixNetVConnection.cc index c1c608b9836..840469a90f0 100644 --- a/iocore/net/UnixNetVConnection.cc +++ b/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); diff --git a/proxy/http/Http1ServerSession.cc b/proxy/http/Http1ServerSession.cc index 2adaf28ff41..813a729d6aa 100644 --- a/proxy/http/Http1ServerSession.cc +++ b/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); diff --git a/proxy/http/HttpSM.cc b/proxy/http/HttpSM.cc index 611e2732be7..3832a213f17 100644 --- a/proxy/http/HttpSM.cc +++ b/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); diff --git a/proxy/http/HttpSessionManager.cc b/proxy/http/HttpSessionManager.cc index 7d0c31cdb49..bf5ea8d1fb0 100644 --- a/proxy/http/HttpSessionManager.cc +++ b/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(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; }