diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index f04b6e5667..73f79c8a11 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -561,6 +561,17 @@ bool CheckForRemoteIceRestart(const SessionDescriptionInterface* old_desc, return false; } +// Generates a string error message for SetLocalDescription/SetRemoteDescription +// from an RTCError. +std::string GetSetDescriptionErrorMessage(cricket::ContentSource source, + SdpType type, + const RTCError& error) { + std::ostringstream oss; + oss << "Failed to set " << (source == cricket::CS_LOCAL ? "local" : "remote") + << " " << SdpTypeToString(type) << " sdp: " << error.message(); + return oss.str(); +} + } // namespace // Upon completion, posts a task to execute the callback of the @@ -1733,9 +1744,15 @@ void PeerConnection::CreateAnswer(CreateSessionDescriptionObserver* observer, void PeerConnection::SetLocalDescription( SetSessionDescriptionObserver* observer, - SessionDescriptionInterface* desc) { + SessionDescriptionInterface* desc_ptr) { TRACE_EVENT0("webrtc", "PeerConnection::SetLocalDescription"); + // The SetLocalDescription contract is that we take ownership of the session + // description regardless of the outcome, so wrap it in a unique_ptr right + // away. Ideally, SetLocalDescription's signature will be changed to take the + // description as a unique_ptr argument to formalize this agreement. + std::unique_ptr desc(desc_ptr); + if (!observer) { RTC_LOG(LS_ERROR) << "SetLocalDescription - observer is NULL."; return; @@ -1746,17 +1763,39 @@ void PeerConnection::SetLocalDescription( return; } - SdpType type = desc->GetType(); + // If a session error has occurred the PeerConnection is in a possibly + // inconsistent state so fail right away. + if (session_error() != SessionError::kNone) { + std::string error_message = GetSessionErrorMsg(); + RTC_LOG(LS_ERROR) << "SetLocalDescription: " << error_message; + PostSetSessionDescriptionFailure(observer, std::move(error_message)); + return; + } - RTCError error = ApplyLocalDescription(rtc::WrapUnique(desc)); + RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_LOCAL); + if (!error.ok()) { + std::string error_message = GetSetDescriptionErrorMessage( + cricket::CS_LOCAL, desc->GetType(), error); + RTC_LOG(LS_ERROR) << error_message; + PostSetSessionDescriptionFailure(observer, std::move(error_message)); + return; + } + + // Grab the description type before moving ownership to ApplyLocalDescription, + // which may destroy it before returning. + const SdpType type = desc->GetType(); + + error = ApplyLocalDescription(std::move(desc)); // |desc| may be destroyed at this point. if (!error.ok()) { - std::ostringstream oss; - oss << "Failed to set local " << SdpTypeToString(type) - << " sdp: " << error.message(); - std::string error_message = oss.str(); - RTC_LOG(LS_ERROR) << error_message << " (" << error.type() << ")"; + // If ApplyLocalDescription fails, the PeerConnection could be in an + // inconsistent state, so act conservatively here and set the session error + // so that future calls to SetLocalDescription/SetRemoteDescription fail. + SetSessionError(SessionError::kContent, error.message()); + std::string error_message = + GetSetDescriptionErrorMessage(cricket::CS_LOCAL, type, error); + RTC_LOG(LS_ERROR) << error_message; PostSetSessionDescriptionFailure(observer, std::move(error_message)); return; } @@ -1788,11 +1827,6 @@ RTCError PeerConnection::ApplyLocalDescription( RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(desc); - RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_LOCAL); - if (!error.ok()) { - return error; - } - // Update stats here so that we have the most recent stats for tracks and // streams that might be removed by updating the session description. stats_->UpdateStats(kStatsOutputLevelStandard); @@ -1867,7 +1901,7 @@ RTCError PeerConnection::ApplyLocalDescription( RemoveUnusedChannels(local_description()->description()); } - error = UpdateSessionState(type, cricket::CS_LOCAL); + RTCError error = UpdateSessionState(type, cricket::CS_LOCAL); if (!error.ok()) { return error; } @@ -1976,23 +2010,48 @@ void PeerConnection::SetRemoteDescription( return; } - const SdpType type = desc->GetType(); - - RTCError error = ApplyRemoteDescription(std::move(desc)); - // |desc| may be destroyed at this point. + // If a session error has occurred the PeerConnection is in a possibly + // inconsistent state so fail right away. + if (session_error() != SessionError::kNone) { + std::string error_message = GetSessionErrorMsg(); + RTC_LOG(LS_ERROR) << "SetRemoteDescription: " << error_message; + observer->OnSetRemoteDescriptionComplete( + RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message))); + return; + } + RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE); if (!error.ok()) { - std::ostringstream oss; - oss << "Failed to set remote " << SdpTypeToString(type) - << " sdp: " << error.message(); - std::string error_message = oss.str(); - RTC_LOG(LS_ERROR) << error_message << " (" << error.type() << ")"; + std::string error_message = GetSetDescriptionErrorMessage( + cricket::CS_REMOTE, desc->GetType(), error); + RTC_LOG(LS_ERROR) << error_message; observer->OnSetRemoteDescriptionComplete( RTCError(error.type(), std::move(error_message))); return; } - if (remote_description()->GetType() == SdpType::kAnswer) { + // Grab the description type before moving ownership to + // ApplyRemoteDescription, which may destroy it before returning. + const SdpType type = desc->GetType(); + + error = ApplyRemoteDescription(std::move(desc)); + // |desc| may be destroyed at this point. + + if (!error.ok()) { + // If ApplyRemoteDescription fails, the PeerConnection could be in an + // inconsistent state, so act conservatively here and set the session error + // so that future calls to SetLocalDescription/SetRemoteDescription fail. + SetSessionError(SessionError::kContent, error.message()); + std::string error_message = + GetSetDescriptionErrorMessage(cricket::CS_REMOTE, type, error); + RTC_LOG(LS_ERROR) << error_message; + observer->OnSetRemoteDescriptionComplete( + RTCError(error.type(), std::move(error_message))); + return; + } + RTC_DCHECK(remote_description()); + + if (type == SdpType::kAnswer) { // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... network_thread()->Invoke( @@ -2036,11 +2095,6 @@ RTCError PeerConnection::ApplyRemoteDescription( RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(desc); - RTCError error = ValidateSessionDescription(desc.get(), cricket::CS_REMOTE); - if (!error.ok()) { - return error; - } - // Update stats here so that we have the most recent stats for tracks and // streams that might be removed by updating the session description. stats_->UpdateStats(kStatsOutputLevelStandard); @@ -2093,7 +2147,7 @@ RTCError PeerConnection::ApplyRemoteDescription( // NOTE: Candidates allocation will be initiated only when SetLocalDescription // is called. - error = UpdateSessionState(type, cricket::CS_REMOTE); + RTCError error = UpdateSessionState(type, cricket::CS_REMOTE); if (!error.ok()) { return error; } @@ -4499,10 +4553,7 @@ RTCError PeerConnection::UpdateSessionState(SdpType type, // descriptions. error = PushdownMediaDescription(type, source); if (!error.ok()) { - SetSessionError(SessionError::kContent, error.message()); - } - if (session_error() != SessionError::kNone) { - LOG_AND_RETURN_ERROR(RTCErrorType::INTERNAL_ERROR, GetSessionErrorMsg()); + return error; } return RTCError::OK(); diff --git a/pc/peerconnection_crypto_unittest.cc b/pc/peerconnection_crypto_unittest.cc index c6cfb90204..b2f24692bf 100644 --- a/pc/peerconnection_crypto_unittest.cc +++ b/pc/peerconnection_crypto_unittest.cc @@ -672,6 +672,49 @@ TEST_P(PeerConnectionCryptoTest, CreateAnswerWithDifferentSslRoles) { ASSERT_TRUE(callee->SetRemoteDescription(std::move(answer))); } +// Tests that if the DTLS fingerprint is invalid then all future calls to +// SetLocalDescription and SetRemoteDescription will fail due to a session +// error. +// This is a regression test for crbug.com/800775 +TEST_P(PeerConnectionCryptoTest, SessionErrorIfFingerprintInvalid) { + auto callee_certificate = rtc::RTCCertificate::FromPEM(kRsaPems[0]); + auto other_certificate = rtc::RTCCertificate::FromPEM(kRsaPems[1]); + + auto caller = CreatePeerConnectionWithAudioVideo(); + RTCConfiguration callee_config; + callee_config.enable_dtls_srtp.emplace(true); + callee_config.certificates.push_back(callee_certificate); + auto callee = CreatePeerConnectionWithAudioVideo(callee_config); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + // Create an invalid answer with the other certificate's fingerprint. + auto invalid_answer = callee->CreateAnswer(); + auto* audio_content = + cricket::GetFirstAudioContent(invalid_answer->description()); + ASSERT_TRUE(audio_content); + auto* audio_transport_info = + invalid_answer->description()->GetTransportInfoByName( + audio_content->name); + ASSERT_TRUE(audio_transport_info); + audio_transport_info->description.identity_fingerprint.reset( + rtc::SSLFingerprint::CreateFromCertificate(other_certificate)); + + // Set the invalid answer and expect a fingerprint error. + std::string error; + ASSERT_FALSE(callee->SetLocalDescription(std::move(invalid_answer), &error)); + EXPECT_PRED_FORMAT2(AssertStringContains, error, + "Local fingerprint does not match identity."); + + // Make sure that setting a valid remote offer or local answer also fails now. + ASSERT_FALSE(callee->SetRemoteDescription(caller->CreateOffer(), &error)); + EXPECT_PRED_FORMAT2(AssertStringContains, error, + "Session error code: ERROR_CONTENT."); + ASSERT_FALSE(callee->SetLocalDescription(callee->CreateAnswer(), &error)); + EXPECT_PRED_FORMAT2(AssertStringContains, error, + "Session error code: ERROR_CONTENT."); +} + INSTANTIATE_TEST_CASE_P(PeerConnectionCryptoTest, PeerConnectionCryptoTest, Values(SdpSemantics::kPlanB, diff --git a/pc/peerconnection_ice_unittest.cc b/pc/peerconnection_ice_unittest.cc index 06dd876dd7..eaccdaf54f 100644 --- a/pc/peerconnection_ice_unittest.cc +++ b/pc/peerconnection_ice_unittest.cc @@ -482,23 +482,22 @@ TEST_P(PeerConnectionIceTest, // The standard (https://tools.ietf.org/html/rfc5245#section-15.4) says that // pwd must be 22-256 characters and ufrag must be 4-256 characters. TEST_P(PeerConnectionIceTest, VerifyUfragPwdLength) { - auto caller = CreatePeerConnectionWithAudioVideo(); - auto callee = CreatePeerConnectionWithAudioVideo(); - auto set_local_description_with_ufrag_pwd_length = - [this, &caller](int ufrag_len, int pwd_len) { - auto offer = caller->CreateOffer(); + [this](int ufrag_len, int pwd_len) { + auto pc = CreatePeerConnectionWithAudioVideo(); + auto offer = pc->CreateOffer(); SetIceUfragPwd(offer.get(), std::string(ufrag_len, 'x'), std::string(pwd_len, 'x')); - return caller->SetLocalDescription(std::move(offer)); + return pc->SetLocalDescription(std::move(offer)); }; auto set_remote_description_with_ufrag_pwd_length = - [this, &caller, &callee](int ufrag_len, int pwd_len) { - auto offer = caller->CreateOffer(); + [this](int ufrag_len, int pwd_len) { + auto pc = CreatePeerConnectionWithAudioVideo(); + auto offer = pc->CreateOffer(); SetIceUfragPwd(offer.get(), std::string(ufrag_len, 'x'), std::string(pwd_len, 'x')); - return callee->SetRemoteDescription(std::move(offer)); + return pc->SetRemoteDescription(std::move(offer)); }; EXPECT_FALSE(set_local_description_with_ufrag_pwd_length(3, 22)); diff --git a/pc/peerconnection_media_unittest.cc b/pc/peerconnection_media_unittest.cc index 3a88e80007..8d1dd7694f 100644 --- a/pc/peerconnection_media_unittest.cc +++ b/pc/peerconnection_media_unittest.cc @@ -918,9 +918,8 @@ TEST_P(PeerConnectionMediaTest, MediaEngineErrorPropagatedToClients) { ASSERT_FALSE(caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal(), &error)); EXPECT_EQ( - "Failed to set remote answer sdp: Session error code: ERROR_CONTENT. " - "Session error description: Failed to set remote video description send " - "parameters..", + "Failed to set remote answer sdp: Failed to set remote video description " + "send parameters.", error); } diff --git a/rtc_base/gunit.cc b/rtc_base/gunit.cc index c4631cdcdc..0dd8f1230d 100644 --- a/rtc_base/gunit.cc +++ b/rtc_base/gunit.cc @@ -26,3 +26,16 @@ << prefix_expr << "\nwhich is\n\"" << prefix << "\""; } } + +::testing::AssertionResult AssertStringContains(const char* str_expr, + const char* substr_expr, + const std::string& str, + const std::string& substr) { + if (str.find(substr) != std::string::npos) { + return ::testing::AssertionSuccess(); + } else { + return ::testing::AssertionFailure() + << str_expr << "\nwhich is\n\"" << str << "\"\ndoes not contain\n" + << substr_expr << "\nwhich is\n\"" << substr << "\""; + } +} diff --git a/rtc_base/gunit.h b/rtc_base/gunit.h index ee62d35dcc..1d3019b58c 100644 --- a/rtc_base/gunit.h +++ b/rtc_base/gunit.h @@ -163,4 +163,10 @@ testing::AssertionResult AssertStartsWith(const char* str_expr, const std::string& str, const std::string& prefix); +// Usage: EXPECT_PRED_FORMAT2(AssertStringContains, str, "substring"); +testing::AssertionResult AssertStringContains(const char* str_expr, + const char* substr_expr, + const std::string& str, + const std::string& substr); + #endif // RTC_BASE_GUNIT_H_