Reland "Set session error if SetLocal/RemoteDescription ever fails"
Original change's description:
> Set session error if SetLocal/RemoteDescription ever fails
>
> This changes SetLocalDescription/SetRemoteDescription to set a
> session error which will cause any future calls to fail early if
> there is an error when applying a session description.
>
> This is needed since until better error recovery is implemented
> failing a call to SetLocalDescription or SetRemoteDescription
> could leave the PeerConnection in an inconsistent state.
>
> Bug: chromium:800775
> Change-Id: If06fd73d6e902af15d072dc562bbe830d3b11ad5
> Reviewed-on: https://webrtc-review.googlesource.com/54061
> Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
> Commit-Queue: Steve Anton <steveanton@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#22061}
Bug: chromium:800775
Change-Id: I0016108264e013452e9d34239c012baf23240e99
Reviewed-on: https://webrtc-review.googlesource.com/54720
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22067}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index f04b6e5..73f79c8 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -561,6 +561,17 @@
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::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<SessionDescriptionInterface> desc(desc_ptr);
+
if (!observer) {
RTC_LOG(LS_ERROR) << "SetLocalDescription - observer is NULL.";
return;
@@ -1746,17 +1763,39 @@
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 @@
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 @@
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 @@
return;
}
- const 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) << "SetRemoteDescription: " << error_message;
+ observer->OnSetRemoteDescriptionComplete(
+ RTCError(RTCErrorType::INTERNAL_ERROR, std::move(error_message)));
+ return;
+ }
- RTCError error = ApplyRemoteDescription(std::move(desc));
- // |desc| may be destroyed at this point.
-
+ 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<void>(
@@ -2036,11 +2095,6 @@
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 @@
// 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 @@
// 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 c6cfb90..b2f2469 100644
--- a/pc/peerconnection_crypto_unittest.cc
+++ b/pc/peerconnection_crypto_unittest.cc
@@ -672,6 +672,49 @@
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 06dd876..eaccdaf 100644
--- a/pc/peerconnection_ice_unittest.cc
+++ b/pc/peerconnection_ice_unittest.cc
@@ -482,23 +482,22 @@
// 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 3a88e80..8d1dd76 100644
--- a/pc/peerconnection_media_unittest.cc
+++ b/pc/peerconnection_media_unittest.cc
@@ -918,9 +918,8 @@
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 c4631cd..0dd8f12 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 ee62d35..1d3019b 100644
--- a/rtc_base/gunit.h
+++ b/rtc_base/gunit.h
@@ -163,4 +163,10 @@
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_
