From dfc5909d0c83bd57e44d1f6018ce22d9c6a088c6 Mon Sep 17 00:00:00 2001 From: "John W. Bruce" Date: Fri, 25 Jan 2019 14:48:00 -0800 Subject: [PATCH] Segfault When Running Jenkins Tests... Sometimes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (This is a merge of http://go/wvgerrit/71330) The Service Certificate unit tests actually relied on the ability to call Properties::Init() multiple times to clear previous mutable state. Unfortunately, they didn't check the return code that could have told them their mutable state wasn't being cleared and instead proceeded to use a pointer which — depending on compiler — could be totally valid and allow the test to pass or could be invalid and cause a segfault. You can read the bug for a fuller explanation of the mechanics. The fix is twofold. First, the tests will now assert out if insertion into the property set map fails, preventing segfaults. Second, a helper has been added to Properties that allows tests interested in re-initializing Properties to do so. The default behavior for most tests remains the same: Properties can only be initialized once and subsequent calls to Properties::Init() are ignored. This patch also fixed a few formatting issues I noticed. Bug: 123099779 Test: Jenkins Unit Tests w/ GCC Test: CE CDM Unit Tests w/ GCC & Clang Change-Id: Ifd29f3ddf5cff934933cf47b92ecd12ab0a4a938 --- libwvdrmengine/cdm/core/include/properties.h | 16 ++++++++- .../test/service_certificate_unittest.cpp | 33 ++++++++++++------- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/properties.h b/libwvdrmengine/cdm/core/include/properties.h index 8840f07c..22efdfb6 100644 --- a/libwvdrmengine/cdm/core/include/properties.h +++ b/libwvdrmengine/cdm/core/include/properties.h @@ -85,6 +85,21 @@ class Properties { CdmClientPropertySet* property_set); static bool RemoveSessionPropertySet(const CdmSessionId& session_id); + protected: + // This function always runs the code in |Init()| (and subsequently + // |InitOnce()|) even if Properties have already been initialized. This is + // needed by certain tests that are dependent on controlling the mutable state + // of Properties. Should not be used in general, as most tests rely on + // Properties' normal guarantee about |Init()| being safe to call multiple + // times without destroying mutable state. + static void ForceReinit() { + { + std::unique_lock lock(init_mutex_); + is_initialized_ = false; + } + Init(); + } + private: static CdmClientPropertySet* GetCdmClientPropertySet( const CdmSessionId& session_id); @@ -111,7 +126,6 @@ class Properties { FRIEND_TEST(CdmLicenseTest, PrepareKeyRequestValidation); #endif - private: // Called at least once before any properties are used. static void InitOnce(); diff --git a/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp b/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp index bd936d87..aecbc92a 100644 --- a/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp @@ -41,6 +41,11 @@ const std::string kTestSignedCertificate = a2bs_hex( "D2E8D5986104AACC4DD475FD96EE9CE4E326F21B83C7058577B38732CDDABC6A6BED13FB0D" "49D38A45EB87A5F4"); +class PropertiesTestPeer : public Properties { + public: + using Properties::ForceReinit; +}; + } // unnamed namespace class ServiceCertificateTest : public ::testing::Test { @@ -75,7 +80,9 @@ class StubCdmClientPropertySet : public CdmClientPropertySet { uint32_t session_sharing_id() const override { return session_sharing_id_; } - void set_session_sharing_id(uint32_t id) override { session_sharing_id_ = id; } + void set_session_sharing_id(uint32_t id) override { + session_sharing_id_ = id; + } const std::string& app_id() const override { return app_id_; } @@ -100,8 +107,9 @@ TEST_F(ServiceCertificateTest, InitPrivacyModeRequired) { property_set.enable_privacy_mode(); - Properties::Init(); - Properties::AddSessionPropertySet(kTestSessionId1, &property_set); + PropertiesTestPeer::ForceReinit(); + ASSERT_TRUE(PropertiesTestPeer::AddSessionPropertySet(kTestSessionId1, + &property_set)); service_certificate_.Init(kTestSessionId1); EXPECT_FALSE(service_certificate_.has_certificate()); @@ -113,14 +121,14 @@ TEST_F(ServiceCertificateTest, InitServiceCertificatePresent) { property_set.enable_privacy_mode(); property_set.set_service_certificate(kTestSignedCertificate); - Properties::Init(); - Properties::AddSessionPropertySet(kTestSessionId1, &property_set); + PropertiesTestPeer::ForceReinit(); + ASSERT_TRUE(PropertiesTestPeer::AddSessionPropertySet(kTestSessionId1, + &property_set)); std::string raw_service_certificate; - EXPECT_TRUE(Properties::GetServiceCertificate(kTestSessionId1, - &raw_service_certificate)); - EXPECT_EQ(NO_ERROR, - service_certificate_.Init(raw_service_certificate)); + EXPECT_TRUE(PropertiesTestPeer::GetServiceCertificate( + kTestSessionId1, &raw_service_certificate)); + EXPECT_EQ(NO_ERROR, service_certificate_.Init(raw_service_certificate)); EXPECT_TRUE(service_certificate_.has_certificate()); } @@ -129,11 +137,12 @@ TEST_F(ServiceCertificateTest, SetServiceCertificate) { property_set.enable_privacy_mode(); - Properties::Init(); - Properties::AddSessionPropertySet(kTestSessionId1, &property_set); + PropertiesTestPeer::ForceReinit(); + ASSERT_TRUE(PropertiesTestPeer::AddSessionPropertySet(kTestSessionId1, + &property_set)); EXPECT_EQ(NO_ERROR, service_certificate_.Init(kTestSignedCertificate)); EXPECT_TRUE(service_certificate_.has_certificate()); } -} +} // namespace wvcdm