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
This commit is contained in:
Alex Dale
2021-07-01 17:45:53 -07:00
parent a3657ab200
commit b6cdd12394
2 changed files with 48 additions and 17 deletions

View File

@@ -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<CdmIdentifier, CdmInfo> cdms_;
std::mutex cdms_lock_;
// This contains weak pointers to the CDM instances contained in |cdms_|.
std::map<std::string, CdmEngine*> 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_

View File

@@ -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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<CdmSecureStopId> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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<std::mutex> 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(