diff --git a/talk/base/common.cc b/talk/base/common.cc index 3c0c352a9..40755ddbd 100644 --- a/talk/base/common.cc +++ b/talk/base/common.cc @@ -28,7 +28,7 @@ #include #include #include -#include +#include #if WIN32 #define WIN32_LEAN_AND_MEAN diff --git a/talk/base/common.h b/talk/base/common.h index a76748b39..e8e89dac1 100644 --- a/talk/base/common.h +++ b/talk/base/common.h @@ -61,8 +61,14 @@ inline void Unused(const void*) {} #endif // UNUSED #ifndef WIN32 + +#ifndef strnicmp #define strnicmp(x, y, n) strncasecmp(x, y, n) +#endif + +#ifndef stricmp #define stricmp(x, y) strcasecmp(x, y) +#endif // TODO(fbarchard): Remove this. std::max should be used everywhere in the code. // NOMINMAX must be defined where we include . diff --git a/talk/base/helpers.cc b/talk/base/helpers.cc index b10a3f742..691a8131f 100644 --- a/talk/base/helpers.cc +++ b/talk/base/helpers.cc @@ -29,6 +29,7 @@ #include +#if defined(FEATURE_ENABLE_SSL) #include "talk/base/sslconfig.h" #if defined(SSL_USE_OPENSSL) #include @@ -40,7 +41,8 @@ #include #include #endif // WIN32 -#endif +#endif // else +#endif // FEATURE_ENABLED_SSL #include "talk/base/base64.h" #include "talk/base/basictypes.h" @@ -153,6 +155,28 @@ class SecureRandomGenerator : public RandomGenerator { RtlGenRandomProc rtl_gen_random_; }; +#elif !defined(FEATURE_ENABLE_SSL) + +// No SSL implementation -- use rand() +class SecureRandomGenerator : public RandomGenerator { + public: + virtual bool Init(const void* seed, size_t len) { + if (len >= 4) { + srand(*reinterpret_cast(seed)); + } else { + srand(*reinterpret_cast(seed)); + } + return true; + } + virtual bool Generate(void* buf, size_t len) { + char* bytes = reinterpret_cast(buf); + for (size_t i = 0; i < len; ++i) { + bytes[i] = static_cast(rand()); + } + return true; + } +}; + #else #error No SSL implementation has been selected! diff --git a/talk/base/ipaddress.cc b/talk/base/ipaddress.cc index 46725908d..b3585d785 100644 --- a/talk/base/ipaddress.cc +++ b/talk/base/ipaddress.cc @@ -32,7 +32,9 @@ #ifdef OPENBSD #include #endif +#ifndef __native_client__ #include +#endif #include #include #include diff --git a/talk/base/nethelpers.cc b/talk/base/nethelpers.cc index e6310ac45..e4d0c52d9 100644 --- a/talk/base/nethelpers.cc +++ b/talk/base/nethelpers.cc @@ -40,6 +40,11 @@ namespace talk_base { int ResolveHostname(const std::string& hostname, int family, std::vector* addresses) { +#ifdef __native_client__ + ASSERT(false); + LOG(LS_WARNING) << "ResolveHostname() is not implemented for NaCl"; + return -1; +#else // __native_client__ if (!addresses) { return -1; } @@ -64,6 +69,7 @@ int ResolveHostname(const std::string& hostname, int family, } freeaddrinfo(result); return 0; +#endif // !__native_client__ } // AsyncResolver diff --git a/talk/base/network.cc b/talk/base/network.cc index d4dda1381..00b04c9eb 100644 --- a/talk/base/network.cc +++ b/talk/base/network.cc @@ -38,7 +38,7 @@ #if defined(ANDROID) || defined(LINUX) #include #include -#else +#elif !defined(__native_client__) #include #endif #include @@ -46,11 +46,13 @@ #include #include #include + #ifdef ANDROID #include "talk/base/ifaddrs-android.h" -#else +#elif !defined(__native_client__) #include #endif + #endif // POSIX #ifdef WIN32 @@ -188,7 +190,16 @@ BasicNetworkManager::BasicNetworkManager() BasicNetworkManager::~BasicNetworkManager() { } -#if defined(POSIX) +#if defined(__native_client__) + +bool BasicNetworkManager::CreateNetworks(bool include_ignored, + NetworkList* networks) const { + ASSERT(false); + LOG(LS_WARNING) << "BasicNetworkManager doesn't work on NaCl yet"; + return false; +} + +#elif defined(POSIX) void BasicNetworkManager::ConvertIfAddrs(struct ifaddrs* interfaces, bool include_ignored, NetworkList* networks) const { diff --git a/talk/base/nssidentity.cc b/talk/base/nssidentity.cc index 053035e56..7bacb4446 100644 --- a/talk/base/nssidentity.cc +++ b/talk/base/nssidentity.cc @@ -51,6 +51,12 @@ namespace talk_base { +// Certificate validity lifetime in seconds. +static const int CERTIFICATE_LIFETIME = 60*60*24*30; // 30 days, arbitrarily +// Certificate validity window in seconds. +// This is to compensate for slightly incorrect system clocks. +static const int CERTIFICATE_WINDOW = -60*60*24; + NSSKeyPair::~NSSKeyPair() { if (privkey_) SECKEY_DestroyPrivateKey(privkey_); @@ -163,6 +169,47 @@ void NSSCertificate::ToDER(Buffer* der_buffer) const { der_buffer->SetData(certificate_->derCert.data, certificate_->derCert.len); } +static bool Certifies(CERTCertificate* parent, CERTCertificate* child) { + // TODO(bemasc): Identify stricter validation checks to use here. In the + // context of some future identity standard, it might make sense to check + // the certificates' roles, expiration dates, self-signatures (if + // self-signed), certificate transparency logging, or many other attributes. + // NOTE: Future changes to this validation may reject some previously allowed + // certificate chains. Users should be advised not to deploy chained + // certificates except in controlled environments until the validity + // requirements are finalized. + + // Check that the parent's name is the same as the child's claimed issuer. + SECComparison name_status = + CERT_CompareName(&child->issuer, &parent->subject); + if (name_status != SECEqual) + return false; + + // Extract the parent's public key, or fail if the key could not be read + // (e.g. certificate is corrupted). + SECKEYPublicKey* parent_key = CERT_ExtractPublicKey(parent); + if (!parent_key) + return false; + + // Check that the parent's privkey was actually used to generate the child's + // signature. + SECStatus verified = CERT_VerifySignedDataWithPublicKey( + &child->signatureWrap, parent_key, NULL); + SECKEY_DestroyPublicKey(parent_key); + return verified == SECSuccess; +} + +bool NSSCertificate::IsValidChain(const CERTCertList* cert_list) { + CERTCertListNode* child = CERT_LIST_HEAD(cert_list); + for (CERTCertListNode* parent = CERT_LIST_NEXT(child); + !CERT_LIST_END(parent, cert_list); + child = parent, parent = CERT_LIST_NEXT(parent)) { + if (!Certifies(parent->cert, child->cert)) + return false; + } + return true; +} + bool NSSCertificate::GetDigestLength(const std::string &algorithm, std::size_t *length) { const SECHashObject *ho; @@ -299,8 +346,8 @@ bool NSSCertificate::GetDigestObject(const std::string &algorithm, } -NSSIdentity *NSSIdentity::Generate(const std::string &common_name) { - std::string subject_name_string = "CN=" + common_name; +NSSIdentity* NSSIdentity::GenerateInternal(const SSLIdentityParams& params) { + std::string subject_name_string = "CN=" + params.common_name; CERTName *subject_name = CERT_AsciiToName( const_cast(subject_name_string.c_str())); NSSIdentity *identity = NULL; @@ -313,9 +360,11 @@ NSSIdentity *NSSIdentity::Generate(const std::string &common_name) { SECStatus rv; PLArenaPool* arena; SECItem signed_cert; - PRTime not_before, not_after; PRTime now = PR_Now(); - PRTime one_day; + PRTime not_before = + now + static_cast(params.not_before) * PR_USEC_PER_SEC; + PRTime not_after = + now + static_cast(params.not_after) * PR_USEC_PER_SEC; inner_der.len = 0; inner_der.data = NULL; @@ -342,11 +391,6 @@ NSSIdentity *NSSIdentity::Generate(const std::string &common_name) { goto fail; } - one_day = 86400; - one_day *= PR_USEC_PER_SEC; - not_before = now - one_day; - not_after = now + 30 * one_day; - validity = CERT_CreateValidity(not_before, not_after); if (!validity) { LOG(LS_ERROR) << "Couldn't create validity"; @@ -408,6 +452,18 @@ NSSIdentity *NSSIdentity::Generate(const std::string &common_name) { return identity; } +NSSIdentity* NSSIdentity::Generate(const std::string &common_name) { + SSLIdentityParams params; + params.common_name = common_name; + params.not_before = CERTIFICATE_WINDOW; + params.not_after = CERTIFICATE_LIFETIME; + return GenerateInternal(params); +} + +NSSIdentity* NSSIdentity::GenerateForTest(const SSLIdentityParams& params) { + return GenerateInternal(params); +} + SSLIdentity* NSSIdentity::FromPEMStrings(const std::string& private_key, const std::string& certificate) { std::string private_key_der; diff --git a/talk/base/nssidentity.h b/talk/base/nssidentity.h index 3f97ebbb6..7bf7c6519 100644 --- a/talk/base/nssidentity.h +++ b/talk/base/nssidentity.h @@ -91,6 +91,11 @@ class NSSCertificate : public SSLCertificate { CERTCertificate* certificate() { return certificate_; } + // Performs minimal checks to determine if the list is a valid chain. This + // only checks that each certificate certifies the preceding certificate, + // and ignores many other certificate features such as expiration dates. + static bool IsValidChain(const CERTCertList* cert_list); + // Helper function to get the length of a digest static bool GetDigestLength(const std::string& algorithm, std::size_t* length); @@ -113,6 +118,7 @@ class NSSCertificate : public SSLCertificate { class NSSIdentity : public SSLIdentity { public: static NSSIdentity* Generate(const std::string& common_name); + static NSSIdentity* GenerateForTest(const SSLIdentityParams& params); static SSLIdentity* FromPEMStrings(const std::string& private_key, const std::string& certificate); virtual ~NSSIdentity() { @@ -128,6 +134,8 @@ class NSSIdentity : public SSLIdentity { NSSIdentity(NSSKeyPair* keypair, NSSCertificate* cert) : keypair_(keypair), certificate_(cert) {} + static NSSIdentity* GenerateInternal(const SSLIdentityParams& params); + talk_base::scoped_ptr keypair_; talk_base::scoped_ptr certificate_; diff --git a/talk/base/nssstreamadapter.cc b/talk/base/nssstreamadapter.cc index 185c243f5..acddb672b 100644 --- a/talk/base/nssstreamadapter.cc +++ b/talk/base/nssstreamadapter.cc @@ -781,11 +781,26 @@ SECStatus NSSStreamAdapter::AuthCertificateHook(void *arg, PRBool isServer) { LOG(LS_INFO) << "NSSStreamAdapter::AuthCertificateHook"; NSSCertificate peer_cert(SSL_PeerCertificate(fd)); - bool ok = false; - - // TODO(ekr@rtfm.com): Should we be enforcing self-signed like - // the OpenSSL version? NSSStreamAdapter *stream = reinterpret_cast(arg); + stream->cert_ok_ = false; + + // Read the peer's certificate chain. + CERTCertList* cert_list = SSL_PeerCertificateChain(fd); + ASSERT(cert_list != NULL); + + // If the peer provided multiple certificates, check that they form a valid + // chain as defined by RFC 5246 Section 7.4.2: "Each following certificate + // MUST directly certify the one preceding it.". This check does NOT + // verify other requirements, such as whether the chain reaches a trusted + // root, self-signed certificates have valid signatures, certificates are not + // expired, etc. + // Even if the chain is valid, the leaf certificate must still match a + // provided certificate or digest. + if (!NSSCertificate::IsValidChain(cert_list)) { + CERT_DestroyCertList(cert_list); + PORT_SetError(SEC_ERROR_BAD_SIGNATURE); + return SECFailure; + } if (stream->peer_certificate_.get()) { LOG(LS_INFO) << "Checking against specified certificate"; @@ -794,7 +809,7 @@ SECStatus NSSStreamAdapter::AuthCertificateHook(void *arg, if (reinterpret_cast(stream->peer_certificate_.get())-> Equals(&peer_cert)) { LOG(LS_INFO) << "Accepted peer certificate"; - ok = true; + stream->cert_ok_ = true; } } else if (!stream->peer_certificate_digest_algorithm_.empty()) { LOG(LS_INFO) << "Checking against specified digest"; @@ -810,7 +825,7 @@ SECStatus NSSStreamAdapter::AuthCertificateHook(void *arg, Buffer computed_digest(digest, digest_length); if (computed_digest == stream->peer_certificate_digest_value_) { LOG(LS_INFO) << "Accepted peer certificate"; - ok = true; + stream->cert_ok_ = true; } } } else { @@ -819,24 +834,19 @@ SECStatus NSSStreamAdapter::AuthCertificateHook(void *arg, UNIMPLEMENTED; } - if (ok) { - stream->cert_ok_ = true; - - // Record the peer's certificate chain. - CERTCertList* cert_list = SSL_PeerCertificateChain(fd); - ASSERT(cert_list != NULL); - - stream->peer_certificate_.reset(new NSSCertificate(cert_list)); - CERT_DestroyCertList(cert_list); - return SECSuccess; - } - - if (!ok && stream->ignore_bad_cert()) { + if (!stream->cert_ok_ && stream->ignore_bad_cert()) { LOG(LS_WARNING) << "Ignoring cert error while verifying cert chain"; stream->cert_ok_ = true; - return SECSuccess; } + if (stream->cert_ok_) + stream->peer_certificate_.reset(new NSSCertificate(cert_list)); + + CERT_DestroyCertList(cert_list); + + if (stream->cert_ok_) + return SECSuccess; + PORT_SetError(SEC_ERROR_UNTRUSTED_CERT); return SECFailure; } diff --git a/talk/base/openssladapter.cc b/talk/base/openssladapter.cc index af92f0c45..95d5a1a34 100644 --- a/talk/base/openssladapter.cc +++ b/talk/base/openssladapter.cc @@ -62,9 +62,7 @@ #define MUTEX_LOCK(x) WaitForSingleObject((x), INFINITE) #define MUTEX_UNLOCK(x) ReleaseMutex(x) #define THREAD_ID GetCurrentThreadId() -#elif defined(_POSIX_THREADS) - // _POSIX_THREADS is normally defined in unistd.h if pthreads are available - // on your platform. +#elif defined(POSIX) #define MUTEX_TYPE pthread_mutex_t #define MUTEX_SETUP(x) pthread_mutex_init(&(x), NULL) #define MUTEX_CLEANUP(x) pthread_mutex_destroy(&(x)) diff --git a/talk/base/opensslidentity.cc b/talk/base/opensslidentity.cc index 4ff760161..eef066585 100644 --- a/talk/base/opensslidentity.cc +++ b/talk/base/opensslidentity.cc @@ -57,7 +57,7 @@ static const int KEY_LENGTH = 1024; static const int SERIAL_RAND_BITS = 64; // Certificate validity lifetime -static const int CERTIFICATE_LIFETIME = 60*60*24*365; // one year, arbitrarily +static const int CERTIFICATE_LIFETIME = 60*60*24*30; // 30 days, arbitrarily // Certificate validity window. // This is to compensate for slightly incorrect system clocks. static const int CERTIFICATE_WINDOW = -60*60*24; @@ -96,8 +96,8 @@ static EVP_PKEY* MakeKey() { // Generate a self-signed certificate, with the public key from the // given key pair. Caller is responsible for freeing the returned object. -static X509* MakeCertificate(EVP_PKEY* pkey, const char* common_name) { - LOG(LS_INFO) << "Making certificate for " << common_name; +static X509* MakeCertificate(EVP_PKEY* pkey, const SSLIdentityParams& params) { + LOG(LS_INFO) << "Making certificate for " << params.common_name; X509* x509 = NULL; BIGNUM* serial_number = NULL; X509_NAME* name = NULL; @@ -128,14 +128,15 @@ static X509* MakeCertificate(EVP_PKEY* pkey, const char* common_name) { // clear during SSL negotiation, so there may be a privacy issue in // putting anything recognizable here. if ((name = X509_NAME_new()) == NULL || - !X509_NAME_add_entry_by_NID(name, NID_commonName, MBSTRING_UTF8, - (unsigned char*)common_name, -1, -1, 0) || + !X509_NAME_add_entry_by_NID( + name, NID_commonName, MBSTRING_UTF8, + (unsigned char*)params.common_name.c_str(), -1, -1, 0) || !X509_set_subject_name(x509, name) || !X509_set_issuer_name(x509, name)) goto error; - if (!X509_gmtime_adj(X509_get_notBefore(x509), CERTIFICATE_WINDOW) || - !X509_gmtime_adj(X509_get_notAfter(x509), CERTIFICATE_LIFETIME)) + if (!X509_gmtime_adj(X509_get_notBefore(x509), params.not_before) || + !X509_gmtime_adj(X509_get_notAfter(x509), params.not_after)) goto error; if (!X509_sign(x509, pkey, EVP_sha1())) @@ -199,12 +200,13 @@ static void PrintCert(X509* x509) { #endif OpenSSLCertificate* OpenSSLCertificate::Generate( - OpenSSLKeyPair* key_pair, const std::string& common_name) { - std::string actual_common_name = common_name; - if (actual_common_name.empty()) + OpenSSLKeyPair* key_pair, const SSLIdentityParams& params) { + SSLIdentityParams actual_params(params); + if (actual_params.common_name.empty()) { // Use a random string, arbitrarily 8chars long. - actual_common_name = CreateRandomString(8); - X509* x509 = MakeCertificate(key_pair->pkey(), actual_common_name.c_str()); + actual_params.common_name = CreateRandomString(8); + } + X509* x509 = MakeCertificate(key_pair->pkey(), actual_params); if (!x509) { LogSSLErrors("Generating certificate"); return NULL; @@ -320,11 +322,12 @@ void OpenSSLCertificate::AddReference() const { CRYPTO_add(&x509_->references, 1, CRYPTO_LOCK_X509); } -OpenSSLIdentity* OpenSSLIdentity::Generate(const std::string& common_name) { +OpenSSLIdentity* OpenSSLIdentity::GenerateInternal( + const SSLIdentityParams& params) { OpenSSLKeyPair *key_pair = OpenSSLKeyPair::Generate(); if (key_pair) { - OpenSSLCertificate *certificate = - OpenSSLCertificate::Generate(key_pair, common_name); + OpenSSLCertificate *certificate = OpenSSLCertificate::Generate( + key_pair, params); if (certificate) return new OpenSSLIdentity(key_pair, certificate); delete key_pair; @@ -333,6 +336,19 @@ OpenSSLIdentity* OpenSSLIdentity::Generate(const std::string& common_name) { return NULL; } +OpenSSLIdentity* OpenSSLIdentity::Generate(const std::string& common_name) { + SSLIdentityParams params; + params.common_name = common_name; + params.not_before = CERTIFICATE_WINDOW; + params.not_after = CERTIFICATE_LIFETIME; + return GenerateInternal(params); +} + +OpenSSLIdentity* OpenSSLIdentity::GenerateForTest( + const SSLIdentityParams& params) { + return GenerateInternal(params); +} + SSLIdentity* OpenSSLIdentity::FromPEMStrings( const std::string& private_key, const std::string& certificate) { diff --git a/talk/base/opensslidentity.h b/talk/base/opensslidentity.h index af18c5c4d..ede731855 100644 --- a/talk/base/opensslidentity.h +++ b/talk/base/opensslidentity.h @@ -78,7 +78,7 @@ class OpenSSLCertificate : public SSLCertificate { } static OpenSSLCertificate* Generate(OpenSSLKeyPair* key_pair, - const std::string& common_name); + const SSLIdentityParams& params); static OpenSSLCertificate* FromPEMString(const std::string& pem_string); virtual ~OpenSSLCertificate(); @@ -127,6 +127,7 @@ class OpenSSLCertificate : public SSLCertificate { class OpenSSLIdentity : public SSLIdentity { public: static OpenSSLIdentity* Generate(const std::string& common_name); + static OpenSSLIdentity* GenerateForTest(const SSLIdentityParams& params); static SSLIdentity* FromPEMStrings(const std::string& private_key, const std::string& certificate); virtual ~OpenSSLIdentity() { } @@ -151,6 +152,8 @@ class OpenSSLIdentity : public SSLIdentity { ASSERT(certificate != NULL); } + static OpenSSLIdentity* GenerateInternal(const SSLIdentityParams& params); + scoped_ptr key_pair_; scoped_ptr certificate_; diff --git a/talk/base/opensslstreamadapter.cc b/talk/base/opensslstreamadapter.cc index 034dfcf92..576b42452 100644 --- a/talk/base/opensslstreamadapter.cc +++ b/talk/base/opensslstreamadapter.cc @@ -210,13 +210,6 @@ void OpenSSLStreamAdapter::SetServerRole(SSLRole role) { role_ = role; } -void OpenSSLStreamAdapter::SetPeerCertificate(SSLCertificate* cert) { - ASSERT(!peer_certificate_); - ASSERT(peer_certificate_digest_algorithm_.empty()); - ASSERT(ssl_server_name_.empty()); - peer_certificate_.reset(static_cast(cert)); -} - bool OpenSSLStreamAdapter::GetPeerCertificate(SSLCertificate** cert) const { if (!peer_certificate_) return false; @@ -613,7 +606,6 @@ int OpenSSLStreamAdapter::BeginSSL() { // The underlying stream has open. If we are in peer-to-peer mode // then a peer certificate must have been specified by now. ASSERT(!ssl_server_name_.empty() || - peer_certificate_ || !peer_certificate_digest_algorithm_.empty()); LOG(LS_INFO) << "BeginSSL: " << (!ssl_server_name_.empty() ? ssl_server_name_ : @@ -661,9 +653,7 @@ int OpenSSLStreamAdapter::ContinueSSL() { case SSL_ERROR_NONE: LOG(LS_VERBOSE) << " -- success"; - if (!SSLPostConnectionCheck(ssl_, ssl_server_name_.c_str(), - peer_certificate_ ? - peer_certificate_->x509() : NULL, + if (!SSLPostConnectionCheck(ssl_, ssl_server_name_.c_str(), NULL, peer_certificate_digest_algorithm_)) { LOG(LS_ERROR) << "TLS post connection check failed"; return -1; @@ -772,18 +762,6 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() { return NULL; } - if (!peer_certificate_) { // traditional mode - // Add the root cert to the SSL context - if (!OpenSSLAdapter::ConfigureTrustedRootCertificates(ctx)) { - SSL_CTX_free(ctx); - return NULL; - } - } - - if (peer_certificate_ && role_ == SSL_SERVER) - // we must specify which client cert to ask for - SSL_CTX_add_client_CA(ctx, peer_certificate_->x509()); - #ifdef _DEBUG SSL_CTX_set_info_callback(ctx, OpenSSLAdapter::SSLInfoCallback); #endif @@ -806,88 +784,39 @@ SSL_CTX* OpenSSLStreamAdapter::SetupSSLContext() { } int OpenSSLStreamAdapter::SSLVerifyCallback(int ok, X509_STORE_CTX* store) { -#if _DEBUG - if (!ok) { - char data[256]; - X509* cert = X509_STORE_CTX_get_current_cert(store); - int depth = X509_STORE_CTX_get_error_depth(store); - int err = X509_STORE_CTX_get_error(store); - - LOG(LS_INFO) << "Error with certificate at depth: " << depth; - X509_NAME_oneline(X509_get_issuer_name(cert), data, sizeof(data)); - LOG(LS_INFO) << " issuer = " << data; - X509_NAME_oneline(X509_get_subject_name(cert), data, sizeof(data)); - LOG(LS_INFO) << " subject = " << data; - LOG(LS_INFO) << " err = " << err - << ":" << X509_verify_cert_error_string(err); - } -#endif - // Get our SSL structure from the store SSL* ssl = reinterpret_cast(X509_STORE_CTX_get_ex_data( store, SSL_get_ex_data_X509_STORE_CTX_idx())); - OpenSSLStreamAdapter* stream = reinterpret_cast(SSL_get_app_data(ssl)); - // In peer-to-peer mode, no root cert / certificate authority was - // specified, so the libraries knows of no certificate to accept, - // and therefore it will necessarily call here on the first cert it - // tries to verify. - if (!ok && stream->peer_certificate_) { - X509* cert = X509_STORE_CTX_get_current_cert(store); - int err = X509_STORE_CTX_get_error(store); - // peer-to-peer mode: allow the certificate to be self-signed, - // assuming it matches the cert that was specified. - if (err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT && - X509_cmp(cert, stream->peer_certificate_->x509()) == 0) { - LOG(LS_INFO) << "Accepted self-signed peer certificate authority"; - ok = 1; - } - } else if (!ok && !stream->peer_certificate_digest_algorithm_.empty()) { - X509* cert = X509_STORE_CTX_get_current_cert(store); - int err = X509_STORE_CTX_get_error(store); - - // peer-to-peer mode: allow the certificate to be self-signed, - // assuming it matches the digest that was specified. - if (err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) { - unsigned char digest[EVP_MAX_MD_SIZE]; - std::size_t digest_length; - - if (OpenSSLCertificate:: - ComputeDigest(cert, - stream->peer_certificate_digest_algorithm_, - digest, sizeof(digest), - &digest_length)) { - Buffer computed_digest(digest, digest_length); - if (computed_digest == stream->peer_certificate_digest_value_) { - LOG(LS_INFO) << - "Accepted self-signed peer certificate authority"; - ok = 1; - - // Record the peer's certificate. - stream->peer_certificate_.reset(new OpenSSLCertificate(cert)); - } - } - } - } else if (!ok && OpenSSLAdapter::custom_verify_callback_) { - // this applies only in traditional mode - void* cert = - reinterpret_cast(X509_STORE_CTX_get_current_cert(store)); - if (OpenSSLAdapter::custom_verify_callback_(cert)) { - stream->custom_verification_succeeded_ = true; - LOG(LS_INFO) << "validated certificate using custom callback"; - ok = 1; - } + if (stream->peer_certificate_digest_algorithm_.empty()) { + return 0; } - - if (!ok && stream->ignore_bad_cert()) { - LOG(LS_WARNING) << "Ignoring cert error while verifying cert chain"; - ok = 1; + X509* cert = X509_STORE_CTX_get_current_cert(store); + unsigned char digest[EVP_MAX_MD_SIZE]; + std::size_t digest_length; + if (!OpenSSLCertificate::ComputeDigest( + cert, + stream->peer_certificate_digest_algorithm_, + digest, sizeof(digest), + &digest_length)) { + LOG(LS_WARNING) << "Failed to compute peer cert digest."; + return 0; } - - return ok; + Buffer computed_digest(digest, digest_length); + if (computed_digest != stream->peer_certificate_digest_value_) { + LOG(LS_WARNING) << "Rejected peer certificate due to mismatched digest."; + return 0; + } + // Ignore any verification error if the digest matches, since there is no + // value in checking the validity of a self-signed cert issued by untrusted + // sources. + LOG(LS_INFO) << "Accepted peer certificate."; + // Record the peer's certificate. + stream->peer_certificate_.reset(new OpenSSLCertificate(cert)); + return 1; } // This code is taken from the "Network Security with OpenSSL" diff --git a/talk/base/opensslstreamadapter.h b/talk/base/opensslstreamadapter.h index 3c478187f..744d29980 100644 --- a/talk/base/opensslstreamadapter.h +++ b/talk/base/opensslstreamadapter.h @@ -2,26 +2,26 @@ * libjingle * Copyright 2004--2008, Google Inc. * - * Redistribution and use in source and binary forms, with or without + * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: * - * 1. Redistributions of source code must retain the above copyright notice, + * 1. Redistributions of source code must retain the above copyright notice, * this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright notice, * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. - * 3. The name of the author may not be used to endorse or promote products + * 3. The name of the author may not be used to endorse or promote products * derived from this software without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED - * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO - * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, - * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR - * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ @@ -81,7 +81,6 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter { // Default argument is for compatibility virtual void SetServerRole(SSLRole role = SSL_SERVER); - virtual void SetPeerCertificate(SSLCertificate* cert); virtual bool SetPeerCertificateDigest(const std::string& digest_alg, const unsigned char* digest_val, size_t digest_len); @@ -175,7 +174,6 @@ class OpenSSLStreamAdapter : public SSLStreamAdapter { // passed. static int SSLVerifyCallback(int ok, X509_STORE_CTX* store); - SSLState state_; SSLRole role_; int ssl_error_code_; // valid when state_ == SSL_ERROR or SSL_CLOSED diff --git a/talk/base/physicalsocketserver.cc b/talk/base/physicalsocketserver.cc index 43be440e7..d4a4b1af7 100644 --- a/talk/base/physicalsocketserver.cc +++ b/talk/base/physicalsocketserver.cc @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #endif @@ -517,7 +518,7 @@ class PhysicalSocket : public AsyncSocket, public sigslot::has_slots<> { *slevel = IPPROTO_IP; *sopt = IP_DONTFRAGMENT; break; -#elif defined(IOS) || defined(OSX) || defined(BSD) +#elif defined(IOS) || defined(OSX) || defined(BSD) || defined(__native_client__) LOG(LS_WARNING) << "Socket::OPT_DONTFRAGMENT not supported."; return -1; #elif defined(POSIX) @@ -1489,10 +1490,14 @@ bool PhysicalSocketServer::InstallSignal(int signum, void (*handler)(int)) { return false; } act.sa_handler = handler; +#if !defined(__native_client__) // Use SA_RESTART so that our syscalls don't get EINTR, since we don't need it // and it's a nuisance. Though some syscalls still return EINTR and there's no // real standard for which ones. :( act.sa_flags = SA_RESTART; +#else + act.sa_flags = 0; +#endif if (sigaction(signum, &act, NULL) != 0) { LOG_ERR(LS_ERROR) << "Couldn't set sigaction"; return false; diff --git a/talk/base/socketaddress.cc b/talk/base/socketaddress.cc index 193a23282..792d414ad 100644 --- a/talk/base/socketaddress.cc +++ b/talk/base/socketaddress.cc @@ -34,7 +34,9 @@ #if defined(OPENBSD) #include #endif +#if !defined(__native_client__) #include +#endif #include #include #include diff --git a/talk/base/sslidentity.cc b/talk/base/sslidentity.cc index 8f704dc30..d2d2b11f1 100644 --- a/talk/base/sslidentity.cc +++ b/talk/base/sslidentity.cc @@ -115,6 +115,10 @@ SSLIdentity* SSLIdentity::Generate(const std::string& common_name) { return NULL; } +SSLIdentity* GenerateForTest(const SSLIdentityParams& params) { + return NULL; +} + SSLIdentity* SSLIdentity::FromPEMStrings(const std::string& private_key, const std::string& certificate) { return NULL; @@ -130,6 +134,10 @@ SSLIdentity* SSLIdentity::Generate(const std::string& common_name) { return OpenSSLIdentity::Generate(common_name); } +SSLIdentity* SSLIdentity::GenerateForTest(const SSLIdentityParams& params) { + return OpenSSLIdentity::GenerateForTest(params); +} + SSLIdentity* SSLIdentity::FromPEMStrings(const std::string& private_key, const std::string& certificate) { return OpenSSLIdentity::FromPEMStrings(private_key, certificate); @@ -145,6 +153,10 @@ SSLIdentity* SSLIdentity::Generate(const std::string& common_name) { return NSSIdentity::Generate(common_name); } +SSLIdentity* SSLIdentity::GenerateForTest(const SSLIdentityParams& params) { + return NSSIdentity::GenerateForTest(params); +} + SSLIdentity* SSLIdentity::FromPEMStrings(const std::string& private_key, const std::string& certificate) { return NSSIdentity::FromPEMStrings(private_key, certificate); diff --git a/talk/base/sslidentity.h b/talk/base/sslidentity.h index 89b100898..00ec53349 100644 --- a/talk/base/sslidentity.h +++ b/talk/base/sslidentity.h @@ -132,6 +132,16 @@ class SSLCertChain { DISALLOW_COPY_AND_ASSIGN(SSLCertChain); }; +// Parameters for generating an identity for testing. If common_name is +// non-empty, it will be used for the certificate's subject and issuer name, +// otherwise a random string will be used. |not_before| and |not_after| are +// offsets to the current time in number of seconds. +struct SSLIdentityParams { + std::string common_name; + int not_before; // in seconds. + int not_after; // in seconds. +}; + // Our identity in an SSL negotiation: a keypair and certificate (both // with the same public key). // This too is pretty much immutable once created. @@ -144,6 +154,9 @@ class SSLIdentity { // Caller is responsible for freeing the returned object. static SSLIdentity* Generate(const std::string& common_name); + // Generates an identity with the specified validity period. + static SSLIdentity* GenerateForTest(const SSLIdentityParams& params); + // Construct an identity from a private key and a certificate. static SSLIdentity* FromPEMStrings(const std::string& private_key, const std::string& certificate); diff --git a/talk/base/sslstreamadapter.h b/talk/base/sslstreamadapter.h index 3a7797370..1811f9566 100644 --- a/talk/base/sslstreamadapter.h +++ b/talk/base/sslstreamadapter.h @@ -2,26 +2,26 @@ * libjingle * Copyright 2004--2008, Google Inc. * - * Redistribution and use in source and binary forms, with or without + * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are met: * - * 1. Redistributions of source code must retain the above copyright notice, + * 1. Redistributions of source code must retain the above copyright notice, * this list of conditions and the following disclaimer. * 2. Redistributions in binary form must reproduce the above copyright notice, * this list of conditions and the following disclaimer in the documentation * and/or other materials provided with the distribution. - * 3. The name of the author may not be used to endorse or promote products + * 3. The name of the author may not be used to endorse or promote products * derived from this software without specific prior written permission. * * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR IMPLIED - * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO - * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; * OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, - * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR - * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR + * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ @@ -116,17 +116,6 @@ class SSLStreamAdapter : public StreamAdapterInterface { // underlying stream opens. virtual int StartSSLWithPeer() = 0; - // Specify the certificate that our peer is expected to use in - // peer-to-peer mode. Only this certificate will be accepted during - // SSL verification. The certificate is assumed to have been - // obtained through some other secure channel (such as the XMPP - // channel). (This could also specify the certificate authority that - // will sign the peer's certificate.) - // SSLStream takes ownership of the SSLCertificate object and will - // free it when appropriate. Should be called no more than once on a - // given SSLStream instance. - virtual void SetPeerCertificate(SSLCertificate* cert) = 0; - // Specify the digest of the certificate that our peer is expected to use in // peer-to-peer mode. Only this certificate will be accepted during // SSL verification. The certificate is assumed to have been @@ -138,11 +127,9 @@ class SSLStreamAdapter : public StreamAdapterInterface { const unsigned char* digest_val, size_t digest_len) = 0; - // Retrieves the peer's X.509 certificate, if a certificate has been - // provided by SetPeerCertificate or a connection has been established. If - // a connection has been established, this returns the - // certificate transmitted over SSL, including the entire chain. - // The returned certificate is owned by the caller. + // Retrieves the peer's X.509 certificate, if a connection has been + // established. It returns the transmitted over SSL, including the entire + // chain. The returned certificate is owned by the caller. virtual bool GetPeerCertificate(SSLCertificate** cert) const = 0; // Key Exporter interface from RFC 5705 diff --git a/talk/base/sslstreamadapter_unittest.cc b/talk/base/sslstreamadapter_unittest.cc index 4b2fd6d84..5cc4bfda7 100644 --- a/talk/base/sslstreamadapter_unittest.cc +++ b/talk/base/sslstreamadapter_unittest.cc @@ -215,6 +215,37 @@ class SSLStreamAdapterTestBase : public testing::Test, talk_base::InitializeSSL(); } + // Recreate the client/server identities with the specified validity period. + // |not_before| and |not_after| are offsets from the current time in number + // of seconds. + void ResetIdentitiesWithValidity(int not_before, int not_after) { + client_stream_ = + new SSLDummyStream(this, "c2s", &client_buffer_, &server_buffer_); + server_stream_ = + new SSLDummyStream(this, "s2c", &server_buffer_, &client_buffer_); + + client_ssl_.reset(talk_base::SSLStreamAdapter::Create(client_stream_)); + server_ssl_.reset(talk_base::SSLStreamAdapter::Create(server_stream_)); + + client_ssl_->SignalEvent.connect(this, &SSLStreamAdapterTestBase::OnEvent); + server_ssl_->SignalEvent.connect(this, &SSLStreamAdapterTestBase::OnEvent); + + talk_base::SSLIdentityParams client_params; + client_params.common_name = "client"; + client_params.not_before = not_before; + client_params.not_after = not_after; + client_identity_ = talk_base::SSLIdentity::GenerateForTest(client_params); + + talk_base::SSLIdentityParams server_params; + server_params.common_name = "server"; + server_params.not_before = not_before; + server_params.not_after = not_after; + server_identity_ = talk_base::SSLIdentity::GenerateForTest(server_params); + + client_ssl_->SetIdentity(client_identity_); + server_ssl_->SetIdentity(server_identity_); + } + virtual void OnEvent(talk_base::StreamInterface *stream, int sig, int err) { LOG(LS_INFO) << "SSLStreamAdapterTestBase::OnEvent sig=" << sig; @@ -227,24 +258,6 @@ class SSLStreamAdapterTestBase : public testing::Test, } } - void SetPeerIdentitiesByCertificate(bool correct) { - LOG(LS_INFO) << "Setting peer identities by certificate"; - - if (correct) { - client_ssl_->SetPeerCertificate(server_identity_->certificate(). - GetReference()); - server_ssl_->SetPeerCertificate(client_identity_->certificate(). - GetReference()); - } else { - // If incorrect, set up to expect our own certificate at the peer - client_ssl_->SetPeerCertificate(client_identity_->certificate(). - GetReference()); - server_ssl_->SetPeerCertificate(server_identity_->certificate(). - GetReference()); - } - identities_set_ = true; - } - void SetPeerIdentitiesByDigest(bool correct) { unsigned char digest[20]; size_t digest_len; @@ -253,8 +266,8 @@ class SSLStreamAdapterTestBase : public testing::Test, LOG(LS_INFO) << "Setting peer identities by digest"; rv = server_identity_->certificate().ComputeDigest(talk_base::DIGEST_SHA_1, - digest, 20, - &digest_len); + digest, 20, + &digest_len); ASSERT_TRUE(rv); if (!correct) { LOG(LS_INFO) << "Setting bogus digest for server cert"; @@ -266,7 +279,7 @@ class SSLStreamAdapterTestBase : public testing::Test, rv = client_identity_->certificate().ComputeDigest(talk_base::DIGEST_SHA_1, - digest, 20, &digest_len); + digest, 20, &digest_len); ASSERT_TRUE(rv); if (!correct) { LOG(LS_INFO) << "Setting bogus digest for client cert"; @@ -722,17 +735,6 @@ TEST_F(SSLStreamAdapterTestTLS, TestTLSBogusDigest) { TestHandshake(false); }; -// Test a handshake with a peer certificate -TEST_F(SSLStreamAdapterTestTLS, TestTLSPeerCertificate) { - SetPeerIdentitiesByCertificate(true); - TestHandshake(); -}; - -// Test a handshake with a bogus peer certificate -TEST_F(SSLStreamAdapterTestTLS, TestTLSBogusPeerCertificate) { - SetPeerIdentitiesByCertificate(false); - TestHandshake(false); -}; // Test moving a bunch of data // Basic tests: DTLS @@ -887,6 +889,24 @@ TEST_F(SSLStreamAdapterTestDTLS, TestDTLSExporter) { ASSERT_TRUE(!memcmp(client_out, server_out, sizeof(client_out))); } +// Test not yet valid certificates are not rejected. +TEST_F(SSLStreamAdapterTestDTLS, TestCertNotYetValid) { + MAYBE_SKIP_TEST(HaveDtls); + long one_day = 60 * 60 * 24; + // Make the certificates not valid until one day later. + ResetIdentitiesWithValidity(one_day, one_day); + TestHandshake(); +} + +// Test expired certificates are not rejected. +TEST_F(SSLStreamAdapterTestDTLS, TestCertExpired) { + MAYBE_SKIP_TEST(HaveDtls); + long one_day = 60 * 60 * 24; + // Make the certificates already expired. + ResetIdentitiesWithValidity(-one_day, -one_day); + TestHandshake(); +} + // Test data transfer using certs created from strings. TEST_F(SSLStreamAdapterTestDTLSFromPEMStrings, TestTransfer) { MAYBE_SKIP_TEST(HaveDtls); diff --git a/talk/base/sslstreamadapterhelper.cc b/talk/base/sslstreamadapterhelper.cc index b42faa80c..7be2878d0 100644 --- a/talk/base/sslstreamadapterhelper.cc +++ b/talk/base/sslstreamadapterhelper.cc @@ -80,13 +80,6 @@ StreamState SSLStreamAdapterHelper::GetState() const { // not reached } -void SSLStreamAdapterHelper::SetPeerCertificate(SSLCertificate* cert) { - ASSERT(peer_certificate_.get() == NULL); - ASSERT(peer_certificate_digest_algorithm_.empty()); - ASSERT(ssl_server_name_.empty()); - peer_certificate_.reset(cert); -} - bool SSLStreamAdapterHelper::GetPeerCertificate(SSLCertificate** cert) const { if (!peer_certificate_) return false; diff --git a/talk/base/sslstreamadapterhelper.h b/talk/base/sslstreamadapterhelper.h index 7c2805661..e0eed3e4e 100644 --- a/talk/base/sslstreamadapterhelper.h +++ b/talk/base/sslstreamadapterhelper.h @@ -59,7 +59,6 @@ class SSLStreamAdapterHelper : public SSLStreamAdapter { virtual int StartSSLWithServer(const char* server_name); virtual int StartSSLWithPeer(); - virtual void SetPeerCertificate(SSLCertificate* cert); virtual bool SetPeerCertificateDigest(const std::string& digest_alg, const unsigned char* digest_val, size_t digest_len); @@ -114,12 +113,10 @@ class SSLStreamAdapterHelper : public SSLStreamAdapter { // in traditional mode, the server name that the server's certificate // must specify. Empty in peer-to-peer mode. std::string ssl_server_name_; - // In peer-to-peer mode, the certificate that the peer must - // present. Empty in traditional mode. + // The peer's certificate. Only used for GetPeerCertificate. scoped_ptr peer_certificate_; - // In peer-to-peer mode, the digest of the certificate that - // the peer must present. + // The digest of the certificate that the peer must present. Buffer peer_certificate_digest_value_; std::string peer_certificate_digest_algorithm_; diff --git a/talk/base/stream.cc b/talk/base/stream.cc index c1cf90743..b6b48f1d6 100644 --- a/talk/base/stream.cc +++ b/talk/base/stream.cc @@ -495,7 +495,7 @@ bool FileStream::Flush() { return false; } -#if defined(POSIX) +#if defined(POSIX) && !defined(__native_client__) bool FileStream::TryLock() { if (file_ == NULL) { diff --git a/talk/base/stream.h b/talk/base/stream.h index 4571def9b..d30be2917 100644 --- a/talk/base/stream.h +++ b/talk/base/stream.h @@ -449,7 +449,7 @@ class FileStream : public StreamInterface { virtual bool Flush(); -#if defined(POSIX) +#if defined(POSIX) && !defined(__native_client__) // Tries to aquire an exclusive lock on the file. // Use OpenShare(...) on win32 to get similar functionality. bool TryLock(); diff --git a/talk/base/thread.cc b/talk/base/thread.cc index d07efb515..d4cebc4d5 100644 --- a/talk/base/thread.cc +++ b/talk/base/thread.cc @@ -252,6 +252,9 @@ bool Thread::Start(Runnable* runnable) { #elif defined(POSIX) pthread_attr_t attr; pthread_attr_init(&attr); + + // Thread priorities are not supported in NaCl. +#if !defined(__native_client__) if (priority_ != PRIORITY_NORMAL) { if (priority_ == PRIORITY_IDLE) { // There is no POSIX-standard way to set a below-normal priority for an @@ -279,6 +282,8 @@ bool Thread::Start(Runnable* runnable) { } } } +#endif // !defined(__native_client__) + int error_code = pthread_create(&thread_, &attr, PreRun, init); if (0 != error_code) { LOG(LS_ERROR) << "Unable to create pthread, error " << error_code; diff --git a/talk/base/unixfilesystem.cc b/talk/base/unixfilesystem.cc index 74168f267..3c8f4d2bd 100644 --- a/talk/base/unixfilesystem.cc +++ b/talk/base/unixfilesystem.cc @@ -42,21 +42,26 @@ #if defined(POSIX) && !defined(OSX) #include -#ifdef ANDROID +#if defined(ANDROID) #include -#else +#elif !defined(__native_client__) #include -#endif // ANDROID +#endif // !defined(__native_client__) +#include #include #include #include #endif // POSIX && !OSX -#ifdef LINUX +#if defined(LINUX) #include #include #endif +#if defined(__native_client__) && !defined(__GLIBC__) +#include +#endif + #include "talk/base/fileutils.h" #include "talk/base/pathutils.h" #include "talk/base/stream.h" @@ -489,6 +494,9 @@ bool UnixFilesystem::GetAppTempFolder(Pathname* path) { } bool UnixFilesystem::GetDiskFreeSpace(const Pathname& path, int64 *freebytes) { +#ifdef __native_client__ + return false; +#else // __native_client__ ASSERT(NULL != freebytes); // TODO: Consider making relative paths absolute using cwd. // TODO: When popping off a symlink, push back on the components of the @@ -515,6 +523,7 @@ bool UnixFilesystem::GetDiskFreeSpace(const Pathname& path, int64 *freebytes) { #endif return true; +#endif // !__native_client__ } Pathname UnixFilesystem::GetCurrentDirectory() { diff --git a/talk/media/base/mediaengine.h b/talk/media/base/mediaengine.h index c04df9f8c..6e071ec2a 100644 --- a/talk/media/base/mediaengine.h +++ b/talk/media/base/mediaengine.h @@ -316,6 +316,7 @@ class NullVoiceEngine { return rtp_header_extensions_; } void SetLogging(int min_sev, const char* filter) {} + bool StartAecDump(FILE* file) { return false; } bool RegisterProcessor(uint32 ssrc, VoiceProcessor* voice_processor, MediaProcessorDirection direction) { return true; } diff --git a/talk/media/sctp/sctpdataengine.cc b/talk/media/sctp/sctpdataengine.cc index 653273bd2..5f18d4ff5 100644 --- a/talk/media/sctp/sctpdataengine.cc +++ b/talk/media/sctp/sctpdataengine.cc @@ -29,6 +29,7 @@ #include #include +#include #include #include "talk/app/webrtc/datachannelinterface.h" @@ -41,7 +42,69 @@ #include "talk/media/sctp/sctputils.h" #include "usrsctplib/usrsctp.h" +namespace { +typedef cricket::SctpDataMediaChannel::StreamSet StreamSet; +// Returns a comma-separated, human-readable list of the stream IDs in 's' +std::string ListStreams(const StreamSet& s) { + std::stringstream result; + bool first = true; + for (StreamSet::iterator it = s.begin(); it != s.end(); ++it) { + if (!first) { + result << ", " << *it; + } else { + result << *it; + first = false; + } + } + return result.str(); +} + +// Returns a pipe-separated, human-readable list of the SCTP_STREAM_RESET +// flags in 'flags' +std::string ListFlags(int flags) { + std::stringstream result; + bool first = true; + // Skip past the first 12 chars (strlen("SCTP_STREAM_")) +#define MAKEFLAG(X) { X, #X + 12} + struct flaginfo_t { + int value; + const char* name; + } flaginfo[] = { + MAKEFLAG(SCTP_STREAM_RESET_INCOMING_SSN), + MAKEFLAG(SCTP_STREAM_RESET_OUTGOING_SSN), + MAKEFLAG(SCTP_STREAM_RESET_DENIED), + MAKEFLAG(SCTP_STREAM_RESET_FAILED), + MAKEFLAG(SCTP_STREAM_CHANGE_DENIED) + }; +#undef MAKEFLAG + for (int i = 0; i < ARRAY_SIZE(flaginfo); ++i) { + if (flags & flaginfo[i].value) { + if (!first) result << " | "; + result << flaginfo[i].name; + first = false; + } + } + return result.str(); +} + +// Returns a comma-separated, human-readable list of the integers in 'array'. +// All 'num_elems' of them. +std::string ListArray(const uint16* array, int num_elems) { + std::stringstream result; + for (int i = 0; i < num_elems; ++i) { + if (i) { + result << ", " << array[i]; + } else { + result << array[i]; + } + } + return result.str(); +} +} // namespace + namespace cricket { +typedef talk_base::ScopedMessageData InboundPacketMessage; +typedef talk_base::ScopedMessageData OutboundPacketMessage; // This is the SCTP port to use. It is passed along the wire and the listener // and connector must be using the same port. It is not related to the ports at @@ -130,9 +193,9 @@ static int OnSctpOutboundPacket(void* addr, void* data, size_t length, << "; tos: " << std::hex << static_cast(tos) << "; set_df: " << std::hex << static_cast(set_df); // Note: We have to copy the data; the caller will delete it. - talk_base::Buffer* buffer = new talk_base::Buffer(data, length); - channel->worker_thread()->Post(channel, MSG_SCTPOUTBOUNDPACKET, - talk_base::WrapMessageData(buffer)); + OutboundPacketMessage* msg = + new OutboundPacketMessage(new talk_base::Buffer(data, length)); + channel->worker_thread()->Post(channel, MSG_SCTPOUTBOUNDPACKET, msg); return 0; } @@ -164,8 +227,9 @@ static int OnSctpInboundPacket(struct socket* sock, union sctp_sockstore addr, packet->params.timestamp = rcv.rcv_tsn; packet->params.type = type; packet->flags = flags; - channel->worker_thread()->Post(channel, MSG_SCTPINBOUNDPACKET, - talk_base::WrapMessageData(packet)); + // The ownership of |packet| transfers to |msg|. + InboundPacketMessage* msg = new InboundPacketMessage(packet); + channel->worker_thread()->Post(channel, MSG_SCTPINBOUNDPACKET, msg); } free(data); return 1; @@ -300,6 +364,18 @@ bool SctpDataMediaChannel::OpenSctpSocket() { return false; } + // Enable stream ID resets. + struct sctp_assoc_value stream_rst; + stream_rst.assoc_id = SCTP_ALL_ASSOC; + stream_rst.assoc_value = 1; + if (usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_ENABLE_STREAM_RESET, + &stream_rst, sizeof(stream_rst))) { + LOG_ERRNO(LS_ERROR) << debug_name_ + << "Failed to set SCTP_ENABLE_STREAM_RESET."; + return false; + } + + // Nagle. uint32_t nodelay = 1; if (usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_NODELAY, &nodelay, sizeof(nodelay))) { @@ -311,7 +387,8 @@ bool SctpDataMediaChannel::OpenSctpSocket() { int event_types[] = {SCTP_ASSOC_CHANGE, SCTP_PEER_ADDR_CHANGE, SCTP_SEND_FAILED_EVENT, - SCTP_SENDER_DRY_EVENT}; + SCTP_SENDER_DRY_EVENT, + SCTP_STREAM_RESET_EVENT}; struct sctp_event event = {0}; event.se_assoc_id = SCTP_ALL_ASSOC; event.se_on = 1; @@ -412,60 +489,19 @@ bool SctpDataMediaChannel::SetReceive(bool receive) { } bool SctpDataMediaChannel::AddSendStream(const StreamParams& stream) { - if (!stream.has_ssrcs()) { - return false; - } - - StreamParams found_stream; - // TODO(lally): Consider keeping this sorted. - if (GetStreamBySsrc(streams_, stream.first_ssrc(), &found_stream)) { - LOG(LS_WARNING) << debug_name_ << "->AddSendStream(...): " - << "Not adding data send stream '" << stream.id - << "' with ssrc=" << stream.first_ssrc() - << " because stream already exists."; - return false; - } - - streams_.push_back(stream); - return true; + return AddStream(stream); } bool SctpDataMediaChannel::RemoveSendStream(uint32 ssrc) { - StreamParams found_stream; - if (!GetStreamBySsrc(streams_, ssrc, &found_stream)) { - return false; - } - - RemoveStreamBySsrc(&streams_, ssrc); - return true; + return ResetStream(ssrc); } -// Note: expects exactly one ssrc. If none are given, it will fail. If more -// than one are given, it will use the first. bool SctpDataMediaChannel::AddRecvStream(const StreamParams& stream) { - if (!stream.has_ssrcs()) { - return false; - } - - StreamParams found_stream; - if (GetStreamBySsrc(streams_, stream.first_ssrc(), &found_stream)) { - LOG(LS_WARNING) << debug_name_ << "->AddRecvStream(...): " - << "Not adding data recv stream '" << stream.id - << "' with ssrc=" << stream.first_ssrc() - << " because stream already exists."; - return false; - } - - streams_.push_back(stream); - LOG(LS_VERBOSE) << debug_name_ << "->AddRecvStream(...): " - << "Added data recv stream '" << stream.id - << "' with ssrc=" << stream.first_ssrc(); - return true; + return AddStream(stream); } bool SctpDataMediaChannel::RemoveRecvStream(uint32 ssrc) { - RemoveStreamBySsrc(&streams_, ssrc); - return true; + return ResetStream(ssrc); } bool SctpDataMediaChannel::SendData( @@ -485,9 +521,8 @@ bool SctpDataMediaChannel::SendData( return false; } - StreamParams found_stream; if (params.type != cricket::DMT_CONTROL && - !GetStreamBySsrc(streams_, params.ssrc, &found_stream)) { + open_streams_.find(params.ssrc) == open_streams_.end()) { LOG(LS_WARNING) << debug_name_ << "->SendData(...): " << "Not sending data because ssrc is unknown: " << params.ssrc; @@ -584,8 +619,7 @@ void SctpDataMediaChannel::OnInboundPacketFromSctpToChannel( void SctpDataMediaChannel::OnDataFromSctpToChannel( const ReceiveDataParams& params, talk_base::Buffer* buffer) { - StreamParams found_stream; - if (!GetStreamBySsrc(streams_, params.ssrc, &found_stream)) { + if (open_streams_.find(params.ssrc) == open_streams_.end()) { if (params.type == DMT_CONTROL) { std::string label; webrtc::DataChannelInit config; @@ -596,8 +630,7 @@ void SctpDataMediaChannel::OnDataFromSctpToChannel( SignalNewStreamReceived(label, config); // Add the stream immediately. - cricket::StreamParams sparams = - cricket::StreamParams::CreateLegacy(params.ssrc); + StreamParams sparams = StreamParams::CreateLegacy(params.ssrc); AddSendStream(sparams); AddRecvStream(sparams); } else { @@ -623,6 +656,61 @@ void SctpDataMediaChannel::OnDataFromSctpToChannel( } } +bool SctpDataMediaChannel::AddStream(const StreamParams& stream) { + if (!stream.has_ssrcs()) { + return false; + } + + const uint32 ssrc = stream.first_ssrc(); + if (open_streams_.find(ssrc) != open_streams_.end()) { + // We usually get an AddSendStream and an AddRecvStream for each stream, so + // this is really unlikely to be a useful warning message. + LOG(LS_VERBOSE) << debug_name_ << "->Add(Send|Recv)Stream(...): " + << "Not adding data stream '" << stream.id + << "' with ssrc=" << ssrc + << " because stream is already open."; + return false; + } else if (queued_reset_streams_.find(ssrc) != queued_reset_streams_.end() + || sent_reset_streams_.find(ssrc) != sent_reset_streams_.end()) { + LOG(LS_WARNING) << debug_name_ << "->Add(Send|Recv)Stream(...): " + << "Not adding data stream '" << stream.id + << "' with ssrc=" << ssrc + << " because stream is still closing."; + return false; + } + + open_streams_.insert(ssrc); + return true; +} + +bool SctpDataMediaChannel::ResetStream(uint32 ssrc) { + // We typically get this called twice for the same stream, once each for + // Send and Recv. + StreamSet::iterator found = open_streams_.find(ssrc); + + if (found == open_streams_.end()) { + LOG(LS_VERBOSE) << debug_name_ << "->ResetStream(" << ssrc << "): " + << "stream not found."; + return false; + } else { + LOG(LS_VERBOSE) << debug_name_ << "->ResetStream(" << ssrc << "): " + << "Removing and queuing RE-CONFIG chunk."; + open_streams_.erase(found); + } + + // SCTP won't let you have more than one stream reset pending at a time, but + // you can close multiple streams in a single reset. So, we keep an internal + // queue of streams-to-reset, and send them as one reset message in + // SendQueuedStreamResets(). + queued_reset_streams_.insert(ssrc); + + // Signal our stream-reset logic that it should try to send now, if it can. + SendQueuedStreamResets(); + + // The stream will actually get removed when we get the acknowledgment. + return true; +} + void SctpDataMediaChannel::OnNotificationFromSctp(talk_base::Buffer* buffer) { const sctp_notification& notification = reinterpret_cast(*buffer->data()); @@ -641,7 +729,7 @@ void SctpDataMediaChannel::OnNotificationFromSctp(talk_base::Buffer* buffer) { LOG(LS_INFO) << "SCTP_SHUTDOWN_EVENT"; break; case SCTP_ADAPTATION_INDICATION: - LOG(LS_INFO) << "SCTP_ADAPTATION_INIDICATION"; + LOG(LS_INFO) << "SCTP_ADAPTATION_INDICATION"; break; case SCTP_PARTIAL_DELIVERY_EVENT: LOG(LS_INFO) << "SCTP_PARTIAL_DELIVERY_EVENT"; @@ -650,7 +738,7 @@ void SctpDataMediaChannel::OnNotificationFromSctp(talk_base::Buffer* buffer) { LOG(LS_INFO) << "SCTP_AUTHENTICATION_EVENT"; break; case SCTP_SENDER_DRY_EVENT: - LOG(LS_INFO) << "SCTP_SENDER_DRY_EVENT"; + LOG(LS_VERBOSE) << "SCTP_SENDER_DRY_EVENT"; SignalReadyToSend(true); break; // TODO(ldixon): Unblock after congestion. @@ -661,15 +749,18 @@ void SctpDataMediaChannel::OnNotificationFromSctp(talk_base::Buffer* buffer) { LOG(LS_INFO) << "SCTP_SEND_FAILED_EVENT"; break; case SCTP_STREAM_RESET_EVENT: - LOG(LS_INFO) << "SCTP_STREAM_RESET_EVENT"; - // TODO(ldixon): Notify up to channel that stream resent has happened, - // and write unit test for this case. + OnStreamResetEvent(¬ification.sn_strreset_event); break; case SCTP_ASSOC_RESET_EVENT: LOG(LS_INFO) << "SCTP_ASSOC_RESET_EVENT"; break; case SCTP_STREAM_CHANGE_EVENT: LOG(LS_INFO) << "SCTP_STREAM_CHANGE_EVENT"; + // An acknowledgment we get after our stream resets have gone through, + // if they've failed. We log the message, but don't react -- we don't + // keep around the last-transmitted set of SSIDs we wanted to close for + // error recovery. It doesn't seem likely to occur, and if so, likely + // harmless within the lifetime of a single SCTP association. break; default: LOG(LS_WARNING) << "Unknown SCTP event: " @@ -702,6 +793,91 @@ void SctpDataMediaChannel::OnNotificationAssocChange( } } +void SctpDataMediaChannel::OnStreamResetEvent( + const struct sctp_stream_reset_event* evt) { + // A stream reset always involves two RE-CONFIG chunks for us -- we always + // simultaneously reset a sid's sequence number in both directions. The + // requesting side transmits a RE-CONFIG chunk and waits for the peer to send + // one back. Both sides get this SCTP_STREAM_RESET_EVENT when they receive + // RE-CONFIGs. + const int num_ssrcs = (evt->strreset_length - sizeof(*evt)) / + sizeof(evt->strreset_stream_list[0]); + LOG(LS_VERBOSE) << "SCTP_STREAM_RESET_EVENT(" << debug_name_ + << "): Flags = 0x" + << std::hex << evt->strreset_flags << " (" + << ListFlags(evt->strreset_flags) << ")"; + LOG(LS_VERBOSE) << "Assoc = " << evt->strreset_assoc_id << ", Streams = [" + << ListArray(evt->strreset_stream_list, num_ssrcs) + << "], Open: [" + << ListStreams(open_streams_) << "], Q'd: [" + << ListStreams(queued_reset_streams_) << "], Sent: [" + << ListStreams(sent_reset_streams_) << "]"; + bool local_stream_reset_acknowledged = false; + + // If both sides try to reset some streams at the same time (even if they're + // disjoint sets), we can get reset failures. + if (evt->strreset_flags & SCTP_STREAM_RESET_FAILED) { + // OK, just try again. The stream IDs sent over when the RESET_FAILED flag + // is set seem to be garbage values. Ignore them. + queued_reset_streams_.insert( + sent_reset_streams_.begin(), + sent_reset_streams_.end()); + sent_reset_streams_.clear(); + local_stream_reset_acknowledged = true; + + } else if (evt->strreset_flags & SCTP_STREAM_RESET_INCOMING_SSN) { + // Each side gets an event for each direction of a stream. That is, + // closing sid k will make each side receive INCOMING and OUTGOING reset + // events for k. As per RFC6525, Section 5, paragraph 2, each side will + // get an INCOMING event first. + for (int i = 0; i < num_ssrcs; i++) { + const int stream_id = evt->strreset_stream_list[i]; + + // See if this stream ID was closed by our peer or ourselves. + StreamSet::iterator it = sent_reset_streams_.find(stream_id); + + // The reset was requested locally. + if (it != sent_reset_streams_.end()) { + LOG(LS_VERBOSE) << "SCTP_STREAM_RESET_EVENT(" << debug_name_ + << "): local sid " << stream_id << " acknowledged."; + local_stream_reset_acknowledged = true; + sent_reset_streams_.erase(it); + + } else if ((it = open_streams_.find(stream_id)) + != open_streams_.end()) { + // The peer requested the reset. + LOG(LS_VERBOSE) << "SCTP_STREAM_RESET_EVENT(" << debug_name_ + << "): closing sid " << stream_id; + open_streams_.erase(it); + SignalStreamClosed(stream_id); + + } else if ((it = queued_reset_streams_.find(stream_id)) + != queued_reset_streams_.end()) { + // The peer requested the reset, but there was a local reset + // queued. + LOG(LS_VERBOSE) << "SCTP_STREAM_RESET_EVENT(" << debug_name_ + << "): double-sided close for sid " << stream_id; + // Both sides want the stream closed, and the peer got to send the + // RE-CONFIG first. Treat it like the local Remove(Send|Recv)Stream + // finished quickly. + queued_reset_streams_.erase(it); + + } else { + // This stream is unknown. Sometimes this can be from an + // RESET_FAILED-related retransmit. + LOG(LS_VERBOSE) << "SCTP_STREAM_RESET_EVENT(" << debug_name_ + << "): Unknown sid " << stream_id; + } + } + } + + if (local_stream_reset_acknowledged) { + // This message acknowledges the last stream-reset request we sent out + // (only one can be outstanding at a time). Send out the next one. + SendQueuedStreamResets(); + } +} + // Puts the specified |param| from the codec identified by |id| into |dest| // and returns true. Or returns false if it wasn't there, leaving |dest| // untouched. @@ -739,28 +915,63 @@ void SctpDataMediaChannel::OnPacketFromSctpToNetwork( talk_base::Buffer* buffer) { if (buffer->length() > kSctpMtu) { LOG(LS_ERROR) << debug_name_ << "->OnPacketFromSctpToNetwork(...): " - << "SCTP seems to have made a poacket that is bigger " + << "SCTP seems to have made a packet that is bigger " "than its official MTU."; } MediaChannel::SendPacket(buffer); } +bool SctpDataMediaChannel::SendQueuedStreamResets() { + if (!sent_reset_streams_.empty() || queued_reset_streams_.empty()) + return true; + + LOG(LS_VERBOSE) << "SendQueuedStreamResets[" << debug_name_ << "]: Sending [" + << ListStreams(queued_reset_streams_) << "], Open: [" + << ListStreams(open_streams_) << "], Sent: [" + << ListStreams(sent_reset_streams_) << "]"; + + const size_t num_streams = queued_reset_streams_.size(); + const size_t num_bytes = sizeof(struct sctp_reset_streams) + + (num_streams * sizeof(uint16)); + + std::vector reset_stream_buf(num_bytes, 0); + struct sctp_reset_streams* resetp = reinterpret_cast( + &reset_stream_buf[0]); + resetp->srs_assoc_id = SCTP_ALL_ASSOC; + resetp->srs_flags = SCTP_STREAM_RESET_INCOMING | SCTP_STREAM_RESET_OUTGOING; + resetp->srs_number_streams = num_streams; + int result_idx = 0; + for (StreamSet::iterator it = queued_reset_streams_.begin(); + it != queued_reset_streams_.end(); ++it) { + resetp->srs_stream_list[result_idx++] = *it; + } + + int ret = usrsctp_setsockopt(sock_, IPPROTO_SCTP, SCTP_RESET_STREAMS, resetp, + reset_stream_buf.size()); + if (ret < 0) { + LOG_ERRNO(LS_ERROR) << debug_name_ << "Failed to send a stream reset for " + << num_streams << " streams"; + return false; + } + + // sent_reset_streams_ is empty, and all the queued_reset_streams_ go into + // it now. + queued_reset_streams_.swap(sent_reset_streams_); + return true; +} + void SctpDataMediaChannel::OnMessage(talk_base::Message* msg) { switch (msg->message_id) { case MSG_SCTPINBOUNDPACKET: { - SctpInboundPacket* packet = - static_cast*>( - msg->pdata)->data(); - OnInboundPacketFromSctpToChannel(packet); - delete packet; + talk_base::scoped_ptr pdata( + static_cast(msg->pdata)); + OnInboundPacketFromSctpToChannel(pdata->data().get()); break; } case MSG_SCTPOUTBOUNDPACKET: { - talk_base::Buffer* buffer = - static_cast*>( - msg->pdata)->data(); - OnPacketFromSctpToNetwork(buffer); - delete buffer; + talk_base::scoped_ptr pdata( + static_cast(msg->pdata)); + OnPacketFromSctpToNetwork(pdata->data().get()); break; } } diff --git a/talk/media/sctp/sctpdataengine.h b/talk/media/sctp/sctpdataengine.h index 4d05cf36e..8a5b4be0f 100644 --- a/talk/media/sctp/sctpdataengine.h +++ b/talk/media/sctp/sctpdataengine.h @@ -50,9 +50,9 @@ enum PreservedErrno { // Defined by "usrsctplib/usrsctp.h" struct sockaddr_conn; struct sctp_assoc_change; +struct sctp_stream_reset_event; // Defined by struct socket; - namespace cricket { // The highest stream ID (Sid) that SCTP allows, and the number of streams we // tell SCTP we're going to use. @@ -122,6 +122,8 @@ class SctpDataMediaChannel : public DataMediaChannel, PPID_TEXT_LAST = 51 }; + typedef std::set StreamSet; + // Given a thread which will be used to post messages (received data) to this // SctpDataMediaChannel instance. explicit SctpDataMediaChannel(talk_base::Thread* thread); @@ -181,6 +183,9 @@ class SctpDataMediaChannel : public DataMediaChannel, } const std::string& debug_name() const { return debug_name_; } + // Called with the SSID of a remote stream that's been closed. + sigslot::signal1 SignalStreamClosed; + private: sockaddr_conn GetSctpSockAddr(int port); @@ -195,6 +200,14 @@ class SctpDataMediaChannel : public DataMediaChannel, // Sets sending_ to false and sock_ to NULL. void CloseSctpSocket(); + // Sends a SCTP_RESET_STREAM for all streams in closing_ssids_. + bool SendQueuedStreamResets(); + + // Adds a stream. + bool AddStream(const StreamParams &sp); + // Queues a stream for reset. + bool ResetStream(uint32 ssrc); + // Called by OnMessage to send packet on the network. void OnPacketFromSctpToNetwork(talk_base::Buffer* buffer); // Called by OnMessage to decide what to do with the packet. @@ -204,6 +217,8 @@ class SctpDataMediaChannel : public DataMediaChannel, void OnNotificationFromSctp(talk_base::Buffer* buffer); void OnNotificationAssocChange(const sctp_assoc_change& change); + void OnStreamResetEvent(const struct sctp_stream_reset_event* evt); + // Responsible for marshalling incoming data to the channels listeners, and // outgoing data to the network interface. talk_base::Thread* worker_thread_; @@ -219,8 +234,17 @@ class SctpDataMediaChannel : public DataMediaChannel, bool sending_; // receiving_ controls whether inbound packets are thrown away. bool receiving_; - // Unified send/receive streams, as each is bidirectional. - std::vector streams_; + + // When a data channel opens a stream, it goes into open_streams_. When we + // want to close it, the stream's ID goes into queued_reset_streams_. When + // we actually transmit a RE-CONFIG chunk with that stream ID, the ID goes + // into sent_reset_streams_. When we get a response RE-CONFIG chunk back + // acknowledging the reset, we remove the stream ID from + // sent_reset_streams_. We use sent_reset_streams_ to differentiate + // between acknowledgment RE-CONFIG and peer-initiated RE-CONFIGs. + StreamSet open_streams_; + StreamSet queued_reset_streams_; + StreamSet sent_reset_streams_; // A human-readable name for debugging messages. std::string debug_name_; diff --git a/talk/media/sctp/sctpdataengine_unittest.cc b/talk/media/sctp/sctpdataengine_unittest.cc index b4ad6ce33..a6ae804ef 100644 --- a/talk/media/sctp/sctpdataengine_unittest.cc +++ b/talk/media/sctp/sctpdataengine_unittest.cc @@ -29,8 +29,10 @@ #include #include #include +#include #include "talk/app/webrtc/datachannelinterface.h" +#include "talk/base/bind.h" #include "talk/base/buffer.h" #include "talk/base/criticalsection.h" #include "talk/base/gunit.h" @@ -162,6 +164,55 @@ class SignalReadyToSendObserver : public sigslot::has_slots<> { bool writable_; }; +class SignalChannelClosedObserver : public sigslot::has_slots<> { + public: + SignalChannelClosedObserver() {} + void BindSelf(cricket::SctpDataMediaChannel* channel) { + channel->SignalStreamClosed.connect( + this, &SignalChannelClosedObserver::OnStreamClosed); + } + void OnStreamClosed(int stream) { + streams_.push_back(stream); + } + + int StreamCloseCount(int stream) { + return std::count(streams_.begin(), streams_.end(), stream); + } + + bool WasStreamClosed(int stream) { + return std::find(streams_.begin(), streams_.end(), stream) + != streams_.end(); + } + + private: + std::vector streams_; +}; + +class SignalChannelClosedReopener : public sigslot::has_slots<> { + public: + SignalChannelClosedReopener(cricket::SctpDataMediaChannel* channel, + cricket::SctpDataMediaChannel* peer) + : channel_(channel), peer_(peer) {} + + void OnStreamClosed(int stream) { + cricket::StreamParams p(cricket::StreamParams::CreateLegacy(stream)); + channel_->AddSendStream(p); + channel_->AddRecvStream(p); + peer_->AddSendStream(p); + peer_->AddRecvStream(p); + streams_.push_back(stream); + } + + int StreamCloseCount(int stream) { + return std::count(streams_.begin(), streams_.end(), stream); + } + + private: + cricket::SctpDataMediaChannel* channel_; + cricket::SctpDataMediaChannel* peer_; + std::vector streams_; +}; + // SCTP Data Engine testing framework. class SctpDataMediaChannelTest : public testing::Test, public sigslot::has_slots<> { @@ -184,11 +235,8 @@ class SctpDataMediaChannelTest : public testing::Test, net2_->SetDestination(chan1_.get()); LOG(LS_VERBOSE) << "Channel setup ----------------------------- "; - chan1_->AddSendStream(cricket::StreamParams::CreateLegacy(1)); - chan2_->AddRecvStream(cricket::StreamParams::CreateLegacy(1)); - - chan2_->AddSendStream(cricket::StreamParams::CreateLegacy(2)); - chan1_->AddRecvStream(cricket::StreamParams::CreateLegacy(2)); + AddStream(1); + AddStream(2); LOG(LS_VERBOSE) << "Connect the channels -----------------------------"; // chan1 wants to setup a data connection. @@ -206,8 +254,21 @@ class SctpDataMediaChannelTest : public testing::Test, chan1_->SetSend(true); } + virtual void TearDown() { + channel1()->SetSend(false); + channel2()->SetSend(false); + } + + void AddStream(int ssrc) { + cricket::StreamParams p(cricket::StreamParams::CreateLegacy(ssrc)); + chan1_->AddSendStream(p); + chan1_->AddRecvStream(p); + chan2_->AddSendStream(p); + chan2_->AddRecvStream(p); + } + cricket::SctpDataMediaChannel* CreateChannel( - SctpFakeNetworkInterface* net, SctpFakeDataReceiver* recv) { + SctpFakeNetworkInterface* net, SctpFakeDataReceiver* recv) { cricket::SctpDataMediaChannel* channel = static_cast(engine_->CreateChannel( cricket::DCT_SCTP)); @@ -225,7 +286,8 @@ class SctpDataMediaChannelTest : public testing::Test, cricket::SendDataResult* result) { cricket::SendDataParams params; params.ssrc = ssrc; - return chan->SendData(params, talk_base::Buffer(msg.data(), msg.length()), result); + return chan->SendData(params, talk_base::Buffer( + msg.data(), msg.length()), result); } bool ReceivedData(const SctpFakeDataReceiver* recv, uint32 ssrc, @@ -256,6 +318,7 @@ class SctpDataMediaChannelTest : public testing::Test, last_label_ = label; last_dc_init_ = init; } + std::string last_label() { return last_label_; } webrtc::DataChannelInit last_dc_init() { return last_dc_init_; } @@ -324,12 +387,6 @@ TEST_F(SctpDataMediaChannelTest, SendData) { << "recv1.last_params.seq_num=" << receiver1()->last_params().seq_num << "recv1.last_data=" << receiver1()->last_data(); - - LOG(LS_VERBOSE) << "Closing down. -----------------------------"; - // Disconnects and closes socket, including setting receiving to false. - channel1()->SetSend(false); - channel2()->SetSend(false); - LOG(LS_VERBOSE) << "Cleaning up. -----------------------------"; } TEST_F(SctpDataMediaChannelTest, SendReceiveOpenMessage) { @@ -358,3 +415,98 @@ TEST_F(SctpDataMediaChannelTest, SendReceiveOpenMessage) { // Verifies the received data. EXPECT_TRUE_WAIT(ReceivedData(receiver2(), config.id, "hi chan2"), 1000); } + +TEST_F(SctpDataMediaChannelTest, ClosesRemoteStream) { + SetupConnectedChannels(); + SignalChannelClosedObserver chan_1_sig_receiver, chan_2_sig_receiver; + chan_1_sig_receiver.BindSelf(channel1()); + chan_2_sig_receiver.BindSelf(channel2()); + + cricket::SendDataResult result; + ASSERT_TRUE(SendData(channel1(), 1, "hello?", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver2(), 1, "hello?"), 1000); + ASSERT_TRUE(SendData(channel2(), 2, "hi chan1", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver1(), 2, "hi chan1"), 1000); + + // Close channel 1. Channel 2 should notify us. + channel1()->RemoveSendStream(1); + EXPECT_TRUE_WAIT(chan_2_sig_receiver.WasStreamClosed(1), 1000); +} + +TEST_F(SctpDataMediaChannelTest, ClosesTwoRemoteStreams) { + SetupConnectedChannels(); + AddStream(3); + SignalChannelClosedObserver chan_1_sig_receiver, chan_2_sig_receiver; + chan_1_sig_receiver.BindSelf(channel1()); + chan_2_sig_receiver.BindSelf(channel2()); + + cricket::SendDataResult result; + ASSERT_TRUE(SendData(channel1(), 1, "hello?", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver2(), 1, "hello?"), 1000); + ASSERT_TRUE(SendData(channel2(), 2, "hi chan1", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver1(), 2, "hi chan1"), 1000); + + // Close two streams on one side. + channel2()->RemoveSendStream(2); + channel2()->RemoveSendStream(3); + EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(2), 1000); + EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(3), 1000); +} + +TEST_F(SctpDataMediaChannelTest, ClosesStreamsOnBothSides) { + SetupConnectedChannels(); + AddStream(3); + AddStream(4); + SignalChannelClosedObserver chan_1_sig_receiver, chan_2_sig_receiver; + chan_1_sig_receiver.BindSelf(channel1()); + chan_2_sig_receiver.BindSelf(channel2()); + + cricket::SendDataResult result; + ASSERT_TRUE(SendData(channel1(), 1, "hello?", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver2(), 1, "hello?"), 1000); + ASSERT_TRUE(SendData(channel2(), 2, "hi chan1", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver1(), 2, "hi chan1"), 1000); + + // Close one stream on channel1(), while closing three streams on + // channel2(). They will conflict (only one side can close anything at a + // time, apparently). Test the resolution of the conflict. + channel1()->RemoveSendStream(1); + + channel2()->RemoveSendStream(2); + channel2()->RemoveSendStream(3); + channel2()->RemoveSendStream(4); + EXPECT_TRUE_WAIT(chan_2_sig_receiver.WasStreamClosed(1), 1000); + EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(2), 1000); + EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(3), 1000); + EXPECT_TRUE_WAIT(chan_1_sig_receiver.WasStreamClosed(4), 1000); +} + +TEST_F(SctpDataMediaChannelTest, ReusesAStream) { + // Shut down channel 1, then open it up again for reuse. + SetupConnectedChannels(); + cricket::SendDataResult result; + SignalChannelClosedObserver chan_2_sig_receiver; + chan_2_sig_receiver.BindSelf(channel2()); + + ASSERT_TRUE(SendData(channel1(), 1, "hello?", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver2(), 1, "hello?"), 1000); + + channel1()->RemoveSendStream(1); + EXPECT_TRUE_WAIT(chan_2_sig_receiver.WasStreamClosed(1), 1000); + // Channel 1 is gone now. + + // Create a new channel 1. + AddStream(1); + ASSERT_TRUE(SendData(channel1(), 1, "hi?", &result)); + EXPECT_EQ(cricket::SDR_SUCCESS, result); + EXPECT_TRUE_WAIT(ReceivedData(receiver2(), 1, "hi?"), 1000); + channel1()->RemoveSendStream(1); + EXPECT_TRUE_WAIT(chan_2_sig_receiver.StreamCloseCount(1) == 2, 1000); +} diff --git a/talk/media/webrtc/webrtcvideocapturer.cc b/talk/media/webrtc/webrtcvideocapturer.cc index 6e81b4016..6b05b991e 100644 --- a/talk/media/webrtc/webrtcvideocapturer.cc +++ b/talk/media/webrtc/webrtcvideocapturer.cc @@ -32,6 +32,7 @@ #endif #ifdef HAVE_WEBRTC_VIDEO +#include "talk/base/criticalsection.h" #include "talk/base/logging.h" #include "talk/base/thread.h" #include "talk/base/timeutils.h" @@ -278,7 +279,13 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) { return CS_STARTING; } +// Critical section blocks Stop from shutting down during callbacks from capture +// thread to OnIncomingCapturedFrame. Note that the crit is try-locked in +// OnFrameCaptured, as the lock ordering between this and the system component +// controlling the camera is reversed: system frame -> OnIncomingCapturedFrame; +// Stop -> system stop camera). void WebRtcVideoCapturer::Stop() { + talk_base::CritScope cs(&critical_section_stopping_); if (IsRunning()) { talk_base::Thread::Current()->Clear(this); module_->StopCapture(); @@ -313,7 +320,17 @@ bool WebRtcVideoCapturer::GetPreferredFourccs( void WebRtcVideoCapturer::OnIncomingCapturedFrame(const int32_t id, webrtc::I420VideoFrame& sample) { - ASSERT(IsRunning()); + // This would be a normal CritScope, except that it's possible that: + // (1) whatever system component producing this frame has taken a lock, and + // (2) Stop() probably calls back into that system component, which may take + // the same lock. Due to the reversed order, we have to try-lock in order to + // avoid a potential deadlock. Besides, if we can't enter because we're + // stopping, we may as well drop the frame. + talk_base::TryCritScope cs(&critical_section_stopping_); + if (!cs.locked() || !IsRunning()) { + // Capturer has been stopped or is in the process of stopping. + return; + } ++captured_frames_; // Log the size and pixel aspect ratio of the first captured frame. diff --git a/talk/media/webrtc/webrtcvideocapturer.h b/talk/media/webrtc/webrtcvideocapturer.h index c20a05919..cefad5629 100644 --- a/talk/media/webrtc/webrtcvideocapturer.h +++ b/talk/media/webrtc/webrtcvideocapturer.h @@ -31,6 +31,7 @@ #include #include +#include "talk/base/criticalsection.h" #include "talk/base/messagehandler.h" #include "talk/media/base/videocapturer.h" #include "talk/media/webrtc/webrtcvideoframe.h" @@ -89,6 +90,9 @@ class WebRtcVideoCapturer : public VideoCapturer, webrtc::VideoCaptureModule* module_; int captured_frames_; std::vector capture_buffer_; + + // Critical section to avoid Stop during an OnIncomingCapturedFrame callback. + talk_base::CriticalSection critical_section_stopping_; }; struct WebRtcCapturedFrame : public CapturedFrame { diff --git a/talk/media/webrtc/webrtcvoiceengine_unittest.cc b/talk/media/webrtc/webrtcvoiceengine_unittest.cc index 9bb681a89..dc725c0f0 100644 --- a/talk/media/webrtc/webrtcvoiceengine_unittest.cc +++ b/talk/media/webrtc/webrtcvoiceengine_unittest.cc @@ -2892,7 +2892,8 @@ TEST(WebRtcVoiceEngineTest, HasCorrectCodecs) { EXPECT_FALSE(engine.FindCodec(cricket::AudioCodec(0, "", 5000, 0, 1, 0))); EXPECT_FALSE(engine.FindCodec(cricket::AudioCodec(0, "", 0, 5000, 1, 0))); // Check that there aren't any extra codecs lying around. - EXPECT_EQ(13U, engine.codecs().size()); + size_t codecs_num = 12; + EXPECT_EQ(codecs_num, engine.codecs().size()); // Verify the payload id of common audio codecs, including CN, ISAC, and G722. for (std::vector::const_iterator it = engine.codecs().begin(); it != engine.codecs().end(); ++it) { diff --git a/talk/p2p/base/port.cc b/talk/p2p/base/port.cc index 24ef4271f..2fc2cb2b6 100644 --- a/talk/p2p/base/port.cc +++ b/talk/p2p/base/port.cc @@ -176,7 +176,7 @@ Port::Port(talk_base::Thread* thread, talk_base::PacketSocketFactory* factory, generation_(0), ice_username_fragment_(username_fragment), password_(password), - lifetime_(LT_PRESTART), + timeout_delay_(kPortTimeoutDelay), enable_port_packets_(false), ice_protocol_(ICEPROTO_GOOGLE), ice_role_(ICEROLE_UNKNOWN), @@ -203,7 +203,7 @@ Port::Port(talk_base::Thread* thread, const std::string& type, generation_(0), ice_username_fragment_(username_fragment), password_(password), - lifetime_(LT_PRESTART), + timeout_delay_(kPortTimeoutDelay), enable_port_packets_(false), ice_protocol_(ICEPROTO_GOOGLE), ice_role_(ICEROLE_UNKNOWN), @@ -669,8 +669,6 @@ void Port::SendBindingErrorResponse(StunMessage* request, void Port::OnMessage(talk_base::Message *pmsg) { ASSERT(pmsg->message_id == MSG_CHECKTIMEOUT); - ASSERT(lifetime_ == LT_PRETIMEOUT); - lifetime_ = LT_POSTTIMEOUT; CheckTimeout(); } @@ -686,24 +684,18 @@ void Port::EnablePortPackets() { enable_port_packets_ = true; } -void Port::Start() { - // The port sticks around for a minimum lifetime, after which - // we destroy it when it drops to zero connections. - if (lifetime_ == LT_PRESTART) { - lifetime_ = LT_PRETIMEOUT; - thread_->PostDelayed(kPortTimeoutDelay, this, MSG_CHECKTIMEOUT); - } else { - LOG_J(LS_WARNING, this) << "Port restart attempted"; - } -} - void Port::OnConnectionDestroyed(Connection* conn) { AddressMap::iterator iter = connections_.find(conn->remote_candidate().address()); ASSERT(iter != connections_.end()); connections_.erase(iter); - CheckTimeout(); + // On the controlled side, ports time out, but only after all connections + // fail. Note: If a new connection is added after this message is posted, + // but it fails and is removed before kPortTimeoutDelay, then this message + // will still cause the Port to be destroyed. + if (ice_role_ == ICEROLE_CONTROLLED) + thread_->PostDelayed(timeout_delay_, this, MSG_CHECKTIMEOUT); } void Port::Destroy() { @@ -714,13 +706,13 @@ void Port::Destroy() { } void Port::CheckTimeout() { + ASSERT(ice_role_ == ICEROLE_CONTROLLED); // If this port has no connections, then there's no reason to keep it around. // When the connections time out (both read and write), they will delete // themselves, so if we have any connections, they are either readable or // writable (or still connecting). - if ((lifetime_ == LT_POSTTIMEOUT) && connections_.empty()) { + if (connections_.empty()) Destroy(); - } } const std::string Port::username_fragment() const { diff --git a/talk/p2p/base/port.h b/talk/p2p/base/port.h index 9ea3f0c37..21f3d6192 100644 --- a/talk/p2p/base/port.h +++ b/talk/p2p/base/port.h @@ -262,10 +262,6 @@ class Port : public PortInterface, public talk_base::MessageHandler, virtual void EnablePortPackets(); - // Indicates to the port that its official use has now begun. This will - // start the timer that checks to see if the port is being used. - void Start(); - // Called if the port has no connections and is no longer useful. void Destroy(); @@ -277,6 +273,9 @@ class Port : public PortInterface, public talk_base::MessageHandler, int min_port() { return min_port_; } int max_port() { return max_port_; } + // Timeout shortening function to speed up unit tests. + void set_timeout_delay(int delay) { timeout_delay_ = delay; } + // This method will return local and remote username fragements from the // stun username attribute if present. bool ParseStunUsername(const StunMessage* stun_msg, @@ -379,7 +378,7 @@ class Port : public PortInterface, public talk_base::MessageHandler, std::string password_; std::vector candidates_; AddressMap connections_; - enum Lifetime { LT_PRESTART, LT_PRETIMEOUT, LT_POSTTIMEOUT } lifetime_; + int timeout_delay_; bool enable_port_packets_; IceProtocolType ice_protocol_; IceRole ice_role_; diff --git a/talk/p2p/base/port_unittest.cc b/talk/p2p/base/port_unittest.cc index 1122d8aea..3ea6375b4 100644 --- a/talk/p2p/base/port_unittest.cc +++ b/talk/p2p/base/port_unittest.cc @@ -213,12 +213,14 @@ class TestPort : public Port { class TestChannel : public sigslot::has_slots<> { public: + // Takes ownership of |p1| (but not |p2|). TestChannel(Port* p1, Port* p2) : ice_mode_(ICEMODE_FULL), src_(p1), dst_(p2), complete_count_(0), conn_(NULL), remote_request_(), nominated_(false) { src_->SignalPortComplete.connect( this, &TestChannel::OnPortComplete); src_->SignalUnknownAddress.connect(this, &TestChannel::OnUnknownAddress); + src_->SignalDestroyed.connect(this, &TestChannel::OnSrcPortDestroyed); } int complete_count() { return complete_count_; } @@ -305,6 +307,11 @@ class TestChannel : public sigslot::has_slots<> { conn_ = NULL; } + void OnSrcPortDestroyed(PortInterface* port) { + Port* destroyed_src = src_.release(); + ASSERT_EQ(destroyed_src, port); + } + bool nominated() const { return nominated_; } private: @@ -341,7 +348,8 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { username_(talk_base::CreateRandomString(ICE_UFRAG_LENGTH)), password_(talk_base::CreateRandomString(ICE_PWD_LENGTH)), ice_protocol_(cricket::ICEPROTO_GOOGLE), - role_conflict_(false) { + role_conflict_(false), + destroyed_(false) { network_.AddIP(talk_base::IPAddress(INADDR_ANY)); } @@ -513,12 +521,17 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { void TestCrossFamilyPorts(int type); - // this does all the work + // This does all the work and then deletes |port1| and |port2|. void TestConnectivity(const char* name1, Port* port1, const char* name2, Port* port2, bool accept, bool same_addr1, bool same_addr2, bool possible); + // This connects and disconnects the provided channels in the same sequence as + // TestConnectivity with all options set to |true|. It does not delete either + // channel. + void ConnectAndDisconnectChannels(TestChannel* ch1, TestChannel* ch2); + void SetIceProtocolType(cricket::IceProtocolType protocol) { ice_protocol_ = protocol; } @@ -562,6 +575,15 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { } bool role_conflict() const { return role_conflict_; } + void ConnectToSignalDestroyed(PortInterface* port) { + port->SignalDestroyed.connect(this, &PortTest::OnDestroyed); + } + + void OnDestroyed(PortInterface* port) { + destroyed_ = true; + } + bool destroyed() const { return destroyed_; } + talk_base::BasicPacketSocketFactory* nat_socket_factory1() { return &nat_socket_factory1_; } @@ -586,6 +608,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> { std::string password_; cricket::IceProtocolType ice_protocol_; bool role_conflict_; + bool destroyed_; }; void PortTest::TestConnectivity(const char* name1, Port* port1, @@ -596,7 +619,7 @@ void PortTest::TestConnectivity(const char* name1, Port* port1, port1->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT); port2->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT); - // Set up channels. + // Set up channels and ensure both ports will be deleted. TestChannel ch1(port1, port2); TestChannel ch2(port2, port1); EXPECT_EQ(0, ch1.complete_count()); @@ -719,6 +742,29 @@ void PortTest::TestConnectivity(const char* name1, Port* port1, EXPECT_TRUE_WAIT(ch2.conn() == NULL, kTimeout); } +void PortTest::ConnectAndDisconnectChannels(TestChannel* ch1, + TestChannel* ch2) { + // Acquire addresses. + ch1->Start(); + ch2->Start(); + + // Send a ping from src to dst. + ch1->CreateConnection(); + EXPECT_TRUE_WAIT(ch1->conn()->connected(), kTimeout); // for TCP connect + ch1->Ping(); + WAIT(!ch2->remote_address().IsNil(), kTimeout); + + // Send a ping from dst to src. + ch2->AcceptConnection(); + ch2->Ping(); + EXPECT_EQ_WAIT(Connection::STATE_WRITABLE, ch2->conn()->write_state(), + kTimeout); + + // Destroy the connections. + ch1->Stop(); + ch2->Stop(); +} + class FakePacketSocketFactory : public talk_base::PacketSocketFactory { public: FakePacketSocketFactory() @@ -2292,3 +2338,59 @@ TEST_F(PortTest, TestIceLiteConnectivity) { EXPECT_TRUE(msg->GetByteString(STUN_ATTR_USE_CANDIDATE) != NULL); ch1.Stop(); } + +// This test case verifies that the CONTROLLING port does not time out. +TEST_F(PortTest, TestControllingNoTimeout) { + SetIceProtocolType(cricket::ICEPROTO_RFC5245); + UDPPort* port1 = CreateUdpPort(kLocalAddr1); + ConnectToSignalDestroyed(port1); + port1->set_timeout_delay(10); // milliseconds + port1->SetIceRole(cricket::ICEROLE_CONTROLLING); + port1->SetIceTiebreaker(kTiebreaker1); + + UDPPort* port2 = CreateUdpPort(kLocalAddr2); + port2->SetIceRole(cricket::ICEROLE_CONTROLLED); + port2->SetIceTiebreaker(kTiebreaker2); + + // Set up channels and ensure both ports will be deleted. + TestChannel ch1(port1, port2); + TestChannel ch2(port2, port1); + + // Simulate a connection that succeeds, and then is destroyed. + ConnectAndDisconnectChannels(&ch1, &ch2); + + // After the connection is destroyed, the port should not be destroyed. + talk_base::Thread::Current()->ProcessMessages(kTimeout); + EXPECT_FALSE(destroyed()); +} + +// This test case verifies that the CONTROLLED port does time out, but only +// after connectivity is lost. +TEST_F(PortTest, TestControlledTimeout) { + SetIceProtocolType(cricket::ICEPROTO_RFC5245); + UDPPort* port1 = CreateUdpPort(kLocalAddr1); + port1->SetIceRole(cricket::ICEROLE_CONTROLLING); + port1->SetIceTiebreaker(kTiebreaker1); + + UDPPort* port2 = CreateUdpPort(kLocalAddr2); + ConnectToSignalDestroyed(port2); + port2->set_timeout_delay(10); // milliseconds + port2->SetIceRole(cricket::ICEROLE_CONTROLLED); + port2->SetIceTiebreaker(kTiebreaker2); + + // The connection must not be destroyed before a connection is attempted. + EXPECT_FALSE(destroyed()); + + port1->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT); + port2->set_component(cricket::ICE_CANDIDATE_COMPONENT_DEFAULT); + + // Set up channels and ensure both ports will be deleted. + TestChannel ch1(port1, port2); + TestChannel ch2(port2, port1); + + // Simulate a connection that succeeds, and then is destroyed. + ConnectAndDisconnectChannels(&ch1, &ch2); + + // The controlled port should be destroyed after 10 milliseconds. + EXPECT_TRUE_WAIT(destroyed(), kTimeout); +} diff --git a/talk/p2p/client/basicportallocator.cc b/talk/p2p/client/basicportallocator.cc index dbc2e3342..e568e75c4 100644 --- a/talk/p2p/client/basicportallocator.cc +++ b/talk/p2p/client/basicportallocator.cc @@ -515,8 +515,6 @@ void BasicPortAllocatorSession::AddAllocatedPort(Port* port, if (prepare_address) port->PrepareAddress(); - if (running_) - port->Start(); } void BasicPortAllocatorSession::OnAllocationSequenceObjectsCreated() { diff --git a/talk/session/tunnel/securetunnelsessionclient.cc b/talk/session/tunnel/securetunnelsessionclient.cc index 9287d22ab..55f408387 100644 --- a/talk/session/tunnel/securetunnelsessionclient.cc +++ b/talk/session/tunnel/securetunnelsessionclient.cc @@ -360,8 +360,8 @@ void SecureTunnelSession::OnAccept() { const std::string& cert_pem = role_ == INITIATOR ? remote_tunnel->server_pem_certificate : remote_tunnel->client_pem_certificate; - talk_base::SSLCertificate* peer_cert = - ParseCertificate(cert_pem); + talk_base::scoped_ptr peer_cert( + ParseCertificate(cert_pem)); if (peer_cert == NULL) { ASSERT(role_ == INITIATOR); // when RESPONDER we validated it earlier LOG(LS_ERROR) @@ -373,7 +373,17 @@ void SecureTunnelSession::OnAccept() { talk_base::SSLStreamAdapter* ssl_stream = static_cast( ssl_stream_reference_->GetStream()); - ssl_stream->SetPeerCertificate(peer_cert); // pass ownership of certificate. + + std::string algorithm; + if (!peer_cert->GetSignatureDigestAlgorithm(&algorithm)) { + LOG(LS_ERROR) << "Failed to get the algorithm for the peer cert signature"; + return; + } + unsigned char digest[talk_base::MessageDigest::kMaxSize]; + size_t digest_len; + peer_cert->ComputeDigest(algorithm, digest, ARRAY_SIZE(digest), &digest_len); + ssl_stream->SetPeerCertificateDigest(algorithm, digest, digest_len); + // We no longer need our handle to the ssl stream. ssl_stream_reference_.reset(); LOG(LS_INFO) << "Connecting tunnel";