Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(787)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: webrtc/p2p/base/dtlstransportchannel.cc

Issue 1414363002: Exposing DTLS transport state from TransportChannel. (Closed) Base URL: https://chromium.googlesource.com/external/webrtc.git@master
Patch Set: Fixing some comments. Created 5 years, 2 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « webrtc/p2p/base/dtlstransportchannel.h ('k') | webrtc/p2p/base/transport.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: webrtc/p2p/base/dtlstransportchannel.cc
diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc
index 148a19108d4149ea76ba9680375037ebb398aae9..a82cee0c706f5d37c3175b69383dc8161a1e8e93 100644
--- a/webrtc/p2p/base/dtlstransportchannel.cc
+++ b/webrtc/p2p/base/dtlstransportchannel.cc
@@ -94,7 +94,6 @@ DtlsTransportChannelWrapper::DtlsTransportChannelWrapper(
worker_thread_(rtc::Thread::Current()),
channel_(channel),
downward_(NULL),
- dtls_state_(STATE_NONE),
ssl_role_(rtc::SSL_CLIENT),
ssl_max_version_(rtc::SSL_PROTOCOL_DTLS_10) {
channel_->SignalWritableState.connect(this,
@@ -124,15 +123,13 @@ DtlsTransportChannelWrapper::~DtlsTransportChannelWrapper() {
void DtlsTransportChannelWrapper::Connect() {
// We should only get a single call to Connect.
- ASSERT(dtls_state_ == STATE_NONE ||
- dtls_state_ == STATE_OFFERED ||
- dtls_state_ == STATE_ACCEPTED);
+ ASSERT(dtls_state() == DTLS_TRANSPORT_NEW);
channel_->Connect();
}
bool DtlsTransportChannelWrapper::SetLocalCertificate(
const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
- if (dtls_state_ != STATE_NONE) {
+ if (dtls_active_) {
if (certificate == local_certificate_) {
// This may happen during renegotiation.
LOG_J(LS_INFO, this) << "Ignoring identical DTLS identity";
@@ -145,7 +142,7 @@ bool DtlsTransportChannelWrapper::SetLocalCertificate(
if (certificate) {
local_certificate_ = certificate;
- dtls_state_ = STATE_OFFERED;
+ dtls_active_ = true;
} else {
LOG_J(LS_INFO, this) << "NULL DTLS identity supplied. Not doing DTLS";
}
@@ -160,7 +157,7 @@ DtlsTransportChannelWrapper::GetLocalCertificate() const {
bool DtlsTransportChannelWrapper::SetSslMaxProtocolVersion(
rtc::SSLProtocolVersion version) {
- if (dtls_state_ != STATE_NONE) {
+ if (dtls_active_) {
LOG(LS_ERROR) << "Not changing max. protocol version "
<< "while DTLS is negotiating";
return false;
@@ -171,7 +168,7 @@ bool DtlsTransportChannelWrapper::SetSslMaxProtocolVersion(
}
bool DtlsTransportChannelWrapper::SetSslRole(rtc::SSLRole role) {
- if (dtls_state_ == STATE_OPEN) {
+ if (dtls_state() == DTLS_TRANSPORT_CONNECTED) {
if (ssl_role_ != role) {
LOG(LS_ERROR) << "SSL Role can't be reversed after the session is setup.";
return false;
@@ -189,7 +186,7 @@ bool DtlsTransportChannelWrapper::GetSslRole(rtc::SSLRole* role) const {
}
bool DtlsTransportChannelWrapper::GetSslCipherSuite(int* cipher) {
- if (dtls_state_ != STATE_OPEN) {
+ if (dtls_state() != DTLS_TRANSPORT_CONNECTED) {
return false;
}
@@ -202,8 +199,7 @@ bool DtlsTransportChannelWrapper::SetRemoteFingerprint(
size_t digest_len) {
rtc::Buffer remote_fingerprint_value(digest, digest_len);
- if (dtls_state_ != STATE_NONE &&
- remote_fingerprint_value_ == remote_fingerprint_value &&
+ if (dtls_active_ && remote_fingerprint_value_ == remote_fingerprint_value &&
!digest_alg.empty()) {
// This may happen during renegotiation.
LOG_J(LS_INFO, this) << "Ignoring identical remote DTLS fingerprint";
@@ -212,15 +208,14 @@ bool DtlsTransportChannelWrapper::SetRemoteFingerprint(
// Allow SetRemoteFingerprint with a NULL digest even if SetLocalCertificate
// hasn't been called.
- if (dtls_state_ > STATE_OFFERED ||
- (dtls_state_ == STATE_NONE && !digest_alg.empty())) {
+ if (dtls_ || (!dtls_active_ && !digest_alg.empty())) {
LOG_J(LS_ERROR, this) << "Can't set DTLS remote settings in this state.";
return false;
}
if (digest_alg.empty()) {
LOG_J(LS_INFO, this) << "Other side didn't support DTLS.";
- dtls_state_ = STATE_NONE;
+ dtls_active_ = false;
return true;
}
@@ -229,18 +224,18 @@ bool DtlsTransportChannelWrapper::SetRemoteFingerprint(
remote_fingerprint_algorithm_ = digest_alg;
if (!SetupDtls()) {
- dtls_state_ = STATE_CLOSED;
+ set_dtls_state(DTLS_TRANSPORT_FAILED);
return false;
}
- dtls_state_ = STATE_ACCEPTED;
return true;
}
bool DtlsTransportChannelWrapper::GetRemoteSSLCertificate(
rtc::SSLCertificate** cert) const {
- if (!dtls_)
+ if (!dtls_) {
return false;
+ }
return dtls_->GetPeerCertificate(cert);
}
@@ -277,7 +272,7 @@ bool DtlsTransportChannelWrapper::SetupDtls() {
return false;
}
} else {
- LOG_J(LS_INFO, this) << "Not using DTLS.";
+ LOG_J(LS_INFO, this) << "Not using DTLS-SRTP.";
}
LOG_J(LS_INFO, this) << "DTLS setup complete.";
@@ -286,15 +281,16 @@ bool DtlsTransportChannelWrapper::SetupDtls() {
bool DtlsTransportChannelWrapper::SetSrtpCiphers(
const std::vector<std::string>& ciphers) {
- if (srtp_ciphers_ == ciphers)
+ if (srtp_ciphers_ == ciphers) {
return true;
+ }
- if (dtls_state_ == STATE_STARTED) {
+ if (dtls_state() == DTLS_TRANSPORT_CONNECTING) {
LOG(LS_WARNING) << "Ignoring new SRTP ciphers while DTLS is negotiating";
return true;
}
- if (dtls_state_ == STATE_OPEN) {
+ if (dtls_state() == DTLS_TRANSPORT_CONNECTED) {
// We don't support DTLS renegotiation currently. If new set of srtp ciphers
// are different than what's being used currently, we will not use it.
// So for now, let's be happy (or sad) with a warning message.
@@ -320,10 +316,7 @@ bool DtlsTransportChannelWrapper::SetSrtpCiphers(
return true;
}
- if (dtls_state_ != STATE_NONE &&
- dtls_state_ != STATE_OFFERED &&
- dtls_state_ != STATE_ACCEPTED) {
- ASSERT(false);
+ if (!VERIFY(dtls_state() == DTLS_TRANSPORT_NEW)) {
return false;
}
@@ -332,7 +325,7 @@ bool DtlsTransportChannelWrapper::SetSrtpCiphers(
}
bool DtlsTransportChannelWrapper::GetSrtpCryptoSuite(std::string* cipher) {
- if (dtls_state_ != STATE_OPEN) {
+ if (dtls_state() != DTLS_TRANSPORT_CONNECTED) {
return false;
}
@@ -346,20 +339,23 @@ int DtlsTransportChannelWrapper::SendPacket(
const rtc::PacketOptions& options, int flags) {
int result = -1;
- switch (dtls_state_) {
- case STATE_OFFERED:
- // We don't know if we are doing DTLS yet, so we can't send a packet.
- // TODO(ekr@rtfm.com): assert here?
+ if (!dtls_active_) {
+ // Not doing DTLS.
+ return channel_->SendPacket(data, size, options);
+ }
+
+ switch (dtls_state()) {
+ case DTLS_TRANSPORT_NEW:
+ // Can't send data until the connection is active.
+ // TODO(ekr@rtfm.com): assert here if dtls_ is NULL?
result = -1;
pthatcher1 2015/10/22 07:05:00 Why not just "return -1"?
Taylor Brandstetter 2015/10/22 19:30:26 Done.
- break;
- case STATE_STARTED:
- case STATE_ACCEPTED:
- // Can't send data until the connection is active
+ case DTLS_TRANSPORT_CONNECTING:
+ // Can't send data until the connection is active.
result = -1;
break;
pthatcher1 2015/10/22 07:05:00 Why not just "return -1"?
Taylor Brandstetter 2015/10/22 19:30:26 Done.
- case STATE_OPEN:
+ case DTLS_TRANSPORT_CONNECTED:
if (flags & PF_SRTP_BYPASS) {
ASSERT(!srtp_ciphers_.empty());
if (!IsRtpPacket(data, size)) {
@@ -373,12 +369,10 @@ int DtlsTransportChannelWrapper::SendPacket(
rtc::SR_SUCCESS) ? static_cast<int>(size) : -1;
pthatcher1 2015/10/22 07:05:00 Why not just "return ..."?
Taylor Brandstetter 2015/10/22 19:30:26 Done.
}
break;
- // Not doing DTLS.
- case STATE_NONE:
- result = channel_->SendPacket(data, size, options);
- break;
- case STATE_CLOSED: // Can't send anything when we're closed.
+ case DTLS_TRANSPORT_FAILED:
+ case DTLS_TRANSPORT_CLOSED:
+ // Can't send anything when we're closed.
return -1;
}
@@ -402,19 +396,16 @@ void DtlsTransportChannelWrapper::OnWritableState(TransportChannel* channel) {
<< "DTLSTransportChannelWrapper: channel writable state changed to "
<< channel_->writable();
- switch (dtls_state_) {
- case STATE_NONE:
- case STATE_OPEN:
- set_writable(channel_->writable());
- // Note: SignalWritableState fired by set_writable.
- break;
-
- case STATE_OFFERED:
- // Do nothing
- break;
+ if (!dtls_active_) {
+ // Not doing DTLS.
+ // Note: SignalWritableState fired by set_writable.
+ set_writable(channel_->writable());
+ return;
+ }
- case STATE_ACCEPTED:
- if (!MaybeStartDtls()) {
+ switch (dtls_state()) {
+ case DTLS_TRANSPORT_NEW:
+ if (dtls_ && !MaybeStartDtls()) {
pthatcher1 2015/10/22 07:05:00 Why not just put if (!dtls_) { return true; }
Taylor Brandstetter 2015/10/22 19:30:26 Done. Though need to use VERIFY, since ASSERT is a
// This should never happen:
// Because we are operating in a nonblocking mode and all
// incoming packets come in via OnReadPacket(), which rejects
@@ -422,17 +413,20 @@ void DtlsTransportChannelWrapper::OnWritableState(TransportChannel* channel) {
// ignore write errors, thus any errors must be because of
// configuration and therefore are our fault.
// Note that in non-debug configurations, failure in
- // MaybeStartDtls() changes the state to STATE_CLOSED.
+ // MaybeStartDtls() changes the state to DTLS_TRANSPORT_FAILED.
ASSERT(false);
}
break;
-
- case STATE_STARTED:
- // Do nothing
+ case DTLS_TRANSPORT_CONNECTED:
+ // Note: SignalWritableState fired by set_writable.
+ set_writable(channel_->writable());
break;
-
- case STATE_CLOSED:
- // Should not happen. Do nothing
+ case DTLS_TRANSPORT_CONNECTING:
+ // Do nothing.
+ break;
+ case DTLS_TRANSPORT_FAILED:
+ case DTLS_TRANSPORT_CLOSED:
+ // Should not happen. Do nothing.
break;
}
}
@@ -443,7 +437,7 @@ void DtlsTransportChannelWrapper::OnReceivingState(TransportChannel* channel) {
LOG_J(LS_VERBOSE, this)
<< "DTLSTransportChannelWrapper: channel receiving state changed to "
<< channel_->receiving();
- if (dtls_state_ == STATE_NONE || dtls_state_ == STATE_OPEN) {
+ if (!dtls_active_ || dtls_state() == DTLS_TRANSPORT_CONNECTED) {
// Note: SignalReceivingState fired by set_receiving.
set_receiving(channel_->receiving());
}
@@ -456,28 +450,29 @@ void DtlsTransportChannelWrapper::OnReadPacket(
ASSERT(channel == channel_);
ASSERT(flags == 0);
- switch (dtls_state_) {
- case STATE_NONE:
- // We are not doing DTLS
- SignalReadPacket(this, data, size, packet_time, 0);
- break;
-
- case STATE_OFFERED:
- // Currently drop the packet, but we might in future
- // decide to take this as evidence that the other
- // side is ready to do DTLS and start the handshake
- // on our end
- LOG_J(LS_WARNING, this) << "Received packet before we know if we are "
- << "doing DTLS or not; dropping.";
- break;
+ if (!dtls_active_) {
+ // Not doing DTLS.
+ SignalReadPacket(this, data, size, packet_time, 0);
+ return;
+ }
- case STATE_ACCEPTED:
- // Drop packets received before DTLS has actually started
- LOG_J(LS_INFO, this) << "Dropping packet received before DTLS started.";
+ switch (dtls_state()) {
+ case DTLS_TRANSPORT_NEW:
+ if (dtls_) {
+ // Drop packets received before DTLS has actually started.
+ LOG_J(LS_INFO, this) << "Dropping packet received before DTLS started.";
+ } else {
+ // Currently drop the packet, but we might in future
+ // decide to take this as evidence that the other
+ // side is ready to do DTLS and start the handshake
+ // on our end.
+ LOG_J(LS_WARNING, this) << "Received packet before we know if we are "
+ << "doing DTLS or not; dropping.";
+ }
break;
- case STATE_STARTED:
- case STATE_OPEN:
+ case DTLS_TRANSPORT_CONNECTING:
+ case DTLS_TRANSPORT_CONNECTED:
// We should only get DTLS or SRTP packets; STUN's already been demuxed.
// Is this potentially a DTLS packet?
if (IsDtlsPacket(data, size)) {
@@ -487,7 +482,7 @@ void DtlsTransportChannelWrapper::OnReadPacket(
}
} else {
// Not a DTLS packet; our handshake should be complete by now.
- if (dtls_state_ != STATE_OPEN) {
+ if (dtls_state() != DTLS_TRANSPORT_CONNECTED) {
LOG_J(LS_ERROR, this) << "Received non-DTLS packet before DTLS "
<< "complete.";
return;
@@ -506,8 +501,9 @@ void DtlsTransportChannelWrapper::OnReadPacket(
SignalReadPacket(this, data, size, packet_time, PF_SRTP_BYPASS);
}
break;
- case STATE_CLOSED:
- // This shouldn't be happening. Drop the packet
+ case DTLS_TRANSPORT_FAILED:
+ case DTLS_TRANSPORT_CLOSED:
+ // This shouldn't be happening. Drop the packet.
break;
}
}
@@ -536,7 +532,7 @@ void DtlsTransportChannelWrapper::OnDtlsEvent(rtc::StreamInterface* dtls,
if (dtls_->GetState() == rtc::SS_OPEN) {
// The check for OPEN shouldn't be necessary but let's make
// sure we don't accidentally frob the state if it's closed.
- dtls_state_ = STATE_OPEN;
+ set_dtls_state(DTLS_TRANSPORT_CONNECTED);
set_writable(true);
}
}
@@ -549,13 +545,14 @@ void DtlsTransportChannelWrapper::OnDtlsEvent(rtc::StreamInterface* dtls,
}
if (sig & rtc::SE_CLOSE) {
ASSERT(sig == rtc::SE_CLOSE); // SE_CLOSE should be by itself.
+ set_writable(false);
if (!err) {
LOG_J(LS_INFO, this) << "DTLS channel closed";
+ set_dtls_state(DTLS_TRANSPORT_CLOSED);
} else {
LOG_J(LS_INFO, this) << "DTLS channel error, code=" << err;
+ set_dtls_state(DTLS_TRANSPORT_FAILED);
}
- set_writable(false);
- dtls_state_ = STATE_CLOSED;
}
}
@@ -563,13 +560,12 @@ bool DtlsTransportChannelWrapper::MaybeStartDtls() {
if (channel_->writable()) {
if (dtls_->StartSSLWithPeer()) {
LOG_J(LS_ERROR, this) << "Couldn't start DTLS handshake";
- dtls_state_ = STATE_CLOSED;
+ set_dtls_state(DTLS_TRANSPORT_FAILED);
return false;
}
LOG_J(LS_INFO, this)
<< "DtlsTransportChannelWrapper: Started DTLS handshake";
-
- dtls_state_ = STATE_STARTED;
+ set_dtls_state(DTLS_TRANSPORT_CONNECTING);
}
return true;
}
« no previous file with comments | « webrtc/p2p/base/dtlstransportchannel.h ('k') | webrtc/p2p/base/transport.h » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698