From 0163607fa3e9633e4c3522217cef1d983714fc44 Mon Sep 17 00:00:00 2001 From: Rahul Frias Date: Sun, 13 May 2018 13:53:14 -0700 Subject: [PATCH 1/3] Revert of "Prevent race conditions between decrypt and close session" [ Original CL http://ag/3890635, Merge of http://go/wvgerrit/50340 ] The original fix was not sufficient to address all race conditions. A subsequent CL will address them. Bug: 73781703 Bug: 79158083 Bug: 79262108 Test: WV unit/integration tests, GTS GtsMediaTestCases tests and 24 hours of continuous Netflix playback. Change-Id: I869c22a250e2467b3d49935815e4157dc012fff5 --- libwvdrmengine/cdm/core/include/cdm_session.h | 5 +---- libwvdrmengine/cdm/core/include/wv_cdm_types.h | 1 - libwvdrmengine/cdm/core/src/cdm_session.cpp | 10 ---------- libwvdrmengine/cdm/core/test/test_printers.cpp | 2 -- libwvdrmengine/include/mapErrors-inl.h | 1 - libwvdrmengine/include_hidl/mapErrors-inl.h | 1 - 6 files changed, 1 insertion(+), 19 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/cdm_session.h b/libwvdrmengine/cdm/core/include/cdm_session.h index 854f459d..113c9e96 100644 --- a/libwvdrmengine/cdm/core/include/cdm_session.h +++ b/libwvdrmengine/cdm/core/include/cdm_session.h @@ -37,7 +37,7 @@ class CdmSession { CdmSession(FileSystem* file_system, metrics::SessionMetrics* metrics); virtual ~CdmSession(); - void Close(); + void Close() { closed_ = true; } bool IsClosed() { return closed_; } // Initializes this instance of CdmSession with the given property set. @@ -263,9 +263,6 @@ class CdmSession { bool mock_license_parser_in_use_; bool mock_policy_engine_in_use_; - // Lock to avoid race conditions between Close() and Decrypt() - Lock close_lock_; - CORE_DISALLOW_COPY_AND_ASSIGN(CdmSession); }; diff --git a/libwvdrmengine/cdm/core/include/wv_cdm_types.h b/libwvdrmengine/cdm/core/include/wv_cdm_types.h index 19fc6310..bead3941 100644 --- a/libwvdrmengine/cdm/core/include/wv_cdm_types.h +++ b/libwvdrmengine/cdm/core/include/wv_cdm_types.h @@ -333,7 +333,6 @@ enum CdmResponseType { GET_PROVISIONING_METHOD_ERROR = 289, SESSION_NOT_FOUND_17 = 290, SESSION_NOT_FOUND_18 = 291, - SESSION_CLOSED_1 = 292, }; enum CdmKeyStatus { diff --git a/libwvdrmengine/cdm/core/src/cdm_session.cpp b/libwvdrmengine/cdm/core/src/cdm_session.cpp index 5e71f747..55808aac 100644 --- a/libwvdrmengine/cdm/core/src/cdm_session.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_session.cpp @@ -72,11 +72,6 @@ CdmSession::~CdmSession() { } } -void CdmSession::Close() { - AutoLock lock(close_lock_); - closed_ = true; -} - CdmResponseType CdmSession::Init( CdmClientPropertySet* cdm_client_property_set) { return Init(cdm_client_property_set, NULL, NULL); @@ -581,11 +576,6 @@ CdmResponseType CdmSession::Decrypt(const CdmDecryptionParameters& params) { return NOT_INITIALIZED_ERROR; } - AutoLock lock(close_lock_); - if (IsClosed()) { - return SESSION_CLOSED_1; - } - // Playback may not begin until either the start time passes or the license // is updated, so we treat this Decrypt call as invalid. if (params.is_encrypted) { diff --git a/libwvdrmengine/cdm/core/test/test_printers.cpp b/libwvdrmengine/cdm/core/test/test_printers.cpp index 0dc40bf6..475c5f89 100644 --- a/libwvdrmengine/cdm/core/test/test_printers.cpp +++ b/libwvdrmengine/cdm/core/test/test_printers.cpp @@ -599,8 +599,6 @@ void PrintTo(const enum CdmResponseType& value, ::std::ostream* os) { break; case SESSION_NOT_FOUND_18: *os << "SESSION_NOT_FOUND_18"; break; - case SESSION_CLOSED_1: *os << "SESSION_CLOSED_1"; - break; default: *os << "Unknown CdmResponseType"; break; diff --git a/libwvdrmengine/include/mapErrors-inl.h b/libwvdrmengine/include/mapErrors-inl.h index 8ebc454a..029c11d9 100644 --- a/libwvdrmengine/include/mapErrors-inl.h +++ b/libwvdrmengine/include/mapErrors-inl.h @@ -230,7 +230,6 @@ static android::status_t mapCdmResponseType(wvcdm::CdmResponseType res) { case wvcdm::SESSION_NOT_FOUND_10: case wvcdm::SESSION_NOT_FOUND_17: case wvcdm::SESSION_NOT_FOUND_18: - case wvcdm::SESSION_CLOSED_1: return android::ERROR_DRM_SESSION_NOT_OPENED; case wvcdm::SESSION_KEYS_NOT_FOUND: return kSessionKeysNotFound; diff --git a/libwvdrmengine/include_hidl/mapErrors-inl.h b/libwvdrmengine/include_hidl/mapErrors-inl.h index 18503efb..1637e626 100644 --- a/libwvdrmengine/include_hidl/mapErrors-inl.h +++ b/libwvdrmengine/include_hidl/mapErrors-inl.h @@ -58,7 +58,6 @@ static Status mapCdmResponseType(wvcdm::CdmResponseType res) { case wvcdm::SESSION_NOT_FOUND_10: case wvcdm::SESSION_NOT_FOUND_17: case wvcdm::SESSION_NOT_FOUND_18: - case wvcdm::SESSION_CLOSED_1: return Status::ERROR_DRM_SESSION_NOT_OPENED; case wvcdm::DECRYPT_ERROR: From dcab2b135596e7ba808740b31f4e8377e6c3d1ad Mon Sep 17 00:00:00 2001 From: Rahul Frias Date: Sun, 13 May 2018 15:54:25 -0700 Subject: [PATCH 2/3] Address concurrency failures between calls to decrypt and periodic timer [ http://go/wvgerrit/50341 ] The shared_ptr implementation was taken from a google3 implementation. Updates to the reference counter needed to be atomic and were platform dependent in the original code. These were not carried over to this codebase. Race conditions between calls to decrypt and the periodic timer, led to incorrect reference count values. CdmSession objects were then destructed while references to them still existed. Segfaults occurred when they were referenced. Bug: 79431096 Test: WV unit/integration tests, GTS GtsMediaTestCases tests and 24 hours of continuous Netflix playback. Change-Id: I6008ddba869efcc58972e5ea8644a204f91410ab --- libwvdrmengine/cdm/core/include/shared_ptr.h | 10 ++++++++++ libwvdrmengine/cdm/core/src/cdm_engine.cpp | 2 ++ 2 files changed, 12 insertions(+) diff --git a/libwvdrmengine/cdm/core/include/shared_ptr.h b/libwvdrmengine/cdm/core/include/shared_ptr.h index 20d4dce3..07c2f31c 100644 --- a/libwvdrmengine/cdm/core/include/shared_ptr.h +++ b/libwvdrmengine/cdm/core/include/shared_ptr.h @@ -12,9 +12,17 @@ #include #include +#include "lock.h" #include "wv_cdm_types.h" +namespace wvcdm { + +extern Lock shared_ptr_ref_count_lock_; + +} // namespace wvcdm + namespace { + bool Barrier_AtomicIncrement(volatile uint32_t* ptr, uint32_t value) { *ptr += value; return *ptr; @@ -25,10 +33,12 @@ bool NoBarrier_AtomicIncrement(volatile uint32_t* ptr, uint32_t value) { } inline bool RefCountDec(volatile uint32_t *ptr) { + wvcdm::AutoLock auto_lock(wvcdm::shared_ptr_ref_count_lock_); return Barrier_AtomicIncrement(ptr, -1) != 0; } inline void RefCountInc(volatile uint32_t *ptr) { + wvcdm::AutoLock auto_lock(wvcdm::shared_ptr_ref_count_lock_); NoBarrier_AtomicIncrement(ptr, 1); } diff --git a/libwvdrmengine/cdm/core/src/cdm_engine.cpp b/libwvdrmengine/cdm/core/src/cdm_engine.cpp index 8de6167c..81d8dffc 100644 --- a/libwvdrmengine/cdm/core/src/cdm_engine.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_engine.cpp @@ -30,6 +30,8 @@ const size_t kUsageReportsPerRequest = 1; namespace wvcdm { +Lock shared_ptr_ref_count_lock_; + class UsagePropertySet : public CdmClientPropertySet { public: UsagePropertySet() {} From e8c3a4afacb48fdddff886ce99d021ba0d728eee Mon Sep 17 00:00:00 2001 From: Rahul Frias Date: Sun, 13 May 2018 21:14:26 -0700 Subject: [PATCH 3/3] Protect sessions from concurrent access. Locks in earlier releases controlled access to sessions and the list of sessions for each CdmEngine instance. This guarded against concurrent access between session management (OpenSession, CloseSession, etc), periodic timer calls and calls to Decrypt. The list of sessions and locking was moved to a separate class CdmSessionMap. This left open the possibility that a session might be destructed, while being called to decrypt or invoked through the timer. An attempt was made to add per-session locks in b/73781703 but this was found insufficient. Per-session locks will be introduced in a future changelist, but for now the coarser locks will be reintroduced. Bug: 73781703 Bug: 79158083 Bug: 79262108 Bug: 79436509 Test: WV unit/integration tests, GTS GtsMediaTestCases tests and 24 hours of continuous Netflix playback. Change-Id: I30a3ede340192370dfe5c92c01b1c76df16b7123 --- libwvdrmengine/cdm/core/include/cdm_engine.h | 11 +++++++++++ .../cdm/core/include/cdm_session_map.h | 9 +++++++-- libwvdrmengine/cdm/core/src/cdm_engine.cpp | 17 +++++++++++++---- libwvdrmengine/cdm/core/src/cdm_session_map.cpp | 10 ++++------ 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/cdm_engine.h b/libwvdrmengine/cdm/core/include/cdm_engine.h index d8898635..d713e08b 100644 --- a/libwvdrmengine/cdm/core/include/cdm_engine.h +++ b/libwvdrmengine/cdm/core/include/cdm_engine.h @@ -330,6 +330,17 @@ class CdmEngine { // Protect release_key_sets_ from non-thread-safe operations. Lock release_key_sets_lock_; + // TODO(rfrias): Replace with two sets of locks, one to protect + // the CdmSessionMap and a per-session lock to control access to + // session usage/destruction. + // Locks the session map |session_map_| and session usage/destruction + // between session management calls (OpenSession, CloseSession, etc), + // periodic timer calls (OnTimerEvent), and calls to Decrypt. + // The layer above the CDM implementation is expected to handle thread + // synchronization to make sure other functions that access sessions do not + // occur simultaneously with OpenSession or CloseSession. + Lock session_map_lock_; + CORE_DISALLOW_COPY_AND_ASSIGN(CdmEngine); }; diff --git a/libwvdrmengine/cdm/core/include/cdm_session_map.h b/libwvdrmengine/cdm/core/include/cdm_session_map.h index 0e9fc779..7fa06b81 100644 --- a/libwvdrmengine/cdm/core/include/cdm_session_map.h +++ b/libwvdrmengine/cdm/core/include/cdm_session_map.h @@ -10,7 +10,6 @@ #include #include "cdm_session.h" -#include "lock.h" #include "shared_ptr.h" #include "wv_cdm_types.h" @@ -18,11 +17,18 @@ namespace wvcdm { typedef std::list > CdmSessionList; +// TODO(rfrias): Concurrency protection for this class has moved to CdmEngine. +// Add it back when locks to control access to session usage and destruction +// are introduced. class CdmSessionMap { public: CdmSessionMap() {} virtual ~CdmSessionMap(); + // Use |Terminate| rather than relying on the destructor to release + // resources, as it can be protected by locks. + void Terminate(); + void Add(const std::string& id, CdmSession* session); bool CloseSession(const std::string& id); @@ -43,7 +49,6 @@ class CdmSessionMap { bool FindSessionNoLock(const CdmSessionId& session_id, shared_ptr* session); - Lock lock_; CdmIdToSessionMap sessions_; CORE_DISALLOW_COPY_AND_ASSIGN(CdmSessionMap); diff --git a/libwvdrmengine/cdm/core/src/cdm_engine.cpp b/libwvdrmengine/cdm/core/src/cdm_engine.cpp index 81d8dffc..c7bc47d4 100644 --- a/libwvdrmengine/cdm/core/src/cdm_engine.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_engine.cpp @@ -85,7 +85,10 @@ CdmEngine::CdmEngine(FileSystem* file_system, const std::string& spoid) } } -CdmEngine::~CdmEngine() {} +CdmEngine::~CdmEngine() { + AutoLock lock(session_map_lock_); + session_map_.Terminate(); +} CdmResponseType CdmEngine::OpenSession( const CdmKeySystem& key_system, CdmClientPropertySet* property_set, @@ -143,6 +146,7 @@ CdmResponseType CdmEngine::OpenSession( CdmSessionId id = new_session->session_id(); LOGI("CdmEngine::OpenSession: %s", id.c_str()); + AutoLock lock(session_map_lock_); session_map_.Add(id, new_session.release()); if (session_id) *session_id = id; return NO_ERROR; @@ -184,6 +188,7 @@ CdmResponseType CdmEngine::OpenKeySetSession( CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) { LOGI("CdmEngine::CloseSession: %s", session_id.c_str()); + AutoLock lock(session_map_lock_); if (!session_map_.CloseSession(session_id)) { LOGE("CdmEngine::CloseSession: session not found = %s", session_id.c_str()); return SESSION_NOT_FOUND_1; @@ -217,6 +222,7 @@ CdmResponseType CdmEngine::CloseKeySetSession(const CdmKeySetId& key_set_id) { } bool CdmEngine::IsOpenSession(const CdmSessionId& session_id) { + AutoLock lock(session_map_lock_); return session_map_.Exists(session_id); } @@ -1472,6 +1478,7 @@ CdmResponseType CdmEngine::Decrypt(const CdmSessionId& session_id, // else we must be level 1 direct and we don't need to return a buffer. } + AutoLock lock(session_map_lock_); shared_ptr session; if (session_id.empty()) { CdmSessionList sessions; @@ -1592,6 +1599,7 @@ bool CdmEngine::FindSessionForKey(const KeyId& key_id, uint32_t session_sharing_id = Properties::GetSessionSharingId(*session_id); + AutoLock lock(session_map_lock_); CdmSessionList sessions; session_map_.GetSessionList(sessions); @@ -1637,9 +1645,6 @@ void CdmEngine::OnTimerEvent() { Clock clock; uint64_t current_time = clock.GetCurrentTime(); - CdmSessionList sessions; - session_map_.GetSessionList(sessions); - bool usage_update_period_expired = false; if (current_time - last_usage_information_update_time_ > kUpdateUsageInformationPeriod) { @@ -1650,6 +1655,10 @@ void CdmEngine::OnTimerEvent() { bool is_initial_usage_update = false; bool is_usage_update_needed = false; + AutoLock lock(session_map_lock_); + CdmSessionList sessions; + session_map_.GetSessionList(sessions); + while (!sessions.empty()) { is_initial_usage_update = is_initial_usage_update || sessions.front()->is_initial_usage_update(); diff --git a/libwvdrmengine/cdm/core/src/cdm_session_map.cpp b/libwvdrmengine/cdm/core/src/cdm_session_map.cpp index 8f8b2bf0..e94a3173 100644 --- a/libwvdrmengine/cdm/core/src/cdm_session_map.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_session_map.cpp @@ -12,7 +12,10 @@ namespace wvcdm { CdmSessionMap::~CdmSessionMap() { - AutoLock lock(lock_); + Terminate(); +} + +void CdmSessionMap::Terminate() { for (CdmIdToSessionMap::iterator i = sessions_.begin(); i != sessions_.end(); ++i) { i->second->Close(); @@ -22,12 +25,10 @@ CdmSessionMap::~CdmSessionMap() { } void CdmSessionMap::Add(const std::string& id, CdmSession* session) { - AutoLock lock(lock_); sessions_[id].reset(session); } bool CdmSessionMap::CloseSession(const std::string& id) { - AutoLock lock(lock_); shared_ptr session; if (!FindSessionNoLock(id, &session)) { return false; @@ -38,13 +39,11 @@ bool CdmSessionMap::CloseSession(const std::string& id) { } bool CdmSessionMap::Exists(const std::string& id) { - AutoLock lock(lock_); return sessions_.find(id) != sessions_.end(); } bool CdmSessionMap::FindSession(const CdmSessionId& id, shared_ptr* session) { - AutoLock lock(lock_); return FindSessionNoLock(id, session); } @@ -61,7 +60,6 @@ bool CdmSessionMap::FindSessionNoLock(const CdmSessionId& session_id, void CdmSessionMap::GetSessionList(CdmSessionList& sessions) { sessions.clear(); - AutoLock lock(lock_); for (CdmIdToSessionMap::iterator iter = sessions_.begin(); iter != sessions_.end(); ++iter) { if (!(iter->second)->IsClosed()) {