diff --git a/groups/ntc/ntcd/ntcd_encryption.cpp b/groups/ntc/ntcd/ntcd_encryption.cpp index e2bde306..ef602682 100644 --- a/groups/ntc/ntcd/ntcd_encryption.cpp +++ b/groups/ntc/ntcd/ntcd_encryption.cpp @@ -1366,13 +1366,6 @@ ntsa::Error Encryption::processIncomingHello() d_shutdownState.close(); d_handshakeState = e_FAILED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), - bsl::shared_ptr(), - "Not authorized"); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } else { error = this->enqueueOutgoingAccept(true); @@ -1385,11 +1378,6 @@ ntsa::Error Encryption::processIncomingHello() if (d_role == ntca::EncryptionRole::e_CLIENT) { BSLS_ASSERT(d_remoteCertificate_sp); d_handshakeState = e_ESTABLISHED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(), d_remoteCertificate_sp, ""); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } else { error = this->enqueueOutgoingHello(); @@ -1438,23 +1426,11 @@ ntsa::Error Encryption::processIncomingAccept() if (d_role == ntca::EncryptionRole::e_SERVER) { BSLS_ASSERT(d_remoteCertificate_sp); d_handshakeState = e_ESTABLISHED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(), d_remoteCertificate_sp, ""); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } } else { d_shutdownState.close(); d_handshakeState = e_FAILED; - { - ntccfg::UnLockGuard guard(&d_mutex); - d_handshakeCallback(ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), - bsl::shared_ptr(), - "Not authorized"); - } - d_handshakeCallback = ntci::Encryption::HandshakeCallback(); } return ntsa::Error(); @@ -1518,7 +1494,7 @@ ntsa::Error Encryption::processIncomingGoodbye() return ntsa::Error(); } -ntsa::Error Encryption::process() +ntsa::Error Encryption::processData() { ntsa::Error error; @@ -1568,6 +1544,49 @@ ntsa::Error Encryption::process() return ntsa::Error(); } +ntsa::Error Encryption::process(LockGuard* lock) +{ + ntsa::Error error = this->processData(); + + // Announce the outcome of the handshake only after all complete incoming + // frames have been consumed, so that a concurrent caller cannot observe + // (and re-process) a partially-processed frame while the mutex is + // released. The handshake callback is reset when the outcome is + // announced, so the outcome is announced at most once. + + if (d_handshakeCallback && (d_handshakeState == e_ESTABLISHED || + d_handshakeState == e_FAILED)) + { + ntci::Encryption::HandshakeCallback handshakeCallback = + d_handshakeCallback; + + d_handshakeCallback = ntci::Encryption::HandshakeCallback(); + + const bool established = d_handshakeState == e_ESTABLISHED; + + bsl::shared_ptr remoteCertificate; + if (established) { + remoteCertificate = d_remoteCertificate_sp; + } + + // Do not re-acquire the mutex after the handshake callback returns: + // the handshake callback may destroy this object. + + lock->release()->unlock(); + + if (established) { + handshakeCallback(ntsa::Error(), remoteCertificate, ""); + } + else { + handshakeCallback(ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), + remoteCertificate, + "Not authorized"); + } + } + + return error; +} + Encryption::Encryption( ntca::EncryptionRole::Value role, const bsl::shared_ptr& certificate, @@ -1627,7 +1646,7 @@ ntsa::Error Encryption::initiateHandshake(const HandshakeCallback& callback) d_handshakeState = e_HELLO_SENT; } - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushIncomingCipherText(const bdlbb::Blob& input) @@ -1640,7 +1659,7 @@ ntsa::Error Encryption::pushIncomingCipherText(const bdlbb::Blob& input) bdlbb::BlobUtil::append(d_incomingCipherText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushIncomingCipherText(const ntsa::Data& input) @@ -1653,7 +1672,7 @@ ntsa::Error Encryption::pushIncomingCipherText(const ntsa::Data& input) ntsa::DataUtil::append(d_incomingCipherText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushOutgoingPlainText(const bdlbb::Blob& input) @@ -1666,7 +1685,7 @@ ntsa::Error Encryption::pushOutgoingPlainText(const bdlbb::Blob& input) bdlbb::BlobUtil::append(d_outgoingPlainText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::pushOutgoingPlainText(const ntsa::Data& input) @@ -1679,7 +1698,7 @@ ntsa::Error Encryption::pushOutgoingPlainText(const ntsa::Data& input) ntsa::DataUtil::append(d_outgoingPlainText_sp.get(), input); - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::popIncomingPlainText(bdlbb::Blob* output) @@ -1693,7 +1712,7 @@ ntsa::Error Encryption::popIncomingPlainText(bdlbb::Blob* output) d_incomingPlainText_sp->length()); } - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::popOutgoingCipherText(bdlbb::Blob* output) @@ -1707,7 +1726,7 @@ ntsa::Error Encryption::popOutgoingCipherText(bdlbb::Blob* output) d_outgoingCipherText_sp->length()); } - return this->process(); + return this->process(&guard); } ntsa::Error Encryption::shutdown() diff --git a/groups/ntc/ntcd/ntcd_encryption.h b/groups/ntc/ntcd/ntcd_encryption.h index a772db47..cb2f10c3 100644 --- a/groups/ntc/ntcd/ntcd_encryption.h +++ b/groups/ntc/ntcd/ntcd_encryption.h @@ -682,7 +682,17 @@ class Encryption : public ntci::Encryption /// Process the available data through the encryption state machine. /// Return the error. - ntsa::Error process(); + ntsa::Error processData(); + + /// Process the available data through the encryption state machine. + /// If the handshake has completed or failed and that outcome has not + /// yet been announced, release the specified 'lock', unlock the mutex, + /// and invoke the handshake callback. Return the error. Note that + /// the handshake callback is invoked only after all complete incoming + /// frames have been consumed, and the mutex is not re-acquired after the + /// handshake callback returns, so the handshake callback may safely + /// destroy this object. + ntsa::Error process(LockGuard* lock); public: /// Create a new encryption operating in the specified 'role' from an diff --git a/groups/ntc/ntci/ntci_encryption.h b/groups/ntc/ntci/ntci_encryption.h index d64f72f4..05e3e0ed 100644 --- a/groups/ntc/ntci/ntci_encryption.h +++ b/groups/ntc/ntci/ntci_encryption.h @@ -50,6 +50,16 @@ namespace ntci { /// @par Thread Safety /// This class is thread safe. /// +/// @par Handshake Callback Invocation +/// Implementations must invoke the handshake callback only from within a call +/// to 'initiateHandshake' or 'pushIncomingCipherText', i.e., only from within +/// the call that supplies the input that determines the outcome of the +/// handshake. Implementations must not invoke the handshake callback from +/// within any other function, in particular 'pushOutgoingPlainText', +/// 'popIncomingPlainText', 'popOutgoingCipherText', or 'shutdown'. Users of +/// this class rely on this guarantee to call those other functions without +/// being prepared to process the completion or failure of the handshake. +/// /// @ingroup module_ntci_encryption class Encryption { @@ -58,7 +68,9 @@ class Encryption /// a handshake is complete. The first parameter indicates the error, /// if any. The second parameter indicates the certificate of the peer, /// if any. The third parameters indicates the cause of the handshake - /// error, if an error is indicated. + /// error, if an error is indicated. Note that this function is invoked + /// only from within a call to 'initiateHandshake' or + /// 'pushIncomingCipherText'. typedef NTCCFG_FUNCTION( const ntsa::Error& error, const bsl::shared_ptr& certificate, @@ -68,30 +80,37 @@ class Encryption virtual ~Encryption(); /// Initiate the handshake to begin the session. Invoke the specified - /// 'callback' when the handshake completes. Return the error. + /// 'callback' when the handshake completes, either from within this + /// call or from within a subsequent call to 'pushIncomingCipherText'. + /// Return the error. virtual ntsa::Error initiateHandshake(const HandshakeCallback& callback); /// Initiate the handshake to begin the session according to the specified /// 'upgradeOptions'. Invoke the specified 'callback' when the handshake - /// completes. Return the error. + /// completes, either from within this call or from within a subsequent + /// call to 'pushIncomingCipherText'. Return the error. virtual ntsa::Error initiateHandshake( const ntca::UpgradeOptions& upgradeOptions, const HandshakeCallback& callback); /// Add the specified 'input' containing ciphertext read from the peer. - /// Return the error. + /// Invoke the handshake callback if 'input' completes the handshake or + /// causes it to fail. Return the error. virtual ntsa::Error pushIncomingCipherText(const bdlbb::Blob& input); /// Add the specified 'input' containing ciphertext read from the peer. - /// Return the error. + /// Invoke the handshake callback if 'input' completes the handshake or + /// causes it to fail. Return the error. virtual ntsa::Error pushIncomingCipherText(const ntsa::Data& input); /// Add the specified 'input' containing plaintext to be sent to the - /// peer. Return the error. + /// peer. Return the error. Note that this function never invokes the + /// handshake callback. virtual ntsa::Error pushOutgoingPlainText(const bdlbb::Blob& input); /// Add the specified 'input' containing plaintext to be sent to the - /// peer. Return the error. + /// peer. Return the error. Note that this function never invokes the + /// handshake callback. virtual ntsa::Error pushOutgoingPlainText(const ntsa::Data& input); /// Pop plaintext read from the peer and append it to the specified diff --git a/groups/ntc/ntcp/ntcp_streamsocket.cpp b/groups/ntc/ntcp/ntcp_streamsocket.cpp index b5a5c29a..a083d4bb 100644 --- a/groups/ntc/ntcp/ntcp_streamsocket.cpp +++ b/groups/ntc/ntcp/ntcp_streamsocket.cpp @@ -215,7 +215,10 @@ void StreamSocket::processSocketConnected(const ntsa::Error& error) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Connection completion is cross-cutting; acquire both mutexes in + // canonical order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -245,7 +248,20 @@ void StreamSocket::processSocketReceived(const ntsa::Error& error, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) + { + return; + } + + ntsa::Error outcome; + + // The receive completion holds the receive mutex throughout. Whenever it + // must perform a cross-cutting transition (completing or failing the + // handshake, downgrading, shutting down, or failing), it additionally + // acquires the send mutex, which the lock order permits, without releasing + // the receive mutex. + + LockGuard receiveLock(&d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -253,27 +269,80 @@ void StreamSocket::processSocketReceived(const ntsa::Error& error, } NTCI_LOG_CONTEXT(); - NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); d_receivePending = false; + if (NTCCFG_UNLIKELY(d_encryption_sp && d_upgradeInProgress)) { + // While the handshake is in progress, decrypting may complete or fail + // the handshake, so hold both mutexes while processing the completion. + + LockGuard sendLock(&d_sendMutex); + + if (error) { + if (error != ntsa::Error::e_CANCELLED) { + NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); + outcome = error; + } + } + else { + NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); + outcome = this->privateCompleteReceive(self, + context.bytesReceivable(), + context.bytesReceived(), + true); + } + + if (outcome == ntsa::Error::e_EOF) { + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); + } + else if (outcome) { + this->privateFailReceive(self, outcome); + } + else { + this->privateInitiateReceive(self); + } + + return; + } + + // Fast path: an unencrypted receive completion, or an encrypted receive + // completion once the handshake has completed, only needs the receive + // mutex. + if (error) { if (error != ntsa::Error::e_CANCELLED) { NTCP_STREAMSOCKET_LOG_RECEIVE_FAILURE(error); - this->privateFailReceive(self, error); + outcome = error; } } else { NTCP_STREAMSOCKET_LOG_RECEIVE_RESULT(context); - this->privateCompleteReceive(self, - context.bytesReceivable(), - context.bytesReceived()); + outcome = this->privateCompleteReceive(self, + context.bytesReceivable(), + context.bytesReceived(), + false); + } + + if (outcome == ntsa::Error::e_EOF) { + LockGuard sendLock(&d_sendMutex); + + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); } + else if (outcome) { + LockGuard sendLock(&d_sendMutex); - this->privateInitiateReceive(self); + this->privateFailReceive(self, outcome); + } + else { + this->privateInitiateReceive(self); + } } void StreamSocket::processSocketSent(const ntsa::Error& error, @@ -283,7 +352,7 @@ void StreamSocket::processSocketSent(const ntsa::Error& error, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -318,7 +387,10 @@ void StreamSocket::processSocketError(const ntsa::Error& error) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Error handling is cross-cutting (it fails the whole socket); acquire + // both mutexes in canonical order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -338,7 +410,11 @@ void StreamSocket::processSocketDetached() { NTCCFG_OBJECT_GUARD(&d_object); - LockGuard lock(&d_mutex); + // The detachment completion runs the deferred shutdown/connect-failure + // completion, which is cross-cutting; acquire both mutexes in + // canonical order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); @@ -364,7 +440,8 @@ void StreamSocket::processConnectDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -406,7 +483,8 @@ void StreamSocket::processConnectRetryTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -444,7 +522,8 @@ void StreamSocket::processUpgradeTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -473,7 +552,7 @@ void StreamSocket::processSendRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -502,7 +581,7 @@ void StreamSocket::processSendRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_sendMutex); } } } @@ -518,7 +597,7 @@ void StreamSocket::processSendDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -552,7 +631,7 @@ void StreamSocket::processSendDeadlineTimer( d_proactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } } @@ -567,7 +646,7 @@ void StreamSocket::processReceiveRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -596,7 +675,7 @@ void StreamSocket::processReceiveRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -612,7 +691,7 @@ void StreamSocket::processReceiveDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -640,7 +719,7 @@ void StreamSocket::processReceiveDeadlineTimer( d_proactorStrand_sp, self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -650,8 +729,14 @@ void StreamSocket::privateEncryptionHandshake( const bsl::shared_ptr& certificate, const bsl::string& details) { - // IMPLEMENTATION NOTE: This function is always called under a lock - // during the exection of 'd_encryption->popIncomingCipherText()'. + // IMPLEMENTATION NOTE: This function is always called while holding + // 'd_receiveMutex' and 'd_sendMutex', so the callback is deferred. The + // encryption session invokes this function only from within + // 'initiateHandshake' or 'pushIncomingCipherText' (see 'ntci::Encryption'), + // which are called only from 'privateUpgrade' and the encrypted receive + // completion while the handshake is in progress, each of which holds both + // mutexes. In particular, this function is never called from the + // encrypted send path, which holds only 'd_sendMutex'. NTCI_LOG_CONTEXT(); @@ -681,8 +766,8 @@ void StreamSocket::privateEncryptionHandshake( event, d_proactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } } else { @@ -833,6 +918,9 @@ void StreamSocket::privateCompleteConnect( NTCI_LOG_TRACE("Connection attempt succeeded"); + // Connection establishment runs while holding both mutexes, so all + // announcements and callbacks are deferred. + if (d_session_sp) { ntcs::Dispatch::announceConnectComplete(d_session_sp, self, @@ -840,8 +928,8 @@ void StreamSocket::privateCompleteConnect( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, - &d_mutex); + true, + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return; @@ -853,8 +941,8 @@ void StreamSocket::privateCompleteConnect( connectEvent, d_proactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return; @@ -866,8 +954,8 @@ void StreamSocket::privateCompleteConnect( d_managerStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } void StreamSocket::privateFailConnect( @@ -1013,6 +1101,12 @@ void StreamSocket::privateFailConnectComplete( bool defer) { NTCI_LOG_CONTEXT(); + + // This cleanup always runs while holding both mutexes (inline from + // 'privateFailConnect' or from the deferred detachment completion), so all + // announcements and callbacks are deferred. + NTCCFG_WARNING_UNUSED(defer); + if (d_systemHandle != ntsa::k_INVALID_HANDLE) { if (d_socket_sp) { ntcs::ObserverRef proactorRef(&d_proactor); @@ -1045,8 +1139,8 @@ void StreamSocket::privateFailConnectComplete( d_sessionStrand_sp, ntci::Strand::unknown(), self, - defer, - &d_mutex); + true, + &d_receiveMutex); } if (connectCallback) { @@ -1054,8 +1148,8 @@ void StreamSocket::privateFailConnectComplete( connectEvent, d_proactorStrand_sp, self, - defer, - &d_mutex); + true, + &d_receiveMutex); } if (d_connectOptions.retryCount().valueOr(bsl::size_t(0)) == 0) { @@ -1075,7 +1169,7 @@ void StreamSocket::privateFailConnectComplete( d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -1119,7 +1213,7 @@ void StreamSocket::privateFailUpgrade( this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1129,14 +1223,14 @@ void StreamSocket::privateFailUpgrade( upgradeEvent, d_proactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } this->privateShutdown(self, ntsa::ShutdownType::e_BOTH, ntsa::ShutdownMode::e_IMMEDIATE, - false); + true); } void StreamSocket::privateInitiateReceive( @@ -1158,7 +1252,12 @@ void StreamSocket::privateInitiateReceive( ntcs::ObserverRef proactorRef(&d_proactor); if (!proactorRef) { - this->privateFailReceive(self, ntsa::Error(ntsa::Error::e_INVALID)); + // Failing the receive is cross-cutting; this function may run holding + // only the receive mutex, so defer the failure to run under both + // mutexes in canonical order. + this->execute(NTCCFG_BIND(&StreamSocket::processSocketError, + self, + ntsa::Error(ntsa::Error::e_INVALID))); return; } @@ -1179,51 +1278,36 @@ void StreamSocket::privateInitiateReceive( error = proactorRef->receive(self, d_receiveBlob_sp.get(), d_receiveOptions); if (error) { - this->privateFailReceive(self, error); + // Failing the receive is cross-cutting; defer it to run under both + // mutexes in canonical order. + this->execute( + NTCCFG_BIND(&StreamSocket::processSocketError, self, error)); return; } d_receivePending = true; } -void StreamSocket::privateCompleteReceive( +ntsa::Error StreamSocket::privateCompleteReceive( const bsl::shared_ptr& self, bsl::size_t numBytesReceivable, - bsl::size_t numBytesReceived) + bsl::size_t numBytesReceived, + bool exclusive) { NTCI_LOG_CONTEXT(); ntsa::Error error; - if (numBytesReceived == 0) { - // Execute the shutdown of the receive buffer asynchronously to allow - // pending writes to first complete, otherwise pending writes which - // have had their data copied to the socket send buffer but not - // announced to be complete by the proactor will be mistakenly - // considered by be discarded. - // - // However, there is no current API to call something like - // 'privateShutdownReceive' indicating the peer has initiated the - // shutdown while not expected that function to be holding the lock. - // - // TODO: Resolve this issue. - // - // this->privateApplyFlowControl(self, - // ntca::FlowControlType::e_RECEIVE, - // ntca::FlowControlMode::e_IMMEDIATE, - // true); - // - // this->execute(NTCCFG_BIND( - // &StreamSocket::privateShutdownReceive, - // self, - // self, - // ntsa::ShutdownOrigin::e_REMOTE, - // false)); + // When the caller holds more than one mutex, all announcements and + // callback invocations must be deferred. - this->privateShutdownReceive(self, - ntsa::ShutdownOrigin::e_REMOTE, - false); - return; + const bool defer = exclusive; + + if (numBytesReceived == 0) { + // The peer has shut down. Return EOF so the caller + // ('processSocketReceived') can perform the cross-cutting shutdown of + // reception after releasing the receive mutex. + return ntsa::Error(ntsa::Error::e_EOF); } d_receiveFeedback.setFeedback(numBytesReceivable, numBytesReceived); @@ -1257,8 +1341,7 @@ void StreamSocket::privateCompleteReceive( *d_receiveBlob_sp, inflateOptions); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } bdlbb::BlobUtil::erase(d_receiveBlob_sp.get(), @@ -1272,10 +1355,16 @@ void StreamSocket::privateCompleteReceive( else { const int receiveQueueLengthBefore = d_receiveQueue.data()->length(); - error = d_encryption_sp->pushIncomingCipherText(*d_receiveBlob_sp); + // Pushing incoming cipher text may invoke the handshake callback, which + // may reset 'd_encryption_sp' if the handshake fails. Retain a + // reference to the encryption session so it is not destroyed while + // one of its member functions is still executing. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->pushIncomingCipherText(*d_receiveBlob_sp); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } bdlbb::BlobUtil::erase(d_receiveBlob_sp.get(), @@ -1283,7 +1372,7 @@ void StreamSocket::privateCompleteReceive( d_receiveBlob_sp->length()); if (!d_encryption_sp) { - return; + return ntsa::Error(); } if (!d_receiveInflater_sp) { @@ -1291,8 +1380,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popIncomingPlainText( d_receiveQueue.data().get()); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } } @@ -1306,7 +1394,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popIncomingLeftovers( d_receiveQueue.data().get()); if (NTCCFG_UNLIKELY(error)) { - return; + return ntsa::Error(); } } } @@ -1316,8 +1404,7 @@ void StreamSocket::privateCompleteReceive( while (NTCCFG_LIKELY(d_encryption_sp->hasIncomingPlainText())) { error = d_encryption_sp->popIncomingPlainText(&plainText); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } } @@ -1330,7 +1417,7 @@ void StreamSocket::privateCompleteReceive( error = d_encryption_sp->popIncomingLeftovers(&plainText); if (NTCCFG_UNLIKELY(error)) { - return; + return ntsa::Error(); } } } @@ -1345,8 +1432,7 @@ void StreamSocket::privateCompleteReceive( plainText, inflateOptions); if (NTCCFG_UNLIKELY(error)) { - this->privateFailReceive(self, error); - return; + return error; } bdlbb::BlobUtil::append(d_receiveQueue.data().get(), inflatedData); @@ -1365,6 +1451,54 @@ void StreamSocket::privateCompleteReceive( d_receiveQueue.pushEntry(entry); } + // Complete any downgrade, and enqueue any outgoing cipher text, before + // announcing the data received: announcing the data may invoke receive + // callbacks synchronously, which releases the receive mutex while they + // execute, and no further data must be decrypted after the peer's TLS + // shutdown has been received. + + if (NTCCFG_UNLIKELY(d_encryption_sp)) { + // Receiving the peer's TLS shutdown, or unencrypted leftovers after + // the encrypted data, downgrades the connection, which resets the + // encryption session: a cross-cutting transition that requires both + // mutexes. The lock order permits acquiring 'd_sendMutex' while + // holding 'd_receiveMutex'. + + if (NTCCFG_UNLIKELY(downgradeAbortively || + d_encryption_sp->isShutdownReceived())) + { + if (exclusive) { + error = this->privateReceiveDowngrade(self, + downgradeAbortively); + } + else { + LockGuard sendLock(&d_sendMutex); + error = this->privateReceiveDowngrade(self, + downgradeAbortively); + } + + if (error) { + return error; + } + } + else if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + // Enqueue any outgoing cipher text generated while decrypting, + // e.g., the response to a TLS 1.3 key update. + + if (exclusive) { + error = this->privateSendOutgoingCipherText(self); + } + else { + LockGuard sendLock(&d_sendMutex); + error = this->privateSendOutgoingCipherText(self); + } + + if (error) { + return error; + } + } + } + NTCP_STREAMSOCKET_LOG_READ_QUEUE_FILLED(d_receiveQueue.size()); NTCS_METRICS_UPDATE_READ_QUEUE_SIZE(d_receiveQueue.size()); @@ -1448,8 +1582,8 @@ void StreamSocket::privateCompleteReceive( receiveEvent, d_proactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } if (d_receiveQueue.authorizeLowWatermarkEvent()) { @@ -1468,8 +1602,8 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -1481,7 +1615,7 @@ void StreamSocket::privateCompleteReceive( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + defer, false); if (d_session_sp) { @@ -1495,14 +1629,72 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } - if (NTCCFG_UNLIKELY(d_encryption_sp)) { - if (NTCCFG_UNLIKELY(downgradeAbortively)) { + return ntsa::Error(); +} + +ntsa::Error StreamSocket::privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive) +{ + ntsa::Error error; + + if (!d_encryption_sp) { + return ntsa::Error(); + } + + if (NTCCFG_UNLIKELY(abortive)) { + if (!d_encryption_sp->isShutdownSent()) { + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_INITIATED); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeInitiated( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_proactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + + d_encryption_sp.reset(); + + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_COMPLETE); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeComplete(d_session_sp, + self, + event, + d_sessionStrand_sp, + d_proactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + else { + if (d_encryption_sp->isShutdownReceived()) { if (!d_encryption_sp->isShutdownSent()) { + error = d_encryption_sp->shutdown(); + if (error) { + return error; + } + if (d_session_sp) { ntca::DowngradeContext context; @@ -1517,11 +1709,18 @@ void StreamSocket::privateCompleteReceive( d_sessionStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } } + } + error = this->privateSendOutgoingCipherText(self); + if (error) { + return error; + } + + if (d_encryption_sp->isShutdownFinished()) { d_encryption_sp.reset(); if (d_session_sp) { @@ -1531,95 +1730,48 @@ void StreamSocket::privateCompleteReceive( event.setType(ntca::DowngradeEventType::e_COMPLETE); event.setContext(context); - ntcs::Dispatch::announceDowngradeComplete(d_session_sp, - self, - event, - d_sessionStrand_sp, - d_proactorStrand_sp, - self, - false, - &d_mutex); + ntcs::Dispatch::announceDowngradeComplete( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_proactorStrand_sp, + self, + true, + &d_receiveMutex); } } - else { - if (d_encryption_sp->isShutdownReceived()) { - if (!d_encryption_sp->isShutdownSent()) { - error = d_encryption_sp->shutdown(); - if (error) { - this->privateFailReceive(self, error); - return; - } - - if (d_session_sp) { - ntca::DowngradeContext context; - - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_INITIATED); - event.setContext(context); - - ntcs::Dispatch::announceDowngradeInitiated( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_proactorStrand_sp, - self, - false, - &d_mutex); - } - } - } - - if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { - bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); - - while ( - NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) - { - error = - d_encryption_sp->popOutgoingCipherText(&cipherData); - if (error) { - this->privateFailReceive(self, error); - return; - } - } + } - if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { - error = this->privateSendRaw(self, - cipherData, - ntca::SendOptions(), - ntca::SendContext(), - d_sendComplete); - if (error) { - this->privateFailReceive(self, error); - return; - } - } - } + return ntsa::Error(); +} - if (d_encryption_sp->isShutdownFinished()) { - d_encryption_sp.reset(); +ntsa::Error StreamSocket::privateSendOutgoingCipherText( + const bsl::shared_ptr& self) +{ + ntsa::Error error; - if (d_session_sp) { - ntca::DowngradeContext context; + bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_COMPLETE); - event.setContext(context); + while (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + error = d_encryption_sp->popOutgoingCipherText(&cipherData); + if (error) { + return error; + } + } - ntcs::Dispatch::announceDowngradeComplete( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_proactorStrand_sp, - self, - false, - &d_mutex); - } - } + if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { + error = this->privateSendRaw(self, + cipherData, + ntca::SendOptions(), + ntca::SendContext(), + d_sendComplete); + if (error) { + return error; } } + + return ntsa::Error(); } void StreamSocket::privateFailReceive( @@ -1710,8 +1862,16 @@ void StreamSocket::privateInitiateSend( break; } else { + // A graceful shutdown sentinel has been drained. Shutting down for + // sending is cross-cutting; this function runs holding only the + // send mutex, so defer the shutdown to run under both mutexes + // in canonical order (via the public 'shutdown', which is a no-op + // sentinel push once the write queue is empty). d_sendQueue.popEntry(); - this->privateShutdownSend(self, false); + this->execute(NTCCFG_BIND(&StreamSocket::shutdown, + self, + ntsa::ShutdownType::e_SEND, + ntsa::ShutdownMode::e_GRACEFUL)); } } } @@ -1760,7 +1920,7 @@ void StreamSocket::privateCompleteSend( d_proactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } if (d_sendQueue.authorizeLowWatermarkEvent()) { @@ -1780,7 +1940,7 @@ void StreamSocket::privateCompleteSend( d_proactorStrand_sp, self, true, - &d_mutex); + &d_sendMutex); } } @@ -1817,7 +1977,7 @@ void StreamSocket::privateFailSend(const bsl::shared_ptr& self, d_proactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } @@ -1837,7 +1997,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1879,8 +2039,8 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, d_sessionStrand_sp, d_proactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } this->privateShutdown(self, @@ -1945,7 +2105,7 @@ ntsa::Error StreamSocket::privateShutdown( d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2106,7 +2266,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2177,7 +2337,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2197,7 +2357,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2217,7 +2377,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2233,7 +2393,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2272,7 +2432,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } callbackEntryVector.clear(); @@ -2292,7 +2452,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } if (d_session_sp) { @@ -2307,7 +2467,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2324,7 +2484,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } // Note that detachment from the proactor is handled earlier in this @@ -2367,13 +2527,13 @@ void StreamSocket::privateShutdownSequenceComplete( d_proactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); if (d_closeCallback) { d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2433,7 +2593,7 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } this->privateInitiateSend(self); @@ -2458,7 +2618,7 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } this->privateInitiateReceive(self); @@ -2513,7 +2673,7 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } // Handled at the completion of the last send. @@ -2536,7 +2696,7 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } // Handled at the completion of the last receive. @@ -2575,7 +2735,7 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } @@ -2596,7 +2756,7 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2637,7 +2797,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_SEND, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canSend()) { @@ -2676,7 +2836,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -2705,7 +2865,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canReceive()) { @@ -2745,7 +2905,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -2931,7 +3091,7 @@ ntsa::Error StreamSocket::privateSendEncrypted( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -2979,7 +3139,7 @@ ntsa::Error StreamSocket::privateSendEncrypted( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -3274,7 +3434,7 @@ ntsa::Error StreamSocket::privateOpen( d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -3291,7 +3451,7 @@ void StreamSocket::processSourceEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; ntca::BindContext bindContext; @@ -3351,7 +3511,7 @@ void StreamSocket::processSourceEndpointResolution( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -3367,7 +3527,8 @@ void StreamSocket::processRemoteEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -3469,8 +3630,8 @@ void StreamSocket::processRemoteEndpointResolution( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, - &d_mutex); + true, + &d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) @@ -3510,12 +3671,23 @@ ntsa::Error StreamSocket::privateUpgrade( bdlf::MemFnUtil::memFn(&StreamSocket::privateEncryptionHandshake, this); - error = - d_encryption_sp->initiateHandshake(upgradeOptions, handshakeCallback); + // Initiating the handshake and pushing incoming cipher text may invoke + // the handshake callback, which resets 'd_encryption_sp' if the handshake + // fails. Retain a reference to the encryption session so it is not + // destroyed while one of its member functions is still executing, and + // stop if the failure of the upgrade has already been processed. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->initiateHandshake(upgradeOptions, handshakeCallback); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + // Push any unconsumed receive queue data that must be now considered // cipher text into the encryption session. @@ -3523,12 +3695,15 @@ ntsa::Error StreamSocket::privateUpgrade( // Push the receive queue data into the encryption session as incoming // cipher text. - error = - d_encryption_sp->pushIncomingCipherText(*d_receiveQueue.data()); + error = encryption->pushIncomingCipherText(*d_receiveQueue.data()); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + bdlbb::BlobUtil::erase(d_receiveQueue.data().get(), 0, d_receiveQueue.data()->length()); @@ -3642,7 +3817,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -3683,7 +3858,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -3743,7 +3918,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } d_encryption_sp.reset(); @@ -3762,7 +3937,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -3973,8 +4148,8 @@ ntsa::Error StreamSocket::privateRetryConnectToEndpoint( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, - &d_mutex); + true, + &d_receiveMutex); } return ntsa::Error(); @@ -4025,7 +4200,8 @@ StreamSocket::StreamSocket( const bsl::shared_ptr& metrics, bslma::Allocator* basicAllocator) : d_object("ntcp::StreamSocket") -, d_mutex() +, d_sendMutex() +, d_receiveMutex() , d_transport(ntsa::Transport::e_UNDEFINED) , d_systemHandle(ntsa::k_INVALID_HANDLE) , d_systemSourceEndpoint() @@ -4195,7 +4371,7 @@ ntsa::Error StreamSocket::open() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self); } @@ -4204,7 +4380,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport); } @@ -4214,7 +4390,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle); } @@ -4225,7 +4401,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket); } @@ -4237,7 +4413,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle, acceptor); } @@ -4249,7 +4425,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket, acceptor); } @@ -4269,7 +4445,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4309,7 +4485,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4338,7 +4514,7 @@ ntsa::Error StreamSocket::bind(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4385,7 +4561,7 @@ ntsa::Error StreamSocket::connect(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4531,7 +4707,7 @@ ntsa::Error StreamSocket::connect(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -4656,7 +4832,8 @@ ntsa::Error StreamSocket::upgrade( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -4814,7 +4991,16 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Both unencrypted and encrypted sends need only the send mutex. The + // encryption object is co-guarded, so it is stable while the send mutex is + // held, and it is itself thread safe. Every path that pops outgoing + // cipher text and enqueues it to the send queue holds the send mutex, so + // TLS records are enqueued in the order they are generated. Pushing + // outgoing plain text never invokes the handshake callback (see + // 'ntci::Encryption'), so the encrypted send path never performs a + // cross-cutting transition. + + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -4854,7 +5040,7 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -4934,7 +5120,10 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Both unencrypted and encrypted sends need only the send mutex. See the + // 'bdlbb::Blob' overload of this function. + + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -4974,7 +5163,7 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -5045,7 +5234,7 @@ ntsa::Error StreamSocket::receive(ntca::ReceiveContext* context, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5167,7 +5356,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5269,7 +5458,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); bool receiveQueueHighWatermarkViolatedAfter = d_receiveQueue.isHighWatermarkViolated(); @@ -5331,14 +5520,14 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntsa::Error StreamSocket::registerResolver( const bsl::shared_ptr& resolver) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver = resolver; return ntsa::Error(); } ntsa::Error StreamSocket::deregisterResolver() { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver.reset(); return ntsa::Error(); } @@ -5348,7 +5537,8 @@ ntsa::Error StreamSocket::registerManager( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (manager) { d_manager_sp = manager; @@ -5368,7 +5558,8 @@ ntsa::Error StreamSocket::registerManager( ntsa::Error StreamSocket::deregisterManager() { - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_manager_sp.reset(); d_managerStrand_sp.reset(); @@ -5381,7 +5572,8 @@ ntsa::Error StreamSocket::registerSession( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (session) { d_session_sp = session; @@ -5411,7 +5603,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -5448,7 +5641,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -5478,7 +5672,8 @@ ntsa::Error StreamSocket::registerSessionCallback( ntsa::Error StreamSocket::deregisterSession() { - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_session_sp.reset(); d_sessionStrand_sp.reset(); @@ -5497,7 +5692,7 @@ ntsa::Error StreamSocket::setConnectRateLimiter( ntsa::Error StreamSocket::setWriteDeflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); d_sendDeflater_sp = compression; @@ -5509,7 +5704,7 @@ ntsa::Error StreamSocket::setWriteRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5538,7 +5733,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5566,7 +5761,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5577,7 +5772,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5605,7 +5800,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5617,7 +5812,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5646,7 +5841,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5668,7 +5863,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5678,7 +5873,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntsa::Error StreamSocket::setReadInflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_receiveInflater_sp = compression; @@ -5690,7 +5885,7 @@ ntsa::Error StreamSocket::setReadRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5719,7 +5914,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5749,7 +5944,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -5760,7 +5955,7 @@ ntsa::Error StreamSocket::setReadQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5786,7 +5981,7 @@ ntsa::Error StreamSocket::setReadQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -5820,7 +6015,8 @@ ntsa::Error StreamSocket::relaxFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5837,7 +6033,8 @@ ntsa::Error StreamSocket::applyFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5879,7 +6076,8 @@ ntsa::Error StreamSocket::cancel(const ntca::ConnectToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5905,7 +6103,8 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5939,7 +6138,7 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } this->privateFail(self, ntsa::Error(ntsa::Error::e_CANCELLED)); @@ -5954,7 +6153,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -5988,7 +6187,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -6000,7 +6199,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6029,7 +6228,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) d_proactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -6041,7 +6240,8 @@ ntsa::Error StreamSocket::downgrade() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); return this->privateDowngrade(self, ntca::DowngradeOptions()); } @@ -6050,7 +6250,8 @@ ntsa::Error StreamSocket::downgrade(const ntca::DowngradeOptions& options) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (options.abortive().isNull() || !options.abortive().value()) { return this->privateDowngrade(self, options); @@ -6065,7 +6266,8 @@ ntsa::Error StreamSocket::shutdown(ntsa::ShutdownType::Value direction, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6114,7 +6316,8 @@ ntsa::Error StreamSocket::release(ntsa::Handle* result, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); *result = ntsa::k_INVALID_HANDLE; @@ -6150,7 +6353,8 @@ void StreamSocket::close(const ntci::CloseCallback& callback) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); this->privateClose(self, callback); } @@ -6271,20 +6475,20 @@ ntsa::Transport::Value StreamSocket::transport() const ntsa::Endpoint StreamSocket::sourceEndpoint() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_systemSourceEndpoint; } ntsa::Endpoint StreamSocket::remoteEndpoint() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_systemRemoteEndpoint; } bsl::shared_ptr StreamSocket::sourceCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -6297,7 +6501,7 @@ bsl::shared_ptr StreamSocket::sourceCertificate() bsl::shared_ptr StreamSocket::remoteCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -6309,7 +6513,7 @@ bsl::shared_ptr StreamSocket::remoteCertificate() bsl::shared_ptr StreamSocket::privateKey() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -6353,37 +6557,37 @@ bsl::size_t StreamSocket::threadIndex() const bsl::size_t StreamSocket::readQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.size(); } bsl::size_t StreamSocket::readQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.lowWatermark(); } bsl::size_t StreamSocket::readQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.highWatermark(); } bsl::size_t StreamSocket::writeQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.size(); } bsl::size_t StreamSocket::writeQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.lowWatermark(); } bsl::size_t StreamSocket::writeQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.highWatermark(); } @@ -6439,23 +6643,28 @@ void StreamSocket::getInfo(ntsa::SocketInfo* result) const d_publicRemoteEndpoint.load(&remoteEndpoint); { - LockGuard lock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + sendQueueSize = d_sendQueue.size(); + } - sendQueueSize = d_sendQueue.size(); + { + LockGuard receiveLock(&d_receiveMutex); receiveQueueSize = d_receiveQueue.size(); + } - if (d_shutdownState.completed()) { - socketState = ntsa::SocketState::e_CLOSED; - } - else if (d_shutdownState.initiated()) { - socketState = ntsa::SocketState::e_CLOSING; - } - else if (d_connectInProgress) { - socketState = ntsa::SocketState::e_SYN_SENT; - } - else { - socketState = ntsa::SocketState::e_ESTABLISHED; - } + // The shutdown and connect predicates are atomic and read without a lock. + + if (d_shutdownState.completed()) { + socketState = ntsa::SocketState::e_CLOSED; + } + else if (d_shutdownState.initiated()) { + socketState = ntsa::SocketState::e_CLOSING; + } + else if (d_connectInProgress) { + socketState = ntsa::SocketState::e_SYN_SENT; + } + else { + socketState = ntsa::SocketState::e_ESTABLISHED; } ntcs::ObserverRef proactorRef(&d_proactor); diff --git a/groups/ntc/ntcp/ntcp_streamsocket.h b/groups/ntc/ntcp/ntcp_streamsocket.h index b585f6d1..08b88cbe 100644 --- a/groups/ntc/ntcp/ntcp_streamsocket.h +++ b/groups/ntc/ntcp/ntcp_streamsocket.h @@ -88,7 +88,51 @@ class StreamSocket : public ntci::StreamSocket, typedef ntccfg::LockGuard LockGuard; ntccfg::Object d_object; - mutable Mutex d_mutex; + // LOCKING STRATEGY + // + // 'd_receiveMutex' guards the read path (the receive queue and incoming + // completion state) and the shared lifecycle and control state of the + // socket: the connect/upgrade machinery and the open, shutdown, and detach + // state machines' *transitions*. Conceptually, it guards the + // connectedness and potential readability of the socket. 'd_sendMutex' + // guards the separate write path (the send queue and outgoing completion + // state), so that sending is not serialized against receiving. + // + // The encryption object is itself thread safe. Encrypting outgoing data + // requires only 'd_sendMutex': every path that pops outgoing cipher text + // and enqueues it to the send queue holds 'd_sendMutex', so TLS records + // are enqueued in the order they are generated, and pushing outgoing plain + // text never invokes the handshake callback. + // + // Decrypting incoming data in the steady state (i.e., once the handshake + // has completed) requires only 'd_receiveMutex': the handshake callback + // can no longer be invoked. Decrypting incoming data while the handshake + // is in progress holds both mutexes, since it may complete or fail the + // handshake (a cross-cutting transition). + // + // When both are held they are always acquired in the order + // 'd_receiveMutex' -> 'd_sendMutex'. This order follows the flow of data: + // the read path may feed the write path, so it may acquire 'd_sendMutex' + // while holding 'd_receiveMutex', but the write path never acquires + // 'd_receiveMutex' while holding 'd_sendMutex'. Consequently, the receive + // completion performs every cross-cutting transition it discovers + // (enqueueing outgoing cipher text generated while decrypting, receiving + // the peer's TLS shutdown, end-of-file, or failure) in place, by + // additionally acquiring 'd_sendMutex' without releasing + // 'd_receiveMutex'. The write path holds only 'd_sendMutex'; when it must + // perform a cross-cutting lifecycle transition (connection establishment, + // failure, shutdown) it first releases 'd_sendMutex', then acquires both + // in canonical order (or defers the transition via 'execute'). + // + // The observable lifecycle predicates ('d_openState', 'd_shutdownState', + // 'd_detachState.mode()', 'd_connectInProgress') are atomic and may be read + // without any lock. 'd_flowControlState' is itself thread safe (lock-free). + // The co-guarded members ('d_socket_sp', 'd_encryption_sp', 'd_session_sp', + // 'd_sessionStrand_sp', 'd_manager_sp', 'd_managerStrand_sp') are written + // only while holding both mutexes and may therefore be read while holding + // either one of them. + mutable Mutex d_sendMutex; + mutable Mutex d_receiveMutex; ntsa::Transport::Value d_transport; ntsa::Handle d_systemHandle; ntsa::Endpoint d_systemSourceEndpoint; @@ -144,7 +188,7 @@ class StreamSocket : public ntci::StreamSocket, bsl::shared_ptr d_connectRetryTimer_sp; bsl::shared_ptr d_connectRateLimiter_sp; bsl::shared_ptr d_connectRateTimer_sp; - bool d_connectInProgress; + bsls::AtomicBool d_connectInProgress; ntca::UpgradeOptions d_upgradeOptions; ntci::UpgradeCallback d_upgradeCallback; bsl::shared_ptr d_upgradeTimer_sp; @@ -232,7 +276,7 @@ class StreamSocket : public ntci::StreamSocket, const bsl::string& details); /// Process the completion of the connection attempt. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless both 'd_receiveMutex' and 'd_sendMutex' are locked. void privateCompleteConnect(const bsl::shared_ptr& self); /// Indicate a connection failure has occurred. If the specified 'defer' @@ -259,7 +303,7 @@ class StreamSocket : public ntci::StreamSocket, bool defer); /// Process the failure of the upgrade attempt. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless both 'd_receiveMutex' and 'd_sendMutex' are locked. void privateFailUpgrade(const bsl::shared_ptr& self, const ntsa::Error& error, const bsl::string& errorDescription); @@ -267,22 +311,47 @@ class StreamSocket : public ntci::StreamSocket, /// Initiate a new reception, if allowed and necessary. void privateInitiateReceive(const bsl::shared_ptr& self); - /// Process the completion of the reception of raw or encrypted - /// data according to the specified 'numBytesReceivable' and - /// 'numBytesReceived'. The behavior is undefined unless 'd_mutex' is - /// locked. - void privateCompleteReceive(const bsl::shared_ptr& self, - bsl::size_t numBytesReceivable, - bsl::size_t numBytesReceived); + /// Process the completion of the reception of raw or encrypted data + /// according to the specified 'numBytesReceivable' and 'numBytesReceived'. + /// If the specified 'exclusive' flag is true, the caller holds both + /// 'd_receiveMutex' and 'd_sendMutex', and all event announcements and + /// callback invocations made during processing are deferred. Otherwise, + /// the caller holds only 'd_receiveMutex', 'd_sendMutex' is additionally + /// acquired as necessary, and the behavior is undefined unless the + /// encryption handshake, if any, has completed. Return + /// 'ntsa::Error::e_EOF' if the peer has shut down, another error if the + /// reception failed, or a default-constructed error on success; the + /// caller performs any resulting shutdown or failure sequence. + ntsa::Error privateCompleteReceive( + const bsl::shared_ptr& self, + bsl::size_t numBytesReceivable, + bsl::size_t numBytesReceived, + bool exclusive); + + /// Complete the downgrade from encrypted to unencrypted communication + /// initiated by the receipt of the peer's TLS shutdown, or, if the + /// specified 'abortive' flag is true, by the receipt of unencrypted + /// leftovers after the encrypted data. Enqueue any outgoing cipher text + /// generated as a result. The behavior is undefined unless both + /// 'd_receiveMutex' and 'd_sendMutex' are locked. Return the error. + ntsa::Error privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive); + + /// Pop all outgoing cipher text from the encryption session and enqueue + /// it for transmission. The behavior is undefined unless 'd_sendMutex' is + /// locked. Return the error. + ntsa::Error privateSendOutgoingCipherText( + const bsl::shared_ptr& self); /// Process the completion of the reception of the raw or unencrypted /// data according to the specified 'numBytesReceived'. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless 'd_receiveMutex' is locked. void privateCompleteReceiveRaw(const bsl::shared_ptr& self, bsl::size_t numBytesReceived); /// Process the failure of the reception of a message. The behavior is - /// undefined unless 'd_mutex' is locked. + /// undefined unless both 'd_receiveMutex' and 'd_sendMutex' are locked. void privateFailReceive(const bsl::shared_ptr& self, const ntsa::Error& error); @@ -293,7 +362,7 @@ class StreamSocket : public ntci::StreamSocket, /// Process the completion of the transmission of raw or /// already-encrypted data at the head of the write queue according to /// the specified 'numBytesSent'. The behavior is undefined unless - /// 'd_mutex' is locked. + /// 'd_sendMutex' is locked. void privateCompleteSend(const bsl::shared_ptr& self, bsl::size_t numBytesSent); @@ -301,7 +370,7 @@ class StreamSocket : public ntci::StreamSocket, /// of the write queue. Announce the failure of the head of the write /// queue if a callback is associated with the head of the write queue /// then pop the head of the write queue. The behavior is undefined - /// unless 'd_mutex' is locked. + /// unless 'd_sendMutex' is locked. void privateFailSend(const bsl::shared_ptr& self, const ntsa::Error& error); @@ -353,7 +422,8 @@ class StreamSocket : public ntci::StreamSocket, bool defer); /// Enable copying from the socket buffers in the specified 'direction'. - /// The behavior is undefined unless 'd_mutex' is locked. + /// The behavior is undefined unless the mutex guarding the 'direction' + /// (or both mutexes, for 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateRelaxFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -362,7 +432,8 @@ class StreamSocket : public ntci::StreamSocket, /// Disable copying from socket buffers in the specified 'direction' /// according to the specified 'mode'. The behavior is undefined unless - /// 'd_mutex' is locked. + /// the mutex guarding the 'direction' (or both mutexes, for + /// 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateApplyFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, diff --git a/groups/ntc/ntcr/ntcr_streamsocket.cpp b/groups/ntc/ntcr/ntcr_streamsocket.cpp index 8e8f9b79..3ee7b55a 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.cpp +++ b/groups/ntc/ntcr/ntcr_streamsocket.cpp @@ -288,14 +288,14 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); - NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + // These lifecycle predicates are atomic and may be read without any lock. + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { return; @@ -306,12 +306,78 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) } ntsa::Error error; + + // The read path holds the receive mutex throughout. Whenever it must + // perform a cross-cutting transition (completing or failing the + // handshake, downgrading, shutting down, or failing), it additionally + // acquires the send mutex, which the lock order permits, without releasing + // the receive mutex. + + LockGuard receiveLock(&d_receiveMutex); + + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) + { + return; + } + + if (!d_shutdownState.canReceive()) { + return; + } + + if (NTCCFG_UNLIKELY(d_encryption_sp && d_upgradeInProgress)) { + // While the handshake is in progress, decrypting may complete or fail + // the handshake, so hold both mutexes for the entire read. + + LockGuard sendLock(&d_sendMutex); + + bsl::size_t numIterations = 0; + + while (true) { + ++numIterations; + + error = this->privateSocketReadableIteration(self, true); + if (error) { + break; + } + + if (!d_receiveGreedily) { + break; + } + + if (!d_shutdownState.canReceive()) { + break; + } + } + + if (numIterations > 0) { + NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); + } + + if (error == ntsa::Error::e_EOF) { + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); + this->privateFail(self, error); + } + else if (error && error != ntsa::Error::e_WOULD_BLOCK) { + this->privateFail(self, error); + } + else { + this->privateRearmAfterReceive(self); + } + + return; + } + + // Fast path: an unencrypted read, or an encrypted read once the handshake + // has completed, only needs the receive mutex. + bsl::size_t numIterations = 0; while (true) { ++numIterations; - error = this->privateSocketReadableIteration(self); + error = this->privateSocketReadableIteration(self, false); if (error) { break; } @@ -329,7 +395,17 @@ void StreamSocket::processSocketReadable(const ntca::ReactorEvent& event) NTCS_METRICS_UPDATE_RECEIVE_ITERATIONS(numIterations); } - if (error && error != ntsa::Error::e_WOULD_BLOCK) { + if (error == ntsa::Error::e_EOF) { + LockGuard sendLock(&d_sendMutex); + + this->privateShutdownReceive(self, + ntsa::ShutdownOrigin::e_REMOTE, + true); + this->privateFail(self, error); + } + else if (error && error != ntsa::Error::e_WOULD_BLOCK) { + LockGuard sendLock(&d_sendMutex); + this->privateFail(self, error); } else { @@ -345,21 +421,37 @@ void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); - NTCI_LOG_CONTEXT(); NTCI_LOG_CONTEXT_GUARD_DESCRIPTOR(d_publicHandle); NTCI_LOG_CONTEXT_GUARD_SOURCE_ENDPOINT(d_systemSourceEndpoint); NTCI_LOG_CONTEXT_GUARD_REMOTE_ENDPOINT(d_systemRemoteEndpoint); + // These lifecycle predicates are atomic and may be read without any lock. + if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { return; } if (NTCCFG_UNLIKELY(d_connectInProgress)) { - this->privateSocketWritableConnection(self); + // Connection establishment is a cross-cutting transition (it sets up + // both the send and receive paths and announces establishment); + // acquire both mutexes in canonical order. + + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); + + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; + } + + if (d_connectInProgress) { + this->privateSocketWritableConnection(self); + } + return; } @@ -368,34 +460,70 @@ void StreamSocket::processSocketWritable(const ntca::ReactorEvent& event) } ntsa::Error error; - bsl::size_t numIterations = 0; + bool shutdownSendPending = false; - while (d_sendQueue.hasEntry()) { - ++numIterations; + { + LockGuard sendLock(&d_sendMutex); - error = this->privateSocketWritableIteration(self); - if (error) { - break; + if (NTCCFG_UNLIKELY(d_detachState.mode() == + ntcs::DetachMode::e_INITIATED)) + { + return; } - if (!d_sendGreedily) { - break; + if (!d_shutdownState.canSend()) { + return; } - if (!d_shutdownState.canSend()) { - break; + bsl::size_t numIterations = 0; + + while (d_sendQueue.hasEntry()) { + ++numIterations; + + error = this->privateSocketWritableIteration(self, + &shutdownSendPending); + if (error) { + break; + } + + if (shutdownSendPending) { + break; + } + + if (!d_sendGreedily) { + break; + } + + if (!d_shutdownState.canSend()) { + break; + } } - } - if (numIterations > 0) { - NTCS_METRICS_UPDATE_SEND_ITERATIONS(numIterations); + if (numIterations > 0) { + NTCS_METRICS_UPDATE_SEND_ITERATIONS(numIterations); + } + + if (!(error && error != ntsa::Error::e_WOULD_BLOCK) && + !shutdownSendPending) + { + this->privateRearmAfterSend(self); + } } + // Perform any cross-cutting failure or shutdown sequence outside the send + // mutex, acquiring both mutexes in canonical order. + if (error && error != ntsa::Error::e_WOULD_BLOCK) { + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); + this->privateFail(self, error); } - else { - this->privateRearmAfterSend(self); + else if (shutdownSendPending) { + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); + + this->privateShutdownSend(self, true); } } @@ -405,7 +533,11 @@ void StreamSocket::processSocketError(const ntca::ReactorEvent& event) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Error handling is cross-cutting (it may fail a connect, an upgrade, or + // the established connection); acquire both mutexes in canonical + // order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -436,7 +568,9 @@ void StreamSocket::processNotifications( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Notifications carry zero-copy completions and transmit timestamps, both + // of which belong to the send path. + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -475,7 +609,10 @@ void StreamSocket::processConnectDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Connect failure is cross-cutting; acquire both mutexes in canonical + // order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -517,7 +654,10 @@ void StreamSocket::processConnectRetryTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Connect retry/failure is cross-cutting; acquire both mutexes in + // canonical order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -555,7 +695,10 @@ void StreamSocket::processUpgradeTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Upgrade failure is cross-cutting; acquire both mutexes in canonical + // order. + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -582,7 +725,7 @@ void StreamSocket::processSendRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -611,7 +754,7 @@ void StreamSocket::processSendRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_sendMutex); } } } @@ -627,7 +770,7 @@ void StreamSocket::processSendDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -661,7 +804,7 @@ void StreamSocket::processSendDeadlineTimer( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } } @@ -676,7 +819,7 @@ void StreamSocket::processReceiveRateTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -705,7 +848,7 @@ void StreamSocket::processReceiveRateTimer( ntci::Strand::unknown(), self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -721,7 +864,7 @@ void StreamSocket::processReceiveDeadlineTimer( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -749,7 +892,7 @@ void StreamSocket::processReceiveDeadlineTimer( d_reactorStrand_sp, self, false, - &d_mutex); + &d_receiveMutex); } } } @@ -759,8 +902,14 @@ void StreamSocket::privateEncryptionHandshake( const bsl::shared_ptr& certificate, const bsl::string& details) { - // IMPLEMENTATION NOTE: This function is always called under a lock - // during the exection of 'd_encryption->popIncomingCipherText()'. + // IMPLEMENTATION NOTE: This function is always called while holding + // 'd_receiveMutex' and 'd_sendMutex', so all callbacks are deferred. The + // encryption session invokes this function only from within + // 'initiateHandshake' or 'pushIncomingCipherText' (see 'ntci::Encryption'), + // which are called only from 'privateUpgrade' and the encrypted read path + // while the handshake is in progress, each of which holds both mutexes. + // In particular, this function is never called from the encrypted send + // path, which holds only 'd_sendMutex'. NTCI_LOG_CONTEXT(); @@ -790,8 +939,8 @@ void StreamSocket::privateEncryptionHandshake( event, d_reactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } } else { @@ -802,12 +951,18 @@ void StreamSocket::privateEncryptionHandshake( } ntsa::Error StreamSocket::privateSocketReadableIteration( - const bsl::shared_ptr& self) + const bsl::shared_ptr& self, + bool exclusive) { NTCI_LOG_CONTEXT(); ntsa::Error error; + // When the caller holds more than one mutex, all announcements and + // callback invocations must be deferred. + + const bool defer = exclusive; + if (d_receiveQueue.isHighWatermarkViolated()) { return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); } @@ -815,7 +970,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( ntsa::ReceiveContext context; error = this->privateDequeueReceiveBuffer(self, &context, - d_receiveQueue.data().get()); + d_receiveQueue.data().get(), + exclusive); if (NTCCFG_UNLIKELY(error)) { return error; } @@ -919,8 +1075,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( receiveEvent, d_reactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } if (d_receiveQueue.authorizeLowWatermarkEvent()) { @@ -939,8 +1095,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -952,7 +1108,7 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + defer, false); if (d_session_sp) { @@ -966,8 +1122,8 @@ ntsa::Error StreamSocket::privateSocketReadableIteration( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + defer, + &d_receiveMutex); } } @@ -1138,6 +1294,9 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( NTCI_LOG_TRACE("Connection attempt succeeded"); + // Connection establishment runs while holding both mutexes, so all + // announcements and callbacks are deferred. + if (d_session_sp) { ntcs::Dispatch::announceConnectComplete(d_session_sp, self, @@ -1145,8 +1304,8 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, - &d_mutex); + true, + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return ntsa::Error(ntsa::Error::e_CONNECTION_DEAD); @@ -1158,8 +1317,8 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( connectEvent, d_reactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); if (d_openState.value() != ntcs::OpenState::e_CONNECTED) { return ntsa::Error(ntsa::Error::e_CONNECTION_DEAD); @@ -1171,14 +1330,15 @@ ntsa::Error StreamSocket::privateSocketWritableConnection( d_managerStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); return ntsa::Error(); } ntsa::Error StreamSocket::privateSocketWritableIteration( - const bsl::shared_ptr& self) + const bsl::shared_ptr& self, + bool* shutdownSendPending) { if (!d_sendQueue.hasEntry()) { return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -1190,7 +1350,8 @@ ntsa::Error StreamSocket::privateSocketWritableIteration( return this->privateSocketWritableIterationBatch(self); } else { - return this->privateSocketWritableIterationFront(self); + return this->privateSocketWritableIterationFront(self, + shutdownSendPending); } } @@ -1313,7 +1474,7 @@ ntsa::Error StreamSocket::privateSocketWritableIterationBatch( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } @@ -1334,7 +1495,7 @@ ntsa::Error StreamSocket::privateSocketWritableIterationBatch( d_reactorStrand_sp, self, true, - &d_mutex); + &d_sendMutex); } } @@ -1350,7 +1511,8 @@ ntsa::Error StreamSocket::privateSocketWritableIterationBatch( } ntsa::Error StreamSocket::privateSocketWritableIterationFront( - const bsl::shared_ptr& self) + const bsl::shared_ptr& self, + bool* shutdownSendPending) { NTCI_LOG_CONTEXT(); @@ -1439,7 +1601,7 @@ ntsa::Error StreamSocket::privateSocketWritableIterationFront( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } if (d_sendQueue.authorizeLowWatermarkEvent()) { @@ -1460,13 +1622,17 @@ ntsa::Error StreamSocket::privateSocketWritableIterationFront( d_reactorStrand_sp, self, true, - &d_mutex); + &d_sendMutex); } } } else { + // A graceful shutdown sentinel has been drained. Defer the actual + // shutdown-for-sending to the caller ('processSocketWritable'), which + // will perform it after releasing the send mutex so the cross-cutting + // shutdown sequence can acquire the mutexes in canonical order. d_sendQueue.popEntry(); - this->privateShutdownSend(self, false); + *shutdownSendPending = true; } if (!d_sendQueue.hasEntry()) { @@ -1631,7 +1797,11 @@ void StreamSocket::privateFailConnectPart2( NTCI_LOG_CONTEXT(); if (lock) { - d_mutex.lock(); + // Invoked asynchronously as the reactor detachment callback. Acquire + // both mutexes in canonical order because connect failure cleanup + // touches the send and receive paths and the shared state. + d_receiveMutex.lock(); + d_sendMutex.lock(); BSLS_ASSERT(d_detachState.mode() == ntcs::DetachMode::e_INITIATED); d_detachState.setMode(ntcs::DetachMode::e_IDLE); } @@ -1664,6 +1834,11 @@ void StreamSocket::privateFailConnectPart2( } } + NTCCFG_WARNING_UNUSED(defer); + + // This cleanup runs while holding both mutexes, so all announcements + // and callbacks are forced to be deferred. + if (d_session_sp) { ntcs::Dispatch::announceConnectComplete(d_session_sp, self, @@ -1671,8 +1846,8 @@ void StreamSocket::privateFailConnectPart2( d_sessionStrand_sp, ntci::Strand::unknown(), self, - defer, - &d_mutex); + true, + &d_receiveMutex); } if (connectCallback) { @@ -1680,15 +1855,15 @@ void StreamSocket::privateFailConnectPart2( connectEvent, d_reactorStrand_sp, self, - defer, - &d_mutex); + true, + &d_receiveMutex); } if (d_closeCallback) { d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -1716,7 +1891,8 @@ void StreamSocket::privateFailConnectPart2( d_deferredCalls.clear(); if (lock) { - d_mutex.unlock(); + d_sendMutex.unlock(); + d_receiveMutex.unlock(); } } @@ -1754,7 +1930,7 @@ void StreamSocket::privateFailUpgrade( this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1764,14 +1940,14 @@ void StreamSocket::privateFailUpgrade( upgradeEvent, d_reactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } this->privateShutdown(self, ntsa::ShutdownType::e_BOTH, ntsa::ShutdownMode::e_IMMEDIATE, - false); + true); } void StreamSocket::privateFail(const bsl::shared_ptr& self, @@ -1790,7 +1966,7 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, this->privateApplyFlowControl(self, ntca::FlowControlType::e_BOTH, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); d_flowControlState.close(); @@ -1832,14 +2008,14 @@ void StreamSocket::privateFail(const bsl::shared_ptr& self, d_sessionStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } this->privateShutdown(self, ntsa::ShutdownType::e_BOTH, ntsa::ShutdownMode::e_IMMEDIATE, - false); + true); } } @@ -1898,7 +2074,7 @@ ntsa::Error StreamSocket::privateShutdown( d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2027,7 +2203,11 @@ void StreamSocket::privateShutdownSequenceComplete( NTCI_LOG_CONTEXT(); if (lock) { - d_mutex.lock(); + // Invoked asynchronously as the reactor detachment callback. Acquire + // both mutexes in canonical order because the shutdown completion + // touches both the send and receive paths as well as the shared state. + d_receiveMutex.lock(); + d_sendMutex.lock(); BSLS_ASSERT(d_detachState.mode() == ntcs::DetachMode::e_INITIATED); d_detachState.setMode(ntcs::DetachMode::e_IDLE); } @@ -2072,7 +2252,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2156,7 +2336,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2176,7 +2356,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2195,7 +2375,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2211,7 +2391,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2250,7 +2430,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } callbackEntryVector.clear(); @@ -2270,7 +2450,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } if (d_session_sp) { @@ -2285,7 +2465,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -2302,7 +2482,7 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); } // Note that detachment from the reactor is handled earlier in this @@ -2345,13 +2525,13 @@ void StreamSocket::privateShutdownSequenceComplete( d_reactorStrand_sp, self, defer, - &d_mutex); + &d_receiveMutex); if (d_closeCallback) { d_closeCallback.dispatch(ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); d_closeCallback.reset(); } @@ -2371,7 +2551,8 @@ void StreamSocket::privateShutdownSequenceComplete( d_deferredCalls.clear(); if (lock) { - d_mutex.unlock(); + d_sendMutex.unlock(); + d_receiveMutex.unlock(); } } @@ -2397,9 +2578,16 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( break; } - ntcs::FlowControlContext context; - if (d_flowControlState.relax(&context, direction, unlock)) { - if (relaxSend) { + // The send-direction flow control state is guarded by 'd_sendMutex' and the + // receive-direction flow control state by 'd_receiveMutex'; the caller must + // hold the corresponding leaf mutex for each direction being relaxed. + + if (relaxSend) { + ntcs::FlowControlContext context; + if (d_flowControlState.relax(&context, + ntca::FlowControlType::e_SEND, + unlock)) + { if (context.enableSend()) { if (d_shutdownState.canSend()) { ntcs::ObserverRef reactorRef(&d_reactor); @@ -2422,13 +2610,19 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } } + } - if (relaxReceive) { + if (relaxReceive) { + ntcs::FlowControlContext context; + if (d_flowControlState.relax(&context, + ntca::FlowControlType::e_RECEIVE, + unlock)) + { if (context.enableReceive()) { if (d_shutdownState.canReceive()) { ntcs::ObserverRef reactorRef(&d_reactor); @@ -2451,7 +2645,7 @@ ntsa::Error StreamSocket::privateRelaxFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2486,9 +2680,16 @@ ntsa::Error StreamSocket::privateApplyFlowControl( break; } - ntcs::FlowControlContext context; - if (d_flowControlState.apply(&context, direction, lock)) { - if (applySend) { + // The send-direction flow control state is guarded by 'd_sendMutex' and the + // receive-direction flow control state by 'd_receiveMutex'; the caller must + // hold the corresponding leaf mutex for each direction being applied. + + if (applySend) { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_SEND, + lock)) + { if (!context.enableSend()) { ntcs::ObserverRef reactorRef(&d_reactor); if (reactorRef) { @@ -2509,12 +2710,18 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } + } - if (applyReceive) { + if (applyReceive) { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_RECEIVE, + lock)) + { if (!context.enableReceive()) { ntcs::ObserverRef reactorRef(&d_reactor); if (reactorRef) { @@ -2535,7 +2742,7 @@ ntsa::Error StreamSocket::privateApplyFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2549,15 +2756,16 @@ bool StreamSocket::privateCloseFlowControl( bool defer, const ntci::SocketDetachedCallback& detachCallback) { - bool applySend = true; - bool applyReceive = true; + // This function is only invoked from the cross-cutting shutdown sequence, + // which holds 'd_receiveMutex' and 'd_sendMutex'. Both directions of flow + // control are therefore closed here. - ntcs::FlowControlContext context; - if (d_flowControlState.apply(&context, - ntca::FlowControlType::e_BOTH, - true)) { - if (applySend) { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_SEND, + true)) + { if (!context.enableSend()) { if (d_session_sp) { ntca::WriteQueueEvent event; @@ -2573,12 +2781,18 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } } } + } - if (applyReceive) { + { + ntcs::FlowControlContext context; + if (d_flowControlState.apply(&context, + ntca::FlowControlType::e_RECEIVE, + true)) + { if (!context.enableReceive()) { if (d_session_sp) { ntca::ReadQueueEvent event; @@ -2594,7 +2808,7 @@ bool StreamSocket::privateCloseFlowControl( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } } @@ -2635,7 +2849,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_SEND, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canSend()) { @@ -2674,7 +2888,7 @@ ntsa::Error StreamSocket::privateThrottleSendBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -2703,7 +2917,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( this->privateApplyFlowControl(self, ntca::FlowControlType::e_RECEIVE, ntca::FlowControlMode::e_IMMEDIATE, - false, + true, true); if (!d_shutdownState.canReceive()) { @@ -2743,7 +2957,7 @@ ntsa::Error StreamSocket::privateThrottleReceiveBuffer( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(ntsa::Error::e_WOULD_BLOCK); @@ -2980,7 +3194,8 @@ ntsa::Error StreamSocket::privateEnqueueSendBuffer( ntsa::Error StreamSocket::privateDequeueReceiveBuffer( const bsl::shared_ptr& self, ntsa::ReceiveContext* context, - bdlbb::Blob* data) + bdlbb::Blob* data, + bool exclusive) { ntsa::Error error; @@ -3107,7 +3322,14 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( return error; } - error = d_encryption_sp->pushIncomingCipherText(*d_receiveBlob_sp); + // Pushing incoming cipher text may invoke the handshake callback, which + // may reset 'd_encryption_sp' if the handshake fails. Retain a + // reference to the encryption session so it is not destroyed while + // one of its member functions is still executing. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->pushIncomingCipherText(*d_receiveBlob_sp); if (NTCCFG_UNLIKELY(error)) { return error; } @@ -3203,8 +3425,98 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_receiveQueue.pushEntry(entry); } - if (NTCCFG_UNLIKELY(downgradeAbortively)) { + // Receiving the peer's TLS shutdown, or unencrypted leftovers after + // the encrypted data, downgrades the connection, which resets the + // encryption session: a cross-cutting transition that requires both + // mutexes. The lock order permits acquiring 'd_sendMutex' while + // holding 'd_receiveMutex'. + + if (NTCCFG_UNLIKELY(downgradeAbortively || + d_encryption_sp->isShutdownReceived())) + { + if (exclusive) { + return this->privateReceiveDowngrade(self, + downgradeAbortively); + } + + LockGuard sendLock(&d_sendMutex); + return this->privateReceiveDowngrade(self, downgradeAbortively); + } + + // Enqueue any outgoing cipher text generated while decrypting, e.g., + // the response to a TLS 1.3 key update. + + if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + if (exclusive) { + return this->privateSendOutgoingCipherText(self); + } + + LockGuard sendLock(&d_sendMutex); + return this->privateSendOutgoingCipherText(self); + } + + return ntsa::Error(); + } +} + +ntsa::Error StreamSocket::privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive) +{ + ntsa::Error error; + + if (!d_encryption_sp) { + return ntsa::Error(); + } + + if (NTCCFG_UNLIKELY(abortive)) { + if (!d_encryption_sp->isShutdownSent()) { + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_INITIATED); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeInitiated( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_reactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + + d_encryption_sp.reset(); + + if (d_session_sp) { + ntca::DowngradeContext context; + + ntca::DowngradeEvent event; + event.setType(ntca::DowngradeEventType::e_COMPLETE); + event.setContext(context); + + ntcs::Dispatch::announceDowngradeComplete(d_session_sp, + self, + event, + d_sessionStrand_sp, + d_reactorStrand_sp, + self, + true, + &d_receiveMutex); + } + } + else { + if (d_encryption_sp->isShutdownReceived()) { if (!d_encryption_sp->isShutdownSent()) { + error = d_encryption_sp->shutdown(); + if (error) { + return error; + } + if (d_session_sp) { ntca::DowngradeContext context; @@ -3219,11 +3531,18 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( d_sessionStrand_sp, d_reactorStrand_sp, self, - false, - &d_mutex); + true, + &d_receiveMutex); } } + } + error = this->privateSendOutgoingCipherText(self); + if (error) { + return error; + } + + if (d_encryption_sp->isShutdownFinished()) { d_encryption_sp.reset(); if (d_session_sp) { @@ -3233,98 +3552,52 @@ ntsa::Error StreamSocket::privateDequeueReceiveBuffer( event.setType(ntca::DowngradeEventType::e_COMPLETE); event.setContext(context); - ntcs::Dispatch::announceDowngradeComplete(d_session_sp, - self, - event, - d_sessionStrand_sp, - d_reactorStrand_sp, - self, - false, - &d_mutex); + ntcs::Dispatch::announceDowngradeComplete( + d_session_sp, + self, + event, + d_sessionStrand_sp, + d_reactorStrand_sp, + self, + true, + &d_receiveMutex); } } - else { - if (d_encryption_sp->isShutdownReceived()) { - if (!d_encryption_sp->isShutdownSent()) { - error = d_encryption_sp->shutdown(); - if (error) { - return error; - } - - if (d_session_sp) { - ntca::DowngradeContext context; - - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_INITIATED); - event.setContext(context); - - ntcs::Dispatch::announceDowngradeInitiated( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_reactorStrand_sp, - self, - false, - &d_mutex); - } - } - } - - if (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { - bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); + } - while ( - NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) - { - error = - d_encryption_sp->popOutgoingCipherText(&cipherData); - if (error) { - return error; - } - } + return ntsa::Error(); +} - if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { - ntcq::SendState state; - state.setCounter(d_sendCounter++); - - error = this->privateSendRaw(self, - cipherData, - state, - ntca::SendOptions(), - ntca::SendContext(), - d_sendComplete); - if (error) { - return error; - } - } - } +ntsa::Error StreamSocket::privateSendOutgoingCipherText( + const bsl::shared_ptr& self) +{ + ntsa::Error error; - if (d_encryption_sp->isShutdownFinished()) { - d_encryption_sp.reset(); + bdlbb::Blob cipherData(d_outgoingBufferFactory_sp.get()); - if (d_session_sp) { - ntca::DowngradeContext context; + while (NTCCFG_UNLIKELY(d_encryption_sp->hasOutgoingCipherText())) { + error = d_encryption_sp->popOutgoingCipherText(&cipherData); + if (error) { + return error; + } + } - ntca::DowngradeEvent event; - event.setType(ntca::DowngradeEventType::e_COMPLETE); - event.setContext(context); + if (NTCCFG_UNLIKELY(cipherData.length() > 0)) { + ntcq::SendState state; + state.setCounter(d_sendCounter++); - ntcs::Dispatch::announceDowngradeComplete( - d_session_sp, - self, - event, - d_sessionStrand_sp, - d_reactorStrand_sp, - self, - false, - &d_mutex); - } - } + error = this->privateSendRaw(self, + cipherData, + state, + ntca::SendOptions(), + ntca::SendContext(), + d_sendComplete); + if (error) { + return error; } - - return ntsa::Error(); } + + return ntsa::Error(); } ntsa::Error StreamSocket::privateDequeueReceiveBufferRaw( @@ -3389,10 +3662,11 @@ ntsa::Error StreamSocket::privateDequeueReceiveBufferRaw( return error; } else if (error == ntsa::Error::e_EOF) { + // The shutdown of reception is performed by the caller + // ('processSocketReadable') after releasing the receive mutex, so + // that the cross-cutting shutdown sequence can acquire the mutexes + // in canonical order. NTCR_STREAMSOCKET_LOG_END_OF_FILE(); - this->privateShutdownReceive(self, - ntsa::ShutdownOrigin::e_REMOTE, - false); return error; } else { @@ -3417,10 +3691,11 @@ ntsa::Error StreamSocket::privateDequeueReceiveBufferRaw( d_totalBytesReceived += context->bytesReceived(); } else { + // The shutdown of reception is performed by the caller + // ('processSocketReadable') after releasing the receive mutex, so that + // the cross-cutting shutdown sequence can acquire the mutexes in + // canonical order. NTCR_STREAMSOCKET_LOG_END_OF_FILE(); - this->privateShutdownReceive(self, - ntsa::ShutdownOrigin::e_REMOTE, - false); return ntsa::Error(ntsa::Error::e_EOF); } @@ -3524,7 +3799,7 @@ ntsa::Error StreamSocket::privateSendRaw( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -3660,7 +3935,7 @@ ntsa::Error StreamSocket::privateSendRaw( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -3793,7 +4068,7 @@ ntsa::Error StreamSocket::privateSendEncrypted( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -3846,7 +4121,7 @@ ntsa::Error StreamSocket::privateSendEncrypted( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_sendMutex); } return ntsa::Error(); @@ -4166,7 +4441,7 @@ ntsa::Error StreamSocket::privateOpen( d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4183,7 +4458,7 @@ void StreamSocket::processSourceEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; ntca::BindContext bindContext; @@ -4240,7 +4515,7 @@ void StreamSocket::processSourceEndpointResolution( ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } } @@ -4256,7 +4531,8 @@ void StreamSocket::processRemoteEndpointResolution( bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) { @@ -4357,8 +4633,8 @@ void StreamSocket::processRemoteEndpointResolution( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, - &d_mutex); + true, + &d_receiveMutex); if (NTCCFG_UNLIKELY(d_detachState.mode() == ntcs::DetachMode::e_INITIATED)) @@ -4422,12 +4698,23 @@ ntsa::Error StreamSocket::privateUpgrade( bdlf::MemFnUtil::memFn(&StreamSocket::privateEncryptionHandshake, this); - error = - d_encryption_sp->initiateHandshake(upgradeOptions, handshakeCallback); + // Initiating the handshake and pushing incoming cipher text may invoke + // the handshake callback, which resets 'd_encryption_sp' if the handshake + // fails. Retain a reference to the encryption session so it is not + // destroyed while one of its member functions is still executing, and + // stop if the failure of the upgrade has already been processed. + + const bsl::shared_ptr encryption = d_encryption_sp; + + error = encryption->initiateHandshake(upgradeOptions, handshakeCallback); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + // Push any unconsumed receive queue data that must be now considered // cipher text into the encryption session. @@ -4435,12 +4722,15 @@ ntsa::Error StreamSocket::privateUpgrade( // Push the receive queue data into the encryption session as incoming // cipher text. - error = - d_encryption_sp->pushIncomingCipherText(*d_receiveQueue.data()); + error = encryption->pushIncomingCipherText(*d_receiveQueue.data()); if (error) { return error; } + if (d_encryption_sp != encryption) { + return ntsa::Error(); + } + bdlbb::BlobUtil::erase(d_receiveQueue.data().get(), 0, d_receiveQueue.data()->length()); @@ -4558,7 +4848,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -4603,7 +4893,7 @@ ntsa::Error StreamSocket::privateDowngrade( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -4663,7 +4953,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } d_encryption_sp.reset(); @@ -4682,7 +4972,7 @@ ntsa::Error StreamSocket::privateDowngradeAbortively( ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -4894,8 +5184,8 @@ void StreamSocket::privateRetryConnectToEndpoint( d_sessionStrand_sp, ntci::Strand::unknown(), self, - false, - &d_mutex); + true, + &d_receiveMutex); } if (d_detachState.mode() == ntcs::DetachMode::e_INITIATED) { @@ -5230,7 +5520,7 @@ void StreamSocket::privateZeroCopyUpdate( d_reactorStrand_sp, self, false, - &d_mutex); + &d_sendMutex); } } } @@ -5282,7 +5572,8 @@ StreamSocket::StreamSocket( const bsl::shared_ptr& metrics, bslma::Allocator* basicAllocator) : d_object("ntcr::StreamSocket") -, d_mutex() +, d_sendMutex() +, d_receiveMutex() , d_transport(ntsa::Transport::e_UNDEFINED) , d_systemHandle(ntsa::k_INVALID_HANDLE) , d_systemSourceEndpoint() @@ -5470,7 +5761,7 @@ ntsa::Error StreamSocket::open() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self); } @@ -5479,7 +5770,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport); } @@ -5489,7 +5780,7 @@ ntsa::Error StreamSocket::open(ntsa::Transport::Value transport, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle); } @@ -5500,7 +5791,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket); } @@ -5512,7 +5803,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, handle, acceptor); } @@ -5524,7 +5815,7 @@ ntsa::Error StreamSocket::open( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return this->privateOpen(self, transport, streamSocket, acceptor); } @@ -5544,7 +5835,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -5584,7 +5875,7 @@ ntsa::Error StreamSocket::bind(const ntsa::Endpoint& endpoint, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); } return ntsa::Error(); @@ -5613,7 +5904,7 @@ ntsa::Error StreamSocket::bind(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -5660,7 +5951,7 @@ ntsa::Error StreamSocket::connect(const ntsa::Endpoint& endpoint, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -5811,7 +6102,7 @@ ntsa::Error StreamSocket::connect(const bsl::string& name, bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); ntsa::Error error; @@ -5936,7 +6227,8 @@ ntsa::Error StreamSocket::upgrade( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6094,7 +6386,16 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Both unencrypted and encrypted sends need only the send mutex. The + // encryption object is co-guarded, so it is stable while the send mutex is + // held, and it is itself thread safe. Every path that pops outgoing + // cipher text and enqueues it to the send queue holds the send mutex, so + // TLS records are enqueued in the order they are generated. Pushing + // outgoing plain text never invokes the handshake callback (see + // 'ntci::Encryption'), so the encrypted send path never performs a + // cross-cutting transition. + + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6137,7 +6438,7 @@ ntsa::Error StreamSocket::send(const bdlbb::Blob& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -6221,7 +6522,10 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + // Both unencrypted and encrypted sends need only the send mutex. See the + // 'bdlbb::Blob' overload of this function. + + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6264,7 +6568,7 @@ ntsa::Error StreamSocket::send(const ntsa::Data& data, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_sendMutex); } } @@ -6339,7 +6643,7 @@ ntsa::Error StreamSocket::receive(ntca::ReceiveContext* context, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6474,7 +6778,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -6588,7 +6892,7 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntci::Strand::unknown(), self, defer, - &d_mutex); + &d_receiveMutex); bool receiveQueueHighWatermarkViolatedAfter = d_receiveQueue.isHighWatermarkViolated(); @@ -6650,14 +6954,14 @@ ntsa::Error StreamSocket::receive(const ntca::ReceiveOptions& options, ntsa::Error StreamSocket::registerResolver( const bsl::shared_ptr& resolver) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver = resolver; return ntsa::Error(); } ntsa::Error StreamSocket::deregisterResolver() { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_resolver.reset(); return ntsa::Error(); } @@ -6667,7 +6971,8 @@ ntsa::Error StreamSocket::registerManager( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (manager) { d_manager_sp = manager; @@ -6687,7 +6992,8 @@ ntsa::Error StreamSocket::registerManager( ntsa::Error StreamSocket::deregisterManager() { - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_manager_sp.reset(); d_managerStrand_sp.reset(); @@ -6700,7 +7006,8 @@ ntsa::Error StreamSocket::registerSession( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (session) { d_session_sp = session; @@ -6730,7 +7037,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -6767,7 +7075,8 @@ ntsa::Error StreamSocket::registerSessionCallback( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (callback) { bsl::shared_ptr session; @@ -6797,7 +7106,8 @@ ntsa::Error StreamSocket::registerSessionCallback( ntsa::Error StreamSocket::deregisterSession() { - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); d_session_sp.reset(); d_sessionStrand_sp.reset(); @@ -6808,7 +7118,7 @@ ntsa::Error StreamSocket::deregisterSession() ntsa::Error StreamSocket::setZeroCopyThreshold(bsl::size_t value) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return this->privateZeroCopyEngage(self, value); } @@ -6824,7 +7134,7 @@ ntsa::Error StreamSocket::setConnectRateLimiter( ntsa::Error StreamSocket::setWriteDeflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); d_sendDeflater_sp = compression; @@ -6836,7 +7146,7 @@ ntsa::Error StreamSocket::setWriteRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6865,7 +7175,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6893,7 +7203,7 @@ ntsa::Error StreamSocket::setWriteQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -6904,7 +7214,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6932,7 +7242,7 @@ ntsa::Error StreamSocket::setWriteQueueHighWatermark(bsl::size_t highWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -6944,7 +7254,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -6973,7 +7283,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -6995,7 +7305,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7005,7 +7315,7 @@ ntsa::Error StreamSocket::setWriteQueueWatermarks(bsl::size_t lowWatermark, ntsa::Error StreamSocket::setReadInflater( const bsl::shared_ptr& compression) { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); d_receiveInflater_sp = compression; @@ -7017,7 +7327,7 @@ ntsa::Error StreamSocket::setReadRateLimiter( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7046,7 +7356,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7076,7 +7386,7 @@ ntsa::Error StreamSocket::setReadQueueLowWatermark(bsl::size_t lowWatermark) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } } @@ -7087,7 +7397,7 @@ ntsa::Error StreamSocket::setReadQueueHighWatermark(bsl::size_t highWatermark) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7113,7 +7423,7 @@ ntsa::Error StreamSocket::setReadQueueWatermarks(bsl::size_t lowWatermark, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7146,7 +7456,7 @@ ntsa::Error StreamSocket::timestampOutgoingData(bool enable) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return privateTimestampOutgoingData(self, enable); } @@ -7155,7 +7465,7 @@ ntsa::Error StreamSocket::timestampIncomingData(bool enable) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return privateTimestampIncomingData(self, enable); } @@ -7165,7 +7475,8 @@ ntsa::Error StreamSocket::relaxFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7182,7 +7493,8 @@ ntsa::Error StreamSocket::applyFlowControl( { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7224,7 +7536,8 @@ ntsa::Error StreamSocket::cancel(const ntca::ConnectToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7250,7 +7563,8 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7284,7 +7598,7 @@ ntsa::Error StreamSocket::cancel(const ntca::UpgradeToken& token) ntci::Strand::unknown(), self, true, - &d_mutex); + &d_receiveMutex); } this->privateFail(self, ntsa::Error(ntsa::Error::e_CANCELLED)); @@ -7299,7 +7613,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7333,7 +7647,7 @@ ntsa::Error StreamSocket::cancel(const ntca::SendToken& token) d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -7345,7 +7659,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); NTCI_LOG_CONTEXT(); @@ -7374,7 +7688,7 @@ ntsa::Error StreamSocket::cancel(const ntca::ReceiveToken& token) d_reactorStrand_sp, self, true, - &d_mutex); + &d_receiveMutex); return ntsa::Error(); } @@ -7386,7 +7700,8 @@ ntsa::Error StreamSocket::downgrade() { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); return this->privateDowngrade(self, ntca::DowngradeOptions()); } @@ -7395,7 +7710,8 @@ ntsa::Error StreamSocket::downgrade(const ntca::DowngradeOptions& options) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); if (options.abortive().isNull() || !options.abortive().value()) { return this->privateDowngrade(self, options); @@ -7410,7 +7726,8 @@ ntsa::Error StreamSocket::shutdown(ntsa::ShutdownType::Value direction, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); NTCI_LOG_CONTEXT(); @@ -7459,7 +7776,8 @@ ntsa::Error StreamSocket::release(ntsa::Handle* result, { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); *result = ntsa::k_INVALID_HANDLE; @@ -7495,7 +7813,8 @@ void StreamSocket::close(const ntci::CloseCallback& callback) { bsl::shared_ptr self = this->getSelf(this); - LockGuard lock(&d_mutex); + LockGuard receiveLock(&d_receiveMutex); + LockGuard sendLock(&d_sendMutex); this->privateClose(self, callback); } @@ -7631,7 +7950,7 @@ ntsa::Endpoint StreamSocket::remoteEndpoint() const bsl::shared_ptr StreamSocket::sourceCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -7644,7 +7963,7 @@ bsl::shared_ptr StreamSocket::sourceCertificate() bsl::shared_ptr StreamSocket::remoteCertificate() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -7656,7 +7975,7 @@ bsl::shared_ptr StreamSocket::remoteCertificate() bsl::shared_ptr StreamSocket::privateKey() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); bsl::shared_ptr result; if (d_encryption_sp) { @@ -7700,49 +8019,49 @@ bsl::size_t StreamSocket::threadIndex() const bsl::size_t StreamSocket::readQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.size(); } bsl::size_t StreamSocket::readQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.lowWatermark(); } bsl::size_t StreamSocket::readQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_receiveQueue.highWatermark(); } bsl::size_t StreamSocket::writeQueueSize() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.size(); } bsl::size_t StreamSocket::writeQueueLowWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.lowWatermark(); } bsl::size_t StreamSocket::writeQueueHighWatermark() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_sendQueue.highWatermark(); } bsl::size_t StreamSocket::totalBytesSent() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_sendMutex); return d_totalBytesSent; } bsl::size_t StreamSocket::totalBytesReceived() const { - LockGuard lock(&d_mutex); + LockGuard lock(&d_receiveMutex); return d_totalBytesReceived; } @@ -7786,11 +8105,16 @@ void StreamSocket::getInfo(ntsa::SocketInfo* result) const d_publicRemoteEndpoint.load(&remoteEndpoint); { - LockGuard lock(&d_mutex); + LockGuard sendLock(&d_sendMutex); + sendQueueSize = d_sendQueue.size(); + } - sendQueueSize = d_sendQueue.size(); + { + LockGuard receiveLock(&d_receiveMutex); receiveQueueSize = d_receiveQueue.size(); + } + { if (d_shutdownState.completed()) { socketState = ntsa::SocketState::e_CLOSED; } diff --git a/groups/ntc/ntcr/ntcr_streamsocket.h b/groups/ntc/ntcr/ntcr_streamsocket.h index 2f6eae2e..cecb86f0 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.h +++ b/groups/ntc/ntcr/ntcr_streamsocket.h @@ -91,7 +91,55 @@ class StreamSocket : public ntci::StreamSocket, typedef ntccfg::LockGuard LockGuard; ntccfg::Object d_object; - mutable Mutex d_mutex; + // LOCKING STRATEGY + // + // 'd_receiveMutex' guards the read path (the receive queue and + // receive-direction reactor interest) and the shared lifecycle and + // control state of the socket: the connect/upgrade machinery and the + // open, shutdown, and detach state machines' *transitions*. Conceptually, + // it guards the connectedness and potential readability of the socket. + // 'd_sendMutex' guards the separate write path (the send queue, zero-copy + // queue, outgoing timestamping, and send-direction reactor interest), so + // that sending is not serialized against receiving. + // + // The encryption object is itself thread safe. Encrypting outgoing data + // requires only 'd_sendMutex': every path that pops outgoing cipher text + // and enqueues it to the send queue holds 'd_sendMutex', so TLS records + // are enqueued in the order they are generated, and pushing outgoing plain + // text never invokes the handshake callback. + // + // Decrypting incoming data in the steady state (i.e., once the handshake + // has completed) requires only 'd_receiveMutex': the handshake callback + // can no longer be invoked. Decrypting incoming data while the handshake + // is in progress holds both mutexes, since it may complete or fail the + // handshake (a cross-cutting transition). + // + // 'd_flowControlState' is itself thread safe (lock-free), so a single + // instance is shared by both directions: the send path applies/relaxes it + // with 'e_SEND' under 'd_sendMutex' and the receive path with 'e_RECEIVE' + // under 'd_receiveMutex'. + // + // When both are held they are always acquired in the order + // 'd_receiveMutex' -> 'd_sendMutex'. This order follows the flow of data: + // the read path may feed the write path, so it may acquire 'd_sendMutex' + // while holding 'd_receiveMutex', but the write path never acquires + // 'd_receiveMutex' while holding 'd_sendMutex'. Consequently, the read + // path performs every cross-cutting transition it discovers (enqueueing + // outgoing cipher text generated while decrypting, receiving the peer's + // TLS shutdown, end-of-file, or failure) in place, by additionally + // acquiring 'd_sendMutex' without releasing 'd_receiveMutex'. The write + // path holds only 'd_sendMutex'; when it must perform a cross-cutting + // lifecycle transition (connection establishment, failure, shutdown) it + // first releases 'd_sendMutex', then acquires both in canonical order. + // + // The observable lifecycle predicates ('d_openState', 'd_shutdownState', + // 'd_detachState.mode()') are atomic and may be read without any lock. The + // co-guarded members ('d_socket_sp', 'd_encryption_sp', 'd_session_sp', + // 'd_sessionStrand_sp', 'd_manager_sp', 'd_managerStrand_sp') are written + // only while holding both mutexes and may therefore be read while holding + // either one of them. + mutable Mutex d_sendMutex; + mutable Mutex d_receiveMutex; ntsa::Transport::Value d_transport; ntsa::Handle d_systemHandle; ntsa::Endpoint d_systemSourceEndpoint; @@ -147,7 +195,7 @@ class StreamSocket : public ntci::StreamSocket, bsl::shared_ptr d_connectRetryTimer_sp; bsl::shared_ptr d_connectRateLimiter_sp; bsl::shared_ptr d_connectRateTimer_sp; - bool d_connectInProgress; + bsls::AtomicBool d_connectInProgress; ntca::UpgradeOptions d_upgradeOptions; ntci::UpgradeCallback d_upgradeCallback; bsl::shared_ptr d_upgradeTimer_sp; @@ -241,9 +289,13 @@ class StreamSocket : public ntci::StreamSocket, const bsl::string& details); /// Process the readability of the socket by performing one read - /// iteration. + /// iteration. If the specified 'exclusive' flag is true, the caller holds + /// both 'd_receiveMutex' and 'd_sendMutex', and all event announcements + /// and callback invocations made during the iteration are deferred. Otherwise, the caller holds only 'd_receiveMutex'. Return + /// the error. ntsa::Error privateSocketReadableIteration( - const bsl::shared_ptr& self); + const bsl::shared_ptr& self, + bool exclusive); /// Process the writability of the socket indicating the connection /// is established. @@ -251,9 +303,14 @@ class StreamSocket : public ntci::StreamSocket, const bsl::shared_ptr& self); /// Process the writability of the socket by performing one write - /// iteration. + /// iteration. If a graceful shutdown sentinel is drained during the + /// iteration, set the specified '*shutdownSendPending' to true instead of + /// shutting down for sending inline, so that the caller + /// ('processSocketWritable') can perform the cross-cutting shutdown + /// sequence after releasing the send mutex. ntsa::Error privateSocketWritableIteration( - const bsl::shared_ptr& self); + const bsl::shared_ptr& self, + bool* shutdownSendPending); /// Process the writability of the socket by performing one write /// iteration from the contiguous range of suitable entries at the front @@ -262,9 +319,12 @@ class StreamSocket : public ntci::StreamSocket, const bsl::shared_ptr& self); /// Process the writability of the socket by performing one write - /// iteration from the entry at the front of the write queue. + /// iteration from the entry at the front of the write queue. If a graceful + /// shutdown sentinel is drained, set the specified '*shutdownSendPending' + /// to true rather than shutting down for sending inline. ntsa::Error privateSocketWritableIterationFront( - const bsl::shared_ptr& self); + const bsl::shared_ptr& self, + bool* shutdownSendPending); /// Indicate a connection failure has occurred. If the specified 'defer' /// flag is true, ensure the announcement is deferred. If the specified @@ -344,7 +404,8 @@ class StreamSocket : public ntci::StreamSocket, bool lock); /// Enable copying from the socket buffers in the specified 'direction'. - /// The behavior is undefined unless 'd_mutex' is locked. + /// The behavior is undefined unless the mutex guarding the 'direction' + /// (or both mutexes, for 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateRelaxFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -353,7 +414,8 @@ class StreamSocket : public ntci::StreamSocket, /// Disable copying from socket buffers in the specified 'direction' /// according to the specified 'mode'. The behavior is undefined unless - /// 'd_mutex' is locked. + /// the mutex guarding the 'direction' (or both mutexes, for + /// 'ntca::FlowControlType::e_BOTH') is locked. ntsa::Error privateApplyFlowControl( const bsl::shared_ptr& self, ntca::FlowControlType::Value direction, @@ -404,11 +466,33 @@ class StreamSocket : public ntci::StreamSocket, ntsa::Handle foreignHandle); /// Dequeue data from the socket receive buffer. Append to the - /// specified 'data' the data dequeued. Return the error. + /// specified 'data' the data dequeued. If the specified 'exclusive' flag + /// is true, the caller holds both 'd_receiveMutex' and 'd_sendMutex'. + /// Otherwise, the caller holds only 'd_receiveMutex', 'd_sendMutex' is + /// additionally acquired as necessary, and the behavior is undefined + /// unless the encryption handshake, if any, has completed. Return the + /// error. ntsa::Error privateDequeueReceiveBuffer( const bsl::shared_ptr& self, ntsa::ReceiveContext* context, - bdlbb::Blob* data); + bdlbb::Blob* data, + bool exclusive); + + /// Complete the downgrade from encrypted to unencrypted communication + /// initiated by the receipt of the peer's TLS shutdown, or, if the + /// specified 'abortive' flag is true, by the receipt of unencrypted + /// leftovers after the encrypted data. Enqueue any outgoing cipher text + /// generated as a result. The behavior is undefined unless both + /// 'd_receiveMutex' and 'd_sendMutex' are locked. Return the error. + ntsa::Error privateReceiveDowngrade( + const bsl::shared_ptr& self, + bool abortive); + + /// Pop all outgoing cipher text from the encryption session and enqueue + /// it for transmission. The behavior is undefined unless 'd_sendMutex' is + /// locked. Return the error. + ntsa::Error privateSendOutgoingCipherText( + const bsl::shared_ptr& self); /// Dequeue raw or encrypted data from the socket receive buffer. Append /// to the specified 'data' the data dequeued . Return the error. diff --git a/groups/ntc/ntcr/ntcr_streamsocket.t.cpp b/groups/ntc/ntcr/ntcr_streamsocket.t.cpp index 56139586..e388556e 100644 --- a/groups/ntc/ntcr/ntcr_streamsocket.t.cpp +++ b/groups/ntc/ntcr/ntcr_streamsocket.t.cpp @@ -4039,6 +4039,14 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica4) .ONCE() .RETURN(ntsa::Error::invalid()); + // The connect failure callback is now delivered deferred: the failure + // path holds more than one mutex, so the announcement is enqueued via + // the reactor rather than invoked synchronously. + ntci::Reactor::Functor deferredCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredCallback)); + ntca::ReactorEvent event; bsl::shared_ptr reactorSocket = socket; reactorSocket->processSocketWritable(event); @@ -4046,6 +4054,9 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica4) NTCI_LOG_DEBUG("Ensure that connection callback was called and " "connection error was indicated"); { + NTSCFG_TEST_TRUE(deferredCallback); + deferredCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -4238,12 +4249,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica5) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -4440,12 +4462,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyShutdownEsoterica6) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5019,6 +5052,14 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica4) .ONCE() .RETURN(ntsa::Error::invalid()); + // The connect failure callback is now delivered deferred: the failure + // path holds more than one mutex, so the announcement is enqueued via + // the reactor rather than invoked synchronously. + ntci::Reactor::Functor deferredCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredCallback)); + ntca::ReactorEvent event; bsl::shared_ptr reactorSocket = socket; reactorSocket->processSocketWritable(event); @@ -5026,6 +5067,9 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica4) NTCI_LOG_DEBUG("Ensure that connection callback was called and " "connection error was indicated"); { + NTSCFG_TEST_TRUE(deferredCallback); + deferredCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5222,12 +5266,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica5) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5431,12 +5486,23 @@ NTSCFG_TEST_FUNCTION(ntcr::StreamSocketTest::verifyCloseEsoterica6) .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); @@ -5620,12 +5686,23 @@ NTSCFG_TEST_FUNCTION( .ONCE() .RETURN(StreamSocketTest::Fixture::k_NO_ERROR); + // The connect failure callback is now delivered deferred (the async + // detachment completion holds all three mutexes), so it is enqueued + // via the reactor before the postponed calls are moved and executed. + ntci::Reactor::Functor deferredConnectCallback; + NTF_EXPECT(*test.d_reactorMock, execute, IGNORE_ARG) + .ONCE() + .SAVE_ARG_1(ntscfg::Mock::TO(&deferredConnectCallback)); + NTF_EXPECT(*test.d_reactorMock, moveAndExecute, IGNORE_ARG, IGNORE_ARG) .ONCE() .SAVE_ARG_1(ntscfg::Mock::TO_DEREF(&functorSequence)); detachCallback(test.d_nullStrand); + NTSCFG_TEST_TRUE(deferredConnectCallback); + deferredConnectCallback(); + NTSCFG_TEST_TRUE(test.d_connectResult.has_value()); NTSCFG_TEST_EQ(test.d_connectResult.value().type(), ntca::ConnectEventType::e_ERROR); diff --git a/groups/ntc/ntcs/ntcs_detachstate.h b/groups/ntc/ntcs/ntcs_detachstate.h index 3044641a..ba5e24b4 100644 --- a/groups/ntc/ntcs/ntcs_detachstate.h +++ b/groups/ntc/ntcs/ntcs_detachstate.h @@ -21,6 +21,7 @@ BSLS_IDENT("$Id: $") #include #include +#include namespace BloombergLP { namespace ntcs { @@ -73,12 +74,14 @@ class DetachGoal /// Provide a enumeration that indicates state of a socket detachment process. /// /// @par Thread Safety -/// This class is not thread safe. +/// The mode is stored atomically: 'mode' may be called concurrently with a +/// single thread invoking 'setMode'. The goal is not synchronized and must +/// only be accessed while externally serialized. /// /// @ingroup module_ntcs class DetachState { - ntcs::DetachMode::Value d_mode; + bsls::AtomicInt d_mode; ntcs::DetachGoal::Value d_goal; private: @@ -121,7 +124,7 @@ DetachState::~DetachState() NTCCFG_INLINE void DetachState::setMode(ntcs::DetachMode::Value mode) { - d_mode = mode; + d_mode.storeRelease(mode); } NTCCFG_INLINE @@ -133,7 +136,7 @@ void DetachState::setGoal(ntcs::DetachGoal::Value goal) NTCCFG_INLINE ntcs::DetachMode::Value DetachState::mode() const { - return d_mode; + return static_cast(d_mode.loadAcquire()); } NTCCFG_INLINE diff --git a/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp b/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp index 782d37dc..96fc05ef 100644 --- a/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp +++ b/groups/ntc/ntcs/ntcs_flowcontrolstate.cpp @@ -24,11 +24,7 @@ namespace BloombergLP { namespace ntcs { FlowControlState::FlowControlState() -: d_enableSend(false) -, d_enableReceive(false) -, d_lockSend(false) -, d_lockReceive(false) -, d_closed(false) +: d_flags(0) { } @@ -42,12 +38,6 @@ bool FlowControlState::apply(ntcs::FlowControlContext* context, { context->reset(); - if (d_closed) { - return false; - } - - bool result = false; - bool applySend = false; bool applyReceive = false; @@ -64,36 +54,58 @@ bool FlowControlState::apply(ntcs::FlowControlContext* context, break; } - if (applySend) { - if (!d_lockSend) { - if (d_enableSend) { - d_enableSend = false; - result = true; - } - } + // Commit the compound change atomically: read the current flags, compute + // the new flags and result from that snapshot, then compare-and-swap. If + // the swap loses a race with a concurrent operation, retry. + + for (;;) { + const unsigned int oldFlags = d_flags.load(); - if (lock) { - d_lockSend = true; + if (oldFlags & e_CLOSED) { + return false; } - } - if (applyReceive) { - if (!d_lockReceive) { - if (d_enableReceive) { - d_enableReceive = false; - result = true; + unsigned int newFlags = oldFlags; + bool result = false; + + if (applySend) { + if (!(newFlags & e_LOCK_SEND)) { + if (newFlags & e_ENABLE_SEND) { + newFlags &= ~static_cast(e_ENABLE_SEND); + result = true; + } + } + + if (lock) { + newFlags |= e_LOCK_SEND; } } - if (lock) { - d_lockReceive = true; + if (applyReceive) { + if (!(newFlags & e_LOCK_RECEIVE)) { + if (newFlags & e_ENABLE_RECEIVE) { + newFlags &= ~static_cast(e_ENABLE_RECEIVE); + result = true; + } + } + + if (lock) { + newFlags |= e_LOCK_RECEIVE; + } } - } - context->setEnableSend(d_enableSend); - context->setEnableReceive(d_enableReceive); + if (newFlags == oldFlags) { + context->setEnableSend((oldFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((oldFlags & e_ENABLE_RECEIVE) != 0); + return result; + } - return result; + if (d_flags.testAndSwap(oldFlags, newFlags) == oldFlags) { + context->setEnableSend((newFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((newFlags & e_ENABLE_RECEIVE) != 0); + return result; + } + } } bool FlowControlState::relax(ntcs::FlowControlContext* context, @@ -102,12 +114,6 @@ bool FlowControlState::relax(ntcs::FlowControlContext* context, { context->reset(); - if (d_closed) { - return false; - } - - bool result = false; - bool relaxSend = false; bool relaxReceive = false; @@ -124,54 +130,67 @@ bool FlowControlState::relax(ntcs::FlowControlContext* context, break; } - if (relaxSend) { - if (unlock) { - d_lockSend = false; + for (;;) { + const unsigned int oldFlags = d_flags.load(); + + if (oldFlags & e_CLOSED) { + return false; } - if (!d_lockSend) { - if (!d_enableSend) { - d_enableSend = true; - result = true; + unsigned int newFlags = oldFlags; + bool result = false; + + if (relaxSend) { + if (unlock) { + newFlags &= ~static_cast(e_LOCK_SEND); } - } - } - if (relaxReceive) { - if (unlock) { - d_lockReceive = false; + if (!(newFlags & e_LOCK_SEND)) { + if (!(newFlags & e_ENABLE_SEND)) { + newFlags |= e_ENABLE_SEND; + result = true; + } + } } - if (!d_lockReceive) { - if (!d_enableReceive) { - d_enableReceive = true; - result = true; + if (relaxReceive) { + if (unlock) { + newFlags &= ~static_cast(e_LOCK_RECEIVE); + } + + if (!(newFlags & e_LOCK_RECEIVE)) { + if (!(newFlags & e_ENABLE_RECEIVE)) { + newFlags |= e_ENABLE_RECEIVE; + result = true; + } } } - } - context->setEnableSend(d_enableSend); - context->setEnableReceive(d_enableReceive); + if (newFlags == oldFlags) { + context->setEnableSend((oldFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((oldFlags & e_ENABLE_RECEIVE) != 0); + return result; + } - return result; + if (d_flags.testAndSwap(oldFlags, newFlags) == oldFlags) { + context->setEnableSend((newFlags & e_ENABLE_SEND) != 0); + context->setEnableReceive((newFlags & e_ENABLE_RECEIVE) != 0); + return result; + } + } } void FlowControlState::close() { - d_enableSend = false; - d_lockSend = false; - d_enableReceive = false; - d_lockReceive = false; - d_closed = true; + // Clear the enable and lock bits and set the closed bit. This is a full + // overwrite to a constant value, so a single atomic store is sufficient; + // any concurrent compare-and-swap will observe the change and retry. + d_flags.store(e_CLOSED); } void FlowControlState::reset() { - d_enableSend = false; - d_enableReceive = false; - d_lockSend = false; - d_lockReceive = false; - d_closed = false; + d_flags.store(0); } bool FlowControlState::rearm(ntcs::FlowControlContext* context, @@ -184,7 +203,9 @@ bool FlowControlState::rearm(ntcs::FlowControlContext* context, return false; } - if (d_closed) { + const unsigned int flags = d_flags.load(); + + if (flags & e_CLOSED) { return false; } @@ -207,21 +228,21 @@ bool FlowControlState::rearm(ntcs::FlowControlContext* context, } if (rearmSend) { - if (d_enableSend) { - BSLS_ASSERT(!d_lockSend); + if (flags & e_ENABLE_SEND) { + BSLS_ASSERT(!(flags & e_LOCK_SEND)); result = true; } } if (rearmReceive) { - if (d_enableReceive) { - BSLS_ASSERT(!d_lockReceive); + if (flags & e_ENABLE_RECEIVE) { + BSLS_ASSERT(!(flags & e_LOCK_RECEIVE)); result = true; } } - context->setEnableSend(d_enableSend); - context->setEnableReceive(d_enableReceive); + context->setEnableSend((flags & e_ENABLE_SEND) != 0); + context->setEnableReceive((flags & e_ENABLE_RECEIVE) != 0); return result; } diff --git a/groups/ntc/ntcs/ntcs_flowcontrolstate.h b/groups/ntc/ntcs/ntcs_flowcontrolstate.h index 1ba39fa7..e4d2ecb6 100644 --- a/groups/ntc/ntcs/ntcs_flowcontrolstate.h +++ b/groups/ntc/ntcs/ntcs_flowcontrolstate.h @@ -24,7 +24,6 @@ BSLS_IDENT("$Id: $") #include #include #include -#include namespace BloombergLP { namespace ntcs { @@ -33,16 +32,26 @@ namespace ntcs { /// Provide a mechanism to to manage flow control. /// /// @par Thread Safety -/// This class is not thread safe. +/// This class is thread safe. The entire flow control state is stored in a +/// single atomic word, and each manipulator commits its compound change +/// atomically using a compare-and-swap loop, so the class is safe for +/// concurrent use without any external synchronization. /// /// @ingroup module_ntcs class FlowControlState { - bool d_enableSend; - bool d_enableReceive; - bool d_lockSend; - bool d_lockReceive; - bool d_closed; + /// This enumeration defines the bit within 'd_flags' that stores each + /// component of the flow control state. + enum Flag { + e_ENABLE_SEND = 1 << 0, + e_ENABLE_RECEIVE = 1 << 1, + e_LOCK_SEND = 1 << 2, + e_LOCK_RECEIVE = 1 << 3, + e_CLOSED = 1 << 4 + }; + + // The flow control state, storing the flags enumerated by 'Flag'. + bsls::AtomicUint d_flags; private: FlowControlState(const FlowControlState&) BSLS_KEYWORD_DELETED; @@ -119,31 +128,31 @@ class FlowControlState NTCCFG_INLINE bool FlowControlState::wantSend() const { - return d_enableSend; + return (d_flags.load() & e_ENABLE_SEND) != 0; } NTCCFG_INLINE bool FlowControlState::wantReceive() const { - return d_enableReceive; + return (d_flags.load() & e_ENABLE_RECEIVE) != 0; } NTCCFG_INLINE bool FlowControlState::lockSend() const { - return d_lockSend; + return (d_flags.load() & e_LOCK_SEND) != 0; } NTCCFG_INLINE bool FlowControlState::lockReceive() const { - return d_lockReceive; + return (d_flags.load() & e_LOCK_RECEIVE) != 0; } NTCCFG_INLINE bool FlowControlState::closed() const { - return d_closed; + return (d_flags.load() & e_CLOSED) != 0; } } // end namespace ntcs diff --git a/groups/ntc/ntcs/ntcs_openstate.cpp b/groups/ntc/ntcs/ntcs_openstate.cpp index 7ee2ec4f..2fb26fcc 100644 --- a/groups/ntc/ntcs/ntcs_openstate.cpp +++ b/groups/ntc/ntcs/ntcs_openstate.cpp @@ -25,7 +25,7 @@ namespace ntcs { const char* OpenState::toString() const { - switch (d_value) { + switch (static_cast(d_value.loadAcquire())) { case e_DEFAULT: return "DEFAULT"; case e_WAITING: diff --git a/groups/ntc/ntcs/ntcs_openstate.h b/groups/ntc/ntcs/ntcs_openstate.h index fb19c233..94c12968 100644 --- a/groups/ntc/ntcs/ntcs_openstate.h +++ b/groups/ntc/ntcs/ntcs_openstate.h @@ -21,6 +21,7 @@ BSLS_IDENT("$Id: $") #include #include +#include namespace BloombergLP { namespace ntcs { @@ -32,7 +33,9 @@ namespace ntcs { /// stream socket when connecting to a remote peer. /// /// @par Thread Safety -/// This class is not thread safe. +/// The state value is stored atomically: the accessors may be called +/// concurrently with each other and with a single thread invoking 'set', +/// but concurrent invocations of 'set' are not safe. /// /// @ingroup module_ntcs class OpenState @@ -57,7 +60,7 @@ class OpenState }; private: - Value d_value; + bsls::AtomicInt d_value; private: OpenState(const OpenState&) BSLS_KEYWORD_DELETED; @@ -126,61 +129,64 @@ OpenState::~OpenState() NTCCFG_INLINE void OpenState::set(Value value) { - d_value = value; + d_value.storeRelease(value); } NTCCFG_INLINE OpenState::Value OpenState::value() const { - return d_value; + return static_cast(d_value.loadAcquire()); } NTCCFG_INLINE bool OpenState::canBind() const { - return d_value == e_DEFAULT; + return d_value.loadAcquire() == e_DEFAULT; } NTCCFG_INLINE bool OpenState::canConnect() const { - return d_value == e_DEFAULT; + return d_value.loadAcquire() == e_DEFAULT; } NTCCFG_INLINE bool OpenState::canSend() const { - return d_value == e_CONNECTED; + return d_value.loadAcquire() == e_CONNECTED; } NTCCFG_INLINE bool OpenState::canReceive() const { - return d_value == e_CONNECTED || d_value == e_CLOSED; + const int value = d_value.loadAcquire(); + return value == e_CONNECTED || value == e_CLOSED; } NTCCFG_INLINE bool OpenState::is(Value value) const { - return d_value == value; + return d_value.loadAcquire() == value; } NTCCFG_INLINE bool OpenState::isEither(Value value1, Value value2) const { - return d_value == value1 || d_value == value2; + const int value = d_value.loadAcquire(); + return value == value1 || value == value2; } NTCCFG_INLINE bool OpenState::isNot(Value value) const { - return d_value != value; + return d_value.loadAcquire() != value; } NTCCFG_INLINE bool OpenState::isNotEither(Value value1, Value value2) const { - return d_value != value1 && d_value != value2; + const int value = d_value.loadAcquire(); + return value != value1 && value != value2; } } // end namespace ntcs diff --git a/groups/ntc/ntcs/ntcs_shutdownstate.cpp b/groups/ntc/ntcs/ntcs_shutdownstate.cpp index b0e15065..fe0f0539 100644 --- a/groups/ntc/ntcs/ntcs_shutdownstate.cpp +++ b/groups/ntc/ntcs/ntcs_shutdownstate.cpp @@ -25,6 +25,10 @@ namespace ntcs { ShutdownState::ShutdownState() : d_context() +, d_canSend(true) +, d_canReceive(true) +, d_initiated(false) +, d_completed(false) { } @@ -32,6 +36,14 @@ ShutdownState::~ShutdownState() { } +void ShutdownState::refreshAtomicState() +{ + d_canSend.storeRelease(!d_context.shutdownSend()); + d_canReceive.storeRelease(!d_context.shutdownReceive()); + d_initiated.storeRelease(d_context.shutdownInitiated()); + d_completed.storeRelease(d_context.shutdownCompleted()); +} + bool ShutdownState::tryShutdownSend(ntcs::ShutdownContext* context, bool keepHalfOpen) { @@ -66,6 +78,8 @@ bool ShutdownState::tryShutdownSend(ntcs::ShutdownContext* context, } } + this->refreshAtomicState(); + return true; } @@ -106,6 +120,8 @@ bool ShutdownState::tryShutdownReceive(ntcs::ShutdownContext* context, } } + this->refreshAtomicState(); + return true; } @@ -120,11 +136,15 @@ void ShutdownState::close() d_context.setShutdownReceive(true); d_context.setShutdownCompleted(true); + + this->refreshAtomicState(); } void ShutdownState::reset() { d_context.reset(); + + this->refreshAtomicState(); } } // close package namespace diff --git a/groups/ntc/ntcs/ntcs_shutdownstate.h b/groups/ntc/ntcs/ntcs_shutdownstate.h index c06bb2f2..80592c5c 100644 --- a/groups/ntc/ntcs/ntcs_shutdownstate.h +++ b/groups/ntc/ntcs/ntcs_shutdownstate.h @@ -38,17 +38,32 @@ namespace ntcs { /// when sequencing the shutdown of reads and writes for that socket. /// /// @par Thread Safety -/// This class is not thread safe. +/// The observable state accessors ('canSend', 'canReceive', 'initiated', +/// 'completed') read atomic mirrors and may be called concurrently with each +/// other and with a single thread invoking one of the manipulators. Concurrent +/// invocations of the manipulators are not safe. /// /// @ingroup module_ntcs class ShutdownState { ntcs::ShutdownContext d_context; + // The following atomic members mirror the observable predicates derived + // from 'd_context' so that the accessors may be evaluated without holding + // any external lock. They are refreshed from 'd_context' by every + // manipulator (see 'refreshAtomicState'). + bsls::AtomicBool d_canSend; + bsls::AtomicBool d_canReceive; + bsls::AtomicBool d_initiated; + bsls::AtomicBool d_completed; + private: ShutdownState(const ShutdownState&) BSLS_KEYWORD_DELETED; ShutdownState& operator=(const ShutdownState&) BSLS_KEYWORD_DELETED; + /// Refresh the atomic mirrors of the observable state from 'd_context'. + void refreshAtomicState(); + public: /// Create a new object in the default state. ShutdownState(); @@ -124,25 +139,25 @@ class ShutdownState NTCCFG_INLINE bool ShutdownState::initiated() const { - return d_context.shutdownInitiated(); + return d_initiated.loadAcquire(); } NTCCFG_INLINE bool ShutdownState::canSend() const { - return !d_context.shutdownSend(); + return d_canSend.loadAcquire(); } NTCCFG_INLINE bool ShutdownState::canReceive() const { - return !d_context.shutdownReceive(); + return d_canReceive.loadAcquire(); } NTCCFG_INLINE bool ShutdownState::completed() const { - return d_context.shutdownCompleted(); + return d_completed.loadAcquire(); } } // end namespace ntcs diff --git a/groups/ntc/ntctls/ntctls_plugin.cpp b/groups/ntc/ntctls/ntctls_plugin.cpp index 047eb1fc..4d6cae3d 100644 --- a/groups/ntc/ntctls/ntctls_plugin.cpp +++ b/groups/ntc/ntctls/ntctls_plugin.cpp @@ -9451,6 +9451,15 @@ ntsa::Error Session::process(LockGuard* lock) return ntsa::Error(ntsa::Error::e_INVALID); } + // Clear any stale errors in this thread's OpenSSL error queue, so that + // 'SSL_get_error' reports the outcome of the operations below rather than + // an unrelated, previously-unconsumed error. Otherwise, a stale error could + // be misinterpreted as a handshake failure, and the handshake callback + // could be invoked from a call other than 'initiateHandshake' or + // 'pushIncomingCipherText'. + + ERR_clear_error(); + bool announceHandshakeComplete = false; if (SSL_in_init(d_ssl_p)) { @@ -9476,12 +9485,17 @@ ntsa::Error Session::process(LockGuard* lock) d_handshakeCallback = ntci::Encryption::HandshakeCallback(); - lock->release()->unlock(); + // The handshake callback is empty if the failure of the handshake + // has already been announced. - handshakeCallback( - ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), - bsl::shared_ptr(), - description); + if (handshakeCallback) { + lock->release()->unlock(); + + handshakeCallback( + ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED), + bsl::shared_ptr(), + description); + } return ntsa::Error(ntsa::Error::e_NOT_AUTHORIZED); } @@ -10030,6 +10044,8 @@ ntsa::Error Session::initiateHandshake( d_handshakeCallback = callback; + ERR_clear_error(); + rc = SSL_do_handshake(d_ssl_p); if (rc != 1) { @@ -10267,6 +10283,8 @@ ntsa::Error Session::shutdown() enum { k_SHUTDOWN_STARTING = 0, k_SHUTDOWN_COMPLETE = 1 }; + ERR_clear_error(); + int rc = SSL_shutdown(d_ssl_p); if (rc != k_SHUTDOWN_STARTING && rc != k_SHUTDOWN_COMPLETE) {