admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Reland "Set session error if SetLocal/RemoteDescription ever fails"

Browse Source 
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}
This commit is contained in:
Steve Anton 2018-02-16 17:08:42 -08:00 committed by Commit Bot
parent 8b815cddca
commit 80dd7b5d68
6 changed files with 157 additions and 46 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

119
pc/peerconnection.cc
View File

@ -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<SessionDescriptionInterface> 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<void>(
@ -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();

43
pc/peerconnection_crypto_unittest.cc
View File

@ -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,

17
pc/peerconnection_ice_unittest.cc
View File

@ -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));

5
pc/peerconnection_media_unittest.cc
View File

@ -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);
}

13
rtc_base/gunit.cc
View File

@ -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 << "\"";
}
}

6
rtc_base/gunit.h
View File

@ -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_