From fec2c6d7eb58574b32eaa26222d3fb903b738cfa Mon Sep 17 00:00:00 2001
From: Joachim Bauch <jbauch@webrtc.org>
Date: Wed, 27 May 2015 23:41:43 +0200
Subject: [PATCH] Prevent potential double-free if srtp_create fails.

If srtp_create fails while adding streams, it deallocates the session
but doesn't clear the passed pointer which then could lead to a
double-free in the SrtpSession dtor.

The CL also adds locking for libsrtp initialization / shutdown.

BUG=4042
R=jiayl@webrtc.org, juberti@google.com, pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/47319004

Cr-Commit-Position: refs/heads/master@{#9300}
---
 talk/session/media/srtpfilter.cc |  6 ++++++
 talk/session/media/srtpfilter.h  |  2 ++
 webrtc/base/criticalsection.cc   |  9 +++++++++
 webrtc/base/criticalsection.h    | 10 ++++++++++
 4 files changed, 27 insertions(+)

diff --git a/talk/session/media/srtpfilter.cc b/talk/session/media/srtpfilter.cc
index b47844f2ee..5fc6946232 100644
--- a/talk/session/media/srtpfilter.cc
+++ b/talk/session/media/srtpfilter.cc
@@ -474,6 +474,7 @@ bool SrtpFilter::ParseKeyParams(const std::string& key_params,
 #ifdef HAVE_SRTP
 
 bool SrtpSession::inited_ = false;
+rtc::GlobalLockPod SrtpSession::lock_;
 
 SrtpSession::SrtpSession()
     : session_(NULL),
@@ -691,6 +692,7 @@ bool SrtpSession::SetKey(int type, const std::string& cs,
 
   int err = srtp_create(&session_, &policy);
   if (err != err_status_ok) {
+    session_ = NULL;
     LOG(LS_ERROR) << "Failed to create SRTP session, err=" << err;
     return false;
   }
@@ -702,6 +704,8 @@ bool SrtpSession::SetKey(int type, const std::string& cs,
 }
 
 bool SrtpSession::Init() {
+  rtc::GlobalLockScope ls(&lock_);
+
   if (!inited_) {
     int err;
     err = srtp_init();
@@ -729,6 +733,8 @@ bool SrtpSession::Init() {
 }
 
 void SrtpSession::Terminate() {
+  rtc::GlobalLockScope ls(&lock_);
+
   if (inited_) {
     int err = srtp_shutdown();
     if (err) {
diff --git a/talk/session/media/srtpfilter.h b/talk/session/media/srtpfilter.h
index 43cb241b75..f171f5fe66 100644
--- a/talk/session/media/srtpfilter.h
+++ b/talk/session/media/srtpfilter.h
@@ -36,6 +36,7 @@
 #include "talk/media/base/cryptoparams.h"
 #include "webrtc/p2p/base/sessiondescription.h"
 #include "webrtc/base/basictypes.h"
+#include "webrtc/base/criticalsection.h"
 #include "webrtc/base/scoped_ptr.h"
 #include "webrtc/base/sigslotrepeater.h"
 
@@ -242,6 +243,7 @@ class SrtpSession {
   int rtcp_auth_tag_len_;
   rtc::scoped_ptr<SrtpStat> srtp_stat_;
   static bool inited_;
+  static rtc::GlobalLockPod lock_;
   int last_send_seq_num_;
   DISALLOW_COPY_AND_ASSIGN(SrtpSession);
 };
diff --git a/webrtc/base/criticalsection.cc b/webrtc/base/criticalsection.cc
index f4822d96fa..4f3a28f2f4 100644
--- a/webrtc/base/criticalsection.cc
+++ b/webrtc/base/criticalsection.cc
@@ -152,4 +152,13 @@ GlobalLock::GlobalLock() {
   lock_acquired = 0;
 }
 
+GlobalLockScope::GlobalLockScope(GlobalLockPod* lock)
+    : lock_(lock) {
+  lock_->Lock();
+}
+
+GlobalLockScope::~GlobalLockScope() {
+  lock_->Unlock();
+}
+
 }  // namespace rtc
diff --git a/webrtc/base/criticalsection.h b/webrtc/base/criticalsection.h
index bd45d85bca..46a4f3408b 100644
--- a/webrtc/base/criticalsection.h
+++ b/webrtc/base/criticalsection.h
@@ -114,6 +114,16 @@ class GlobalLock : public GlobalLockPod {
   GlobalLock();
 };
 
+// GlobalLockScope, for serializing execution through a scope.
+class SCOPED_LOCKABLE GlobalLockScope {
+ public:
+  explicit GlobalLockScope(GlobalLockPod* lock) EXCLUSIVE_LOCK_FUNCTION(lock);
+  ~GlobalLockScope() UNLOCK_FUNCTION();
+ private:
+  GlobalLockPod* const lock_;
+  DISALLOW_COPY_AND_ASSIGN(GlobalLockScope);
+};
+
 } // namespace rtc
 
 #endif // WEBRTC_BASE_CRITICALSECTION_H_