From b6cdd12394e1538c15fbec9bc51c1afe65a53cf1 Mon Sep 17 00:00:00 2001 From: Alex Dale Date: Thu, 1 Jul 2021 17:45:53 -0700 Subject: [PATCH] Added write protection for session to engine map. [ Merge of http://go/wvgerrit/128325 ] There were a few cases where |cdm_by_session_id_| was being iterated over and the CDM did not acquire any write-protection locks to prevent other threads from changing the map simultaneously. In particular, it was possible that while cleaning up a CDM, and removing all the associated session in |cdm_by_session_id_| another CDM could have been opening a session and creating a new association in |cdm_by_session_id_| at the same time. Cases where |cdms_| and/or |cdm_by_session_id_| is being written to or iteratively read from should require a lock. The iterator of std::map maintains a "view" into the map's tree structure. Modifying the map (inserting or deleting elements) can potentially change the structure of the map and the underlying assumptions built into an iterator's view (ex, the iterator thinking there is an element to the left or right). Modifying the value within the map can potentially cause problems, but is not applicable in our case (we modify the object pointed to by the map element, but not the pointer itself). Bug: 190405462 Test: build_and_run_all_unit_tests.sh and MediaDrmTest Change-Id: I043e238570dac9a0db990f8fe66be271062b965c --- .../include/wv_content_decryption_module.h | 29 ++++++++++++--- .../cdm/src/wv_content_decryption_module.cpp | 36 ++++++++++++------- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/libwvdrmengine/cdm/include/wv_content_decryption_module.h b/libwvdrmengine/cdm/include/wv_content_decryption_module.h index b1a9c8c4..4095a6ca 100644 --- a/libwvdrmengine/cdm/include/wv_content_decryption_module.h +++ b/libwvdrmengine/cdm/include/wv_content_decryption_module.h @@ -215,6 +215,14 @@ class WvContentDecryptionModule : public android::RefBase, public TimerHandler { void DisableTimerAndWaitForExit(); void OnTimerEvent(); + // Fill the |metrics| parameter with the metrics data for the CdmEngine + // associated with the given |identifier|. If the CdmEngine instance does + // not exist, this will return an error. + // This methods assumes that |metrics| is not null and that the |cdms_lock_| + // has already been acquired. + CdmResponseType GetMetricsInternal(const CdmIdentifier& identifier, + drm_metrics::WvCdmMetrics* metrics); + static std::mutex session_sharing_id_generation_lock_; std::mutex timer_lock_; Timer timer_; @@ -222,14 +230,27 @@ class WvContentDecryptionModule : public android::RefBase, public TimerHandler { // instance variables // This manages the lifetime of the CDM instances. std::map cdms_; - std::mutex cdms_lock_; - // This contains weak pointers to the CDM instances contained in |cdms_|. std::map cdm_by_session_id_; + // Lock for accessing either |cdms_| or |cdm_by_session_id_|. + // This lock should be acquired for any of the following: + // 1) Getting a CDM instance from |cdms_| or |cdm_by_session_id_| + // - Hold lock while searching, release lock once pointer is acquired + // 2) Iterating over |cdms_| or |cdm_by_session_id_| + // - Hold lock for the entire duration of iterating over CDMs + // - DO NOT release the lock until all operations are complete + // - This MUST be done regardless of whether |cdms_| or + // |cdm_by_session_id_| is being modified + // 3) Creating a new CDM instance with |cdms_| + // - Hold lock when creating AND initializing the CDM + // - Release once CDM is fully initialized + // 4) Linking a session to a CDM with |cdm_by_session_id_| + // - Hold lock when mapping a session ID to a CDM, release once set + // 5) Unlinking a session from a CDM with |cdm_by_session_id_| + // - Hold lock when erasing, release once erased. + std::mutex cdms_lock_; CORE_DISALLOW_COPY_AND_ASSIGN(WvContentDecryptionModule); }; - } // namespace wvcdm - #endif // CDM_BASE_WV_CONTENT_DECRYPTION_MODULE_H_ diff --git a/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp b/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp index b6645127..53763827 100644 --- a/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp +++ b/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp @@ -71,6 +71,7 @@ CdmResponseType WvContentDecryptionModule::OpenSession( CdmResponseType sts = cdm_engine->OpenSession(key_system, property_set, event_listener, session_id); if (sts == NO_ERROR) { + std::unique_lock auto_lock(cdms_lock_); cdm_by_session_id_[*session_id] = cdm_engine; } return sts; @@ -82,12 +83,11 @@ CdmResponseType WvContentDecryptionModule::CloseSession( CdmEngine* cdm_engine = GetCdmForSessionId(session_id); // TODO(rfrias): Avoid reusing the error codes from CdmEngine. if (!cdm_engine) return SESSION_NOT_FOUND_1; - std::unique_lock auto_lock(cdms_lock_); - CdmResponseType sts = cdm_engine->CloseSession(session_id); + const CdmResponseType sts = cdm_engine->CloseSession(session_id); if (sts == NO_ERROR) { + std::unique_lock auto_lock(cdms_lock_); cdm_by_session_id_.erase(session_id); } - return sts; } @@ -107,6 +107,7 @@ CdmResponseType WvContentDecryptionModule::GenerateKeyRequest( if (license_type == kLicenseTypeRelease) { sts = cdm_engine->OpenKeySetSession(key_set_id, property_set, nullptr); if (sts != NO_ERROR) return sts; + std::unique_lock auto_lock(cdms_lock_); cdm_by_session_id_[key_set_id] = cdm_engine; } @@ -130,6 +131,7 @@ CdmResponseType WvContentDecryptionModule::GenerateKeyRequest( app_parameters, key_request); if (license_type == kLicenseTypeRelease && sts != KEY_MESSAGE) { cdm_engine->CloseKeySetSession(key_set_id); + std::unique_lock auto_lock(cdms_lock_); cdm_by_session_id_.erase(key_set_id); } return sts; @@ -152,6 +154,7 @@ CdmResponseType WvContentDecryptionModule::AddKey( // Empty session id indicates license type release. if (sts == KEY_ADDED && session_id.empty()) { cdm_engine->CloseKeySetSession(release_key_set_id); + std::unique_lock auto_lock(cdms_lock_); cdm_by_session_id_.erase(release_key_set_id); } return sts; @@ -297,11 +300,11 @@ CdmResponseType WvContentDecryptionModule::GetSecureStopIds( } CdmEngine* cdm_engine = EnsureCdmForIdentifier(identifier); - CdmResponseType sts = + const CdmResponseType sts = cdm_engine->ListUsageIds(app_id, kSecurityLevelL1, nullptr, ssids); std::vector secure_stop_ids; - CdmResponseType sts_l3 = cdm_engine->ListUsageIds(app_id, kSecurityLevelL3, - nullptr, &secure_stop_ids); + const CdmResponseType sts_l3 = cdm_engine->ListUsageIds( + app_id, kSecurityLevelL3, nullptr, &secure_stop_ids); ssids->insert(ssids->end(), secure_stop_ids.begin(), secure_stop_ids.end()); return sts_l3 != NO_ERROR ? sts_l3 : sts; } @@ -383,17 +386,19 @@ CdmResponseType WvContentDecryptionModule::GetMetrics( if (!metrics || !full_list_returned) { return PARAMETER_NULL; } + std::unique_lock auto_lock(cdms_lock_); for (auto& key_value_pair : cdms_) { + const CdmIdentifier& identifier = key_value_pair.first; drm_metrics::WvCdmMetrics metric; - CdmResponseType status = GetMetrics(key_value_pair.first, &metric); + const CdmResponseType status = GetMetricsInternal(identifier, &metric); if (status == NO_ERROR) { metrics->push_back(metric); } else { LOGD("GetMetrics call failed: cdm identifier=%u, error=%d", - key_value_pair.first.unique_id, status); + identifier.unique_id, status); } } - // With no streaming activities, cdms_ size would be zero, + // With no streaming activities, |cdms_| size would be zero, // treat it as a non full list in that case. *full_list_returned = !cdms_.empty() && metrics->size() == cdms_.size(); @@ -410,6 +415,12 @@ CdmResponseType WvContentDecryptionModule::GetMetrics( return PARAMETER_NULL; } std::unique_lock auto_lock(cdms_lock_); + return GetMetricsInternal(identifier, metrics); +} + +CdmResponseType WvContentDecryptionModule::GetMetricsInternal( + const CdmIdentifier& identifier, drm_metrics::WvCdmMetrics* metrics) { + // Note: Caller should lock before calling. auto it = cdms_.find(identifier); if (it == cdms_.end()) { LOGE("Cdm Identifier not found"); @@ -454,6 +465,7 @@ CdmEngine* WvContentDecryptionModule::EnsureCdmForIdentifier( CdmEngine* WvContentDecryptionModule::GetCdmForSessionId( const std::string& session_id) { + std::unique_lock auto_lock(cdms_lock_); // Use find to avoid creating empty entries when not found. auto it = cdm_by_session_id_.find(session_id); if (it == cdm_by_session_id_.end()) return nullptr; @@ -462,10 +474,8 @@ CdmEngine* WvContentDecryptionModule::GetCdmForSessionId( void WvContentDecryptionModule::CloseAllCdms() { std::unique_lock auto_lock(cdms_lock_); - - for (auto it = cdms_.begin(); it != cdms_.end();) { - it = cdms_.erase(it); - } + cdm_by_session_id_.clear(); + cdms_.clear(); } CdmResponseType WvContentDecryptionModule::CloseCdm(