From d7eab65c8ccc2b8ad450339e2d8f280474ad4b80 Mon Sep 17 00:00:00 2001 From: Alex Dale Date: Wed, 2 Jul 2025 14:42:52 -0700 Subject: [PATCH] Fixed dangling pointer issue in CdmInfo. [ Cherry-pick of http://ag/34331848 ] [ Merge of http://go/wvgerrit/224951 ] There was a potential dangling pointer issue that was enabled by how CdmInfo is initilized. The file system that was passed into the CdmEngine instance was pointing to a location in memory that was not stable between move operations in the CdmInfo. See b/429054262 for memory diagram of issue. The CdmInfo is a private class within the Android CDM class, which restricts the potential operations on it. The easiest solution is wrap the file system in a unique pointer; ensuring the pointer remains stable even if a particular data segment of CdmInfo is moved. The default constructor for CdmInfo is deleted; this will force the compiler to fail if |cdms_| is used in ways that would result in uninitialized pointers. Bug: 429054262 Test: WvTs on Komodo Change-Id: I76a49fc5181ebd1613e238aa49986083a9f397ec (cherry picked from commit 4c105faa4923bd9bd6352f757dedf3eaf9ed88fd) --- .../include/wv_content_decryption_module.h | 26 +++++++-- .../cdm/src/wv_content_decryption_module.cpp | 57 +++++++++++++------ 2 files changed, 61 insertions(+), 22 deletions(-) diff --git a/libwvdrmengine/cdm/include/wv_content_decryption_module.h b/libwvdrmengine/cdm/include/wv_content_decryption_module.h index 50cb0582..c4ad340f 100644 --- a/libwvdrmengine/cdm/include/wv_content_decryption_module.h +++ b/libwvdrmengine/cdm/include/wv_content_decryption_module.h @@ -299,11 +299,29 @@ class WvContentDecryptionModule : public android::RefBase, public TimerHandler { const std::string& signature); private: - struct CdmInfo { - CdmInfo(); + class CdmInfo { + public: + // This should never be used. + CdmInfo() = delete; + // It is expected that the filesystem loaded into |cdm_engine| + // is the same instance as |file_system|. + CdmInfo(std::unique_ptr&& file_system, + std::unique_ptr&& cdm_engine); + // No copy operators. + CdmInfo(const CdmInfo&) = delete; + CdmInfo& operator=(const CdmInfo&) = delete; + // Move operators OK. + CdmInfo(CdmInfo&&) = default; + CdmInfo& operator==(CdmInfo&& other); - wvutil::FileSystem file_system; - std::unique_ptr cdm_engine; + wvutil::FileSystem* file_system() { return file_system_.get(); } + CdmEngine* cdm_engine() { return cdm_engine_.get(); } + + private: + // Order matters, |cdm_engine_| is expected to contain a pointer + // to |file_system_|. + std::unique_ptr file_system_; + std::unique_ptr cdm_engine_; }; // Finds the CdmEngine instance for the given identifier, creating one if diff --git a/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp b/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp index 11e62770..e0857c7d 100644 --- a/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp +++ b/libwvdrmengine/cdm/src/wv_content_decryption_module.cpp @@ -566,7 +566,7 @@ CdmResponseType WvContentDecryptionModule::GetCurrentMetricsInternal( // TODO(blueeyes): Add a better error. return CdmResponseType(UNKNOWN_ERROR); } - return it->second.cdm_engine->GetMetricsSnapshot(metrics) + return it->second.cdm_engine()->GetMetricsSnapshot(metrics) ? CdmResponseType(NO_ERROR) : CdmResponseType(UNKNOWN_ERROR); } @@ -582,31 +582,52 @@ void WvContentDecryptionModule::SaveMetrics( WvMetricsSnapshot::MakeSnapshot(identifier, std::move(metrics))); } -WvContentDecryptionModule::CdmInfo::CdmInfo() - : cdm_engine(CdmEngineFactory::CreateCdmEngine(&file_system)) {} +WvContentDecryptionModule::CdmInfo::CdmInfo( + std::unique_ptr&& file_system, + std::unique_ptr&& cdm_engine) + : file_system_(std::move(file_system)), + cdm_engine_(std::move(cdm_engine)) {} + +WvContentDecryptionModule::CdmInfo& +WvContentDecryptionModule::CdmInfo::operator==( + WvContentDecryptionModule::CdmInfo&& other) { + // Must move |cdm_engine_| first; the current value depends + // on the current value of |file_system_|. + cdm_engine_ = std::move(other.cdm_engine_); + file_system_ = std::move(other.file_system_); + return *this; +} CdmEngine* WvContentDecryptionModule::EnsureCdmForIdentifier( const CdmIdentifier& identifier) { - CdmEngine* cdm_engine; + CdmEngine* cdm_engine = nullptr; bool enable_timer = false; { std::unique_lock auto_lock(cdms_lock_); - if (cdms_.size() == 0) enable_timer = true; - if (cdms_.find(identifier) == cdms_.end()) { - // Accessing the map entry will create a new instance using the default - // constructor. We then need to provide it with two pieces of info: The + if (cdms_.empty()) enable_timer = true; + auto it = cdms_.find(identifier); + if (it != cdms_.end()) { + // Already exists. + cdm_engine = it->second.cdm_engine(); + } else { + std::unique_ptr fs = + std::make_unique(); + // Provide it with two pieces of info: The // origin provided by the app and an identifier that uniquely identifies // this CDM. We concatenate all pieces of the CdmIdentifier in order to // create an ID that is unique to that identifier. - cdms_[identifier].file_system.set_origin(identifier.origin); - cdms_[identifier].file_system.set_identifier(identifier.spoid + - identifier.origin); - cdms_[identifier].cdm_engine->SetAppPackageName( - identifier.app_package_name); - cdms_[identifier].cdm_engine->SetSpoid(identifier.spoid); - cdms_[identifier].cdm_engine->SetUserId(identifier.user_id); + fs->set_origin(identifier.origin); + fs->set_identifier(identifier.spoid + identifier.origin); + + std::unique_ptr engine( + CdmEngineFactory::CreateCdmEngine(fs.get())); + engine->SetAppPackageName(identifier.app_package_name); + engine->SetSpoid(identifier.spoid); + engine->SetUserId(identifier.user_id); + + cdm_engine = engine.get(); + cdms_.emplace(identifier, CdmInfo(std::move(fs), std::move(engine))); } - cdm_engine = cdms_[identifier].cdm_engine.get(); } // Do not enable timer while holding on to the |cdms_lock_| if (enable_timer) EnableTimer(); @@ -649,7 +670,7 @@ CdmResponseType WvContentDecryptionModule::CloseCdm( // TODO(blueeyes): Create a better error. return CdmResponseType(UNKNOWN_ERROR); } - CdmEngine* cdm_engine = cdm_it->second.cdm_engine.get(); + CdmEngine* cdm_engine = cdm_it->second.cdm_engine(); // Save metrics snapshot. drm_metrics::WvCdmMetrics metrics; const bool success = cdm_engine->GetMetricsSnapshot(&metrics); @@ -738,7 +759,7 @@ void WvContentDecryptionModule::OnTimerEvent() { std::unique_lock auto_lock(cdms_lock_); for (auto it = cdms_.begin(); it != cdms_.end(); ++it) { LoggingUidSetter set_uid(it->first.user_id); - it->second.cdm_engine->OnTimerEvent(); + it->second.cdm_engine()->OnTimerEvent(); } if (cdms_.empty()) { // The following code cannot be attributed to any app uid.