From 802fe3b35c42975cad30b947903e002500f2a6c7 Mon Sep 17 00:00:00 2001 From: Fred Gylys-Colwell Date: Fri, 6 Feb 2015 19:13:30 -0800 Subject: [PATCH] Lock session list in CdmEngine OnTimerEvent This is a copy of the widevine CL: https://widevine-internal-review.googlesource.com/#/c/12742/ If a session is closed at the same time as an OnTimerEvent is processing an event, there could be a race condition between the two threads. This CL adds a lock that prevents a session from being removed from the list while the timer is currently processing an event. If CloseSession is called while the OnTimerEvent method is active, the session will be added to a dead list, and deleted when the timer event has finished. This CL does not address the main problem in bug 19252886, but one bugreport, netflix_log_3.txt, indicates there may have been a problem with the CDM timer. bug: 19252886 Change-Id: I17190edaeb3eef1295d4d204232cc4262cb5fa9b --- libwvdrmengine/cdm/core/include/cdm_engine.h | 7 +++ libwvdrmengine/cdm/core/src/cdm_engine.cpp | 45 ++++++++++++++++---- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/cdm_engine.h b/libwvdrmengine/cdm/core/include/cdm_engine.h index 5d1a7ff2..4da639ee 100644 --- a/libwvdrmengine/cdm/core/include/cdm_engine.h +++ b/libwvdrmengine/cdm/core/include/cdm_engine.h @@ -9,6 +9,7 @@ #include "certificate_provisioning.h" #include "crypto_session.h" #include "initialization_data.h" +#include "lock.h" #include "oemcrypto_adapter.h" #include "scoped_ptr.h" #include "wv_cdm_types.h" @@ -154,6 +155,12 @@ class CdmEngine { scoped_ptr usage_property_set_; int64_t last_usage_information_update_time_; + Lock session_list_lock_; + // If the timer is currently active -- do not delete any sessions. + bool timer_event_active_; + // Sessions to be deleted after the timer has finished. + std::vector dead_sessions_; + CORE_DISALLOW_COPY_AND_ASSIGN(CdmEngine); }; diff --git a/libwvdrmengine/cdm/core/src/cdm_engine.cpp b/libwvdrmengine/cdm/core/src/cdm_engine.cpp index 905cb0c4..c470b704 100644 --- a/libwvdrmengine/cdm/core/src/cdm_engine.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_engine.cpp @@ -58,7 +58,8 @@ CdmEngine::CdmEngine() : cert_provisioning_(NULL), cert_provisioning_requested_security_level_(kLevelDefault), usage_session_(NULL), - last_usage_information_update_time_(0) { + last_usage_information_update_time_(0), + timer_event_active_(false) { Properties::Init(); if (!seeded_) { Clock clock; @@ -68,6 +69,7 @@ CdmEngine::CdmEngine() } CdmEngine::~CdmEngine() { + AutoLock lock(session_list_lock_); CdmSessionMap::iterator i(sessions_.begin()); for (; i != sessions_.end(); ++i) { delete i->second; @@ -108,6 +110,7 @@ CdmResponseType CdmEngine::OpenSession( return sts; } *session_id = new_session->session_id(); + AutoLock lock(session_list_lock_); sessions_[*session_id] = new_session.release(); return NO_ERROR; } @@ -134,7 +137,7 @@ CdmResponseType CdmEngine::OpenKeySetSession( CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) { LOGI("CdmEngine::CloseSession"); - + AutoLock lock(session_list_lock_); CdmSessionMap::iterator iter = sessions_.find(session_id); if (iter == sessions_.end()) { LOGE("CdmEngine::CloseSession: session not found = %s", session_id.c_str()); @@ -143,7 +146,9 @@ CdmResponseType CdmEngine::CloseSession(const CdmSessionId& session_id) { CdmSession* session = iter->second; sessions_.erase(session_id); - delete session; + // If the event timer is active, it will delete this session when it's done. + if (timer_event_active_) dead_sessions_.push_back(session); + else delete session; return NO_ERROR; } @@ -905,26 +910,38 @@ void CdmEngine::OnTimerEvent() { bool is_initial_usage_update = false; bool is_usage_update_needed = false; + // Make a copy of all currently open sessions. While this event handler is + // active, after we release session_list_lock_, CloseSession and OpenSession + // might modify sessions_, but they will not delete sessions. + session_list_lock_.Acquire(); + const size_t session_count = sessions_.size(); + CdmSession* event_sessions[session_count]; + timer_event_active_ = true; + size_t index = 0; for (CdmSessionMap::iterator iter = sessions_.begin(); - iter != sessions_.end(); ++iter) { + iter != sessions_.end() && index < session_count; ++iter) { is_initial_usage_update = is_initial_usage_update || iter->second->is_initial_usage_update(); is_usage_update_needed = is_usage_update_needed || iter->second->is_usage_update_needed(); + event_sessions[index++] = iter->second; + } + session_list_lock_.Release(); - iter->second->OnTimerEvent(usage_update_period_expired); + + for (size_t i=0; i < session_count; i++) { + event_sessions[i]->OnTimerEvent(usage_update_period_expired); } if (is_usage_update_needed && (usage_update_period_expired || is_initial_usage_update)) { bool has_usage_been_updated = false; - for (CdmSessionMap::iterator iter = sessions_.begin(); - iter != sessions_.end(); ++iter) { - iter->second->reset_usage_flags(); + for (size_t i=0; i < session_count; i++) { + event_sessions[i]->reset_usage_flags(); if (!has_usage_been_updated) { // usage is updated for all sessions so this needs to be // called only once per update usage information period - CdmResponseType status = iter->second->UpdateUsageInformation(); + CdmResponseType status = event_sessions[i]->UpdateUsageInformation(); if (NO_ERROR != status) { LOGW("Update usage information failed: %d", status); } else { @@ -933,6 +950,16 @@ void CdmEngine::OnTimerEvent() { } } } + { + // We are done with the event handler. Now we look for any sessions that + // were closed. + AutoLock lock(session_list_lock_); + for (size_t i=0; i < dead_sessions_.size(); i++) { + delete dead_sessions_[i]; + } + dead_sessions_.clear(); + timer_event_active_ = false; + } } void CdmEngine::OnKeyReleaseEvent(const CdmKeySetId& key_set_id) {