Fix the SrtpFilter crash caused by two local offers.
BUG=http://crbug.com/421774
R=juberti@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/30789004
git-svn-id: http://webrtc.googlecode.com/svn/trunk@7530 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/talk/session/media/srtpfilter.cc b/talk/session/media/srtpfilter.cc
index c79bf13..5a774db 100644
--- a/talk/session/media/srtpfilter.cc
+++ b/talk/session/media/srtpfilter.cc
@@ -150,7 +150,7 @@
const uint8* send_key, int send_key_len,
const std::string& recv_cs,
const uint8* recv_key, int recv_key_len) {
- if (state_ == ST_ACTIVE) {
+ if (IsActive()) {
LOG(LS_ERROR) << "Tried to set SRTP Params when filter already active";
return false;
}
@@ -212,6 +212,7 @@
LOG(LS_WARNING) << "Failed to ProtectRtp: SRTP not active";
return false;
}
+ ASSERT(send_session_ != NULL);
return send_session_->ProtectRtp(p, in_len, max_len, out_len);
}
@@ -221,7 +222,7 @@
LOG(LS_WARNING) << "Failed to ProtectRtp: SRTP not active";
return false;
}
-
+ ASSERT(send_session_ != NULL);
return send_session_->ProtectRtp(p, in_len, max_len, out_len, index);
}
@@ -233,6 +234,7 @@
if (send_rtcp_session_) {
return send_rtcp_session_->ProtectRtcp(p, in_len, max_len, out_len);
} else {
+ ASSERT(send_session_ != NULL);
return send_session_->ProtectRtcp(p, in_len, max_len, out_len);
}
}
@@ -242,6 +244,7 @@
LOG(LS_WARNING) << "Failed to UnprotectRtp: SRTP not active";
return false;
}
+ ASSERT(recv_session_ != NULL);
return recv_session_->UnprotectRtp(p, in_len, out_len);
}
@@ -253,6 +256,7 @@
if (recv_rtcp_session_) {
return recv_rtcp_session_->UnprotectRtcp(p, in_len, out_len);
} else {
+ ASSERT(recv_session_ != NULL);
return recv_session_->UnprotectRtcp(p, in_len, out_len);
}
}
@@ -263,13 +267,16 @@
return false;
}
+ ASSERT(send_session_ != NULL);
return send_session_->GetRtpAuthParams(key, key_len, tag_len);
}
void SrtpFilter::set_signal_silent_time(uint32 signal_silent_time_in_ms) {
signal_silent_time_in_ms_ = signal_silent_time_in_ms;
- if (state_ == ST_ACTIVE) {
+ if (IsActive()) {
+ ASSERT(send_session_ != NULL);
send_session_->set_signal_silent_time(signal_silent_time_in_ms);
+ ASSERT(recv_session_ != NULL);
recv_session_->set_signal_silent_time(signal_silent_time_in_ms);
if (send_rtcp_session_)
send_rtcp_session_->set_signal_silent_time(signal_silent_time_in_ms);
@@ -292,7 +299,7 @@
offer_params_ = params;
if (state_ == ST_INIT) {
state_ = (source == CS_LOCAL) ? ST_SENTOFFER : ST_RECEIVEDOFFER;
- } else { // state >= ST_ACTIVE
+ } else if (state_ == ST_ACTIVE) {
state_ =
(source == CS_LOCAL) ? ST_SENTUPDATEDOFFER : ST_RECEIVEDUPDATEDOFFER;
}
diff --git a/talk/session/media/srtpfilter_unittest.cc b/talk/session/media/srtpfilter_unittest.cc
index 19efa2a..8b859f8 100644
--- a/talk/session/media/srtpfilter_unittest.cc
+++ b/talk/session/media/srtpfilter_unittest.cc
@@ -82,9 +82,12 @@
const std::vector<CryptoParams>& params2) {
EXPECT_TRUE(f1_.SetOffer(params1, CS_LOCAL));
EXPECT_TRUE(f2_.SetOffer(params1, CS_REMOTE));
+ EXPECT_FALSE(f1_.IsActive());
+ EXPECT_FALSE(f2_.IsActive());
EXPECT_TRUE(f2_.SetAnswer(params2, CS_LOCAL));
EXPECT_TRUE(f1_.SetAnswer(params2, CS_REMOTE));
EXPECT_TRUE(f1_.IsActive());
+ EXPECT_TRUE(f2_.IsActive());
}
void TestProtectUnprotect(const std::string& cs1, const std::string& cs2) {
char rtp_packet[sizeof(kPcmuFrame) + 10];
@@ -139,6 +142,7 @@
// Test that we can set up the session and keys properly.
TEST_F(SrtpFilterTest, TestGoodSetupOneCipherSuite) {
EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL));
+ EXPECT_FALSE(f1_.IsActive());
EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams2), CS_REMOTE));
EXPECT_TRUE(f1_.IsActive());
}
@@ -153,6 +157,7 @@
answer[0].tag = 2;
answer[0].cipher_suite = CS_AES_CM_128_HMAC_SHA1_32;
EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL));
+ EXPECT_FALSE(f1_.IsActive());
EXPECT_TRUE(f1_.SetAnswer(answer, CS_REMOTE));
EXPECT_TRUE(f1_.IsActive());
}
@@ -188,6 +193,7 @@
TEST_F(SrtpFilterTest, TestGoodSetupMultipleOffers) {
EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL));
EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams2), CS_LOCAL));
+ EXPECT_FALSE(f1_.IsActive());
EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams2), CS_REMOTE));
EXPECT_TRUE(f1_.IsActive());
EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL));
@@ -196,6 +202,7 @@
EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams1), CS_REMOTE));
EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE));
+ EXPECT_FALSE(f2_.IsActive());
EXPECT_TRUE(f2_.SetAnswer(MakeVector(kTestCryptoParams2), CS_LOCAL));
EXPECT_TRUE(f2_.IsActive());
EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams1), CS_REMOTE));
@@ -206,6 +213,7 @@
TEST_F(SrtpFilterTest, TestBadSetupMultipleOffers) {
EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL));
EXPECT_FALSE(f1_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE));
+ EXPECT_FALSE(f1_.IsActive());
EXPECT_TRUE(f1_.SetAnswer(MakeVector(kTestCryptoParams1), CS_REMOTE));
EXPECT_TRUE(f1_.IsActive());
EXPECT_TRUE(f1_.SetOffer(MakeVector(kTestCryptoParams2), CS_LOCAL));
@@ -214,6 +222,7 @@
EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE));
EXPECT_FALSE(f2_.SetOffer(MakeVector(kTestCryptoParams1), CS_LOCAL));
+ EXPECT_FALSE(f2_.IsActive());
EXPECT_TRUE(f2_.SetAnswer(MakeVector(kTestCryptoParams2), CS_LOCAL));
EXPECT_TRUE(f2_.IsActive());
EXPECT_TRUE(f2_.SetOffer(MakeVector(kTestCryptoParams2), CS_REMOTE));
@@ -402,6 +411,8 @@
EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL));
EXPECT_TRUE(f2_.SetOffer(offer, CS_REMOTE));
+ EXPECT_FALSE(f1_.IsActive());
+ EXPECT_FALSE(f2_.IsActive());
EXPECT_TRUE(f2_.SetProvisionalAnswer(answer, CS_LOCAL));
EXPECT_TRUE(f1_.SetProvisionalAnswer(answer, CS_REMOTE));
EXPECT_TRUE(f1_.IsActive());
@@ -425,6 +436,8 @@
EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL));
EXPECT_TRUE(f2_.SetOffer(offer, CS_REMOTE));
+ EXPECT_FALSE(f1_.IsActive());
+ EXPECT_FALSE(f2_.IsActive());
EXPECT_TRUE(f2_.SetProvisionalAnswer(answer, CS_LOCAL));
EXPECT_TRUE(f1_.SetProvisionalAnswer(answer, CS_REMOTE));
EXPECT_FALSE(f1_.IsActive());
@@ -438,6 +451,33 @@
TestProtectUnprotect(CS_AES_CM_128_HMAC_SHA1_80, CS_AES_CM_128_HMAC_SHA1_80);
}
+// Test that if we get a new local offer after a provisional answer
+// with no crypto, that we are in an inactive state.
+TEST_F(SrtpFilterTest, TestLocalOfferAfterProvisionalAnswerWithoutCrypto) {
+ std::vector<CryptoParams> offer(MakeVector(kTestCryptoParams1));
+ std::vector<CryptoParams> answer;
+
+ EXPECT_TRUE(f1_.SetOffer(offer, CS_LOCAL));
+ EXPECT_TRUE(f2_.SetOffer(offer, CS_REMOTE));
+ EXPECT_TRUE(f1_.SetProvisionalAnswer(answer, CS_REMOTE));
+ EXPECT_TRUE(f2_.SetProvisionalAnswer(answer, CS_LOCAL));
+ EXPECT_FALSE(f1_.IsActive());
+ EXPECT_FALSE(f2_.IsActive());
+ // The calls to set an offer after a provisional answer fail, so the
+ // state doesn't change.
+ EXPECT_FALSE(f1_.SetOffer(offer, CS_LOCAL));
+ EXPECT_FALSE(f2_.SetOffer(offer, CS_REMOTE));
+ EXPECT_FALSE(f1_.IsActive());
+ EXPECT_FALSE(f2_.IsActive());
+
+ answer.push_back(kTestCryptoParams2);
+ EXPECT_TRUE(f2_.SetAnswer(answer, CS_LOCAL));
+ EXPECT_TRUE(f1_.SetAnswer(answer, CS_REMOTE));
+ EXPECT_TRUE(f1_.IsActive());
+ EXPECT_TRUE(f2_.IsActive());
+ TestProtectUnprotect(CS_AES_CM_128_HMAC_SHA1_80, CS_AES_CM_128_HMAC_SHA1_80);
+}
+
// Test that we can disable encryption.
TEST_F(SrtpFilterTest, TestDisableEncryption) {
std::vector<CryptoParams> offer(MakeVector(kTestCryptoParams1));
