From 8574a4b98cd1e88d66655f3a6d083f0f26fd6b77 Mon Sep 17 00:00:00 2001 From: "John \"Juce\" Bruce" Date: Wed, 1 Dec 2021 11:42:19 -0800 Subject: [PATCH 1/4] Update handling of move-only types in DeviceFiles unit test (This is a merge of http://go/wvgerrit/139569.) The unit test for DeviceFiles previously had to work around googlemock's lack of support for move-only types. Now that we have upgraded to googletest 1.10, we can use move-only types directly via the ByMove() utility, removing the need for this workaround. Bug: 207693599 Test: x86-64 Change-Id: Ib4dcc5ec367ef413465a3e8a8f45f9187976ed5e --- .../cdm/core/test/device_files_unittest.cpp | 165 +++++++++--------- 1 file changed, 86 insertions(+), 79 deletions(-) diff --git a/libwvdrmengine/cdm/core/test/device_files_unittest.cpp b/libwvdrmengine/cdm/core/test/device_files_unittest.cpp index 0eba423b..7a1376ce 100644 --- a/libwvdrmengine/cdm/core/test/device_files_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/device_files_unittest.cpp @@ -3756,15 +3756,7 @@ class MockFileSystem : public FileSystem { MockFileSystem() {} ~MockFileSystem() {} - // Until gmock is updated to a version post-April 2017, we need this - // workaround to test functions that take or return smart pointers. - // See - // https://github.com/abseil/googletest/blob/master/googlemock/docs/CookBook.md#legacy-workarounds-for-move-only-types - std::unique_ptr Open(const std::string& buffer, int flags) { - return std::unique_ptr(DoOpen(buffer, flags)); - } - - MOCK_METHOD2(DoOpen, File*(const std::string&, int flags)); + MOCK_METHOD2(Open, std::unique_ptr(const std::string&, int flags)); MOCK_METHOD0(IsFactoryReset, bool()); MOCK_METHOD1(Exists, bool(const std::string&)); @@ -3780,6 +3772,7 @@ using ::testing::_; using ::testing::AllArgs; using ::testing::AllOf; using ::testing::AtLeast; +using ::testing::ByMove; using ::testing::DoAll; using ::testing::Eq; using ::testing::Expectation; @@ -3931,8 +3924,8 @@ TEST_P(StoreCertificateTest, DefaultAndLegacy) { // Call to Open will return a unique_ptr, freeing this object. MockFile* file = new MockFile(); EXPECT_CALL(file_system, - DoOpen(StrEq(device_certificate_path), IsCreateFileFlagSet())) - .WillOnce(Return(file)); + Open(StrEq(device_certificate_path), IsCreateFileFlagSet())) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains( std::vector{certificate, private_key.key()}))) @@ -3979,8 +3972,8 @@ TEST_F(DeviceFilesTest, RetrieveAtscCertificate) { .WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(device_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_certificate_path), _)) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(device_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4051,9 +4044,9 @@ TEST_F(DeviceFilesTest, RetrieveLegacyCertificateWithoutExpirationTime) { .WillOnce(Return(data.size())); // Retrieving the legacy license will cause a read as well as a write // to fill in a random expiry date ~6 months later if one has not been set - EXPECT_CALL(file_system, DoOpen(StrEq(device_legacy_certificate_path), _)) - .WillOnce(Return(read_file)) - .WillOnce(Return(write_file)); + EXPECT_CALL(file_system, Open(StrEq(device_legacy_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))) + .WillOnce(Return(ByMove(std::unique_ptr(write_file)))); EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4103,8 +4096,8 @@ TEST_F(DeviceFilesTest, RetrieveLegacyCertificateWithClientExpirationTime) { .WillRepeatedly(Return(false)); EXPECT_CALL(file_system, FileSize(StrEq(device_legacy_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_legacy_certificate_path), _)) - .WillOnce(Return(read_file)); + EXPECT_CALL(file_system, Open(StrEq(device_legacy_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))); EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4157,8 +4150,8 @@ TEST_P(RetrieveLegacyCertificateTest, ErrorScenarios) { .WillRepeatedly(Return(false)); EXPECT_CALL(file_system, FileSize(StrEq(device_legacy_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_legacy_certificate_path), _)) - .WillOnce(Return(read_file)); + EXPECT_CALL(file_system, Open(StrEq(device_legacy_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))); EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4203,8 +4196,8 @@ TEST_F(DeviceFilesTest, RetrieveDefaultCertificate) { .WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(device_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_certificate_path), _)) - .WillOnce(Return(read_file)); + EXPECT_CALL(file_system, Open(StrEq(device_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))); EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4244,8 +4237,8 @@ TEST_F(DeviceFilesTest, RetrieveDefaultCertificateNeverExpires) { .WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(device_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_certificate_path), _)) - .WillOnce(Return(read_file)); + EXPECT_CALL(file_system, Open(StrEq(device_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))); EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4291,8 +4284,8 @@ TEST_P(RetrieveDefaultCertificateTest, ErrorScenarios) { .WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(device_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_certificate_path), _)) - .WillOnce(Return(read_file)); + EXPECT_CALL(file_system, Open(StrEq(device_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))); EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), Return(data.size()))); @@ -4348,10 +4341,10 @@ TEST_F(DeviceFilesTest, RetrieveCertificateWithoutKeyType) { .WillRepeatedly(Return(false)); EXPECT_CALL(file_system, FileSize(StrEq(device_legacy_certificate_path))) .WillOnce(Return(data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(device_legacy_certificate_path), _)) - .WillOnce(Return(read_file)); + EXPECT_CALL(file_system, Open(StrEq(device_legacy_certificate_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(read_file)))); /* TODO(b/192430982): Renable expiration of legacy DRM certificates - .WillOnce(Return(write_file)); + .WillOnce(Return(ByMove(std::unique_ptr(write_file)))); */ EXPECT_CALL(*read_file, Read(NotNull(), Eq(data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(data.begin(), data.end()), @@ -4391,7 +4384,7 @@ TEST_F(DeviceFilesTest, HasCertificateAtsc) { EXPECT_CALL(file_system, Exists(StrEq(device_certificate_path))) .WillOnce(Return(false)) .WillOnce(Return(true)); - EXPECT_CALL(file_system, DoOpen(_, _)).Times(0); + EXPECT_CALL(file_system, Open(_, _)).Times(0); DeviceFiles device_files(&file_system); ASSERT_TRUE(device_files.Init(kSecurityLevelL1)); @@ -4412,7 +4405,7 @@ TEST_F(DeviceFilesTest, HasCertificateDefault) { EXPECT_CALL(file_system, Exists(StrEq(device_certificate_path))) .WillOnce(Return(true)); - EXPECT_CALL(file_system, DoOpen(_, _)).Times(0); + EXPECT_CALL(file_system, Open(_, _)).Times(0); DeviceFiles device_files(&file_system); ASSERT_TRUE(device_files.Init(kSecurityLevelL1)); @@ -4436,7 +4429,7 @@ TEST_F(DeviceFilesTest, HasCertificateLegacy) { .WillOnce(Return(false)); EXPECT_CALL(file_system, Exists(StrEq(device_legacy_certificate_path))) .WillOnce(Return(true)); - EXPECT_CALL(file_system, DoOpen(_, _)).Times(0); + EXPECT_CALL(file_system, Open(_, _)).Times(0); DeviceFiles device_files(&file_system); ASSERT_TRUE(device_files.Init(kSecurityLevelL1)); @@ -4460,7 +4453,7 @@ TEST_F(DeviceFilesTest, HasCertificateNone) { .WillOnce(Return(false)); EXPECT_CALL(file_system, Exists(StrEq(device_legacy_certificate_path))) .WillOnce(Return(false)); - EXPECT_CALL(file_system, DoOpen(_, _)).Times(0); + EXPECT_CALL(file_system, Open(_, _)).Times(0); DeviceFiles device_files(&file_system); ASSERT_TRUE(device_files.Init(kSecurityLevelL1)); @@ -4487,8 +4480,8 @@ TEST_P(DeviceFilesSecurityLevelTest, SecurityLevel) { // Call to Open will return a unique_ptr, freeing this object. MockFile* file = new MockFile(); EXPECT_CALL(file_system, - DoOpen(StrEq(device_certificate_path), IsCreateFileFlagSet())) - .WillOnce(Return(file)); + Open(StrEq(device_certificate_path), IsCreateFileFlagSet())) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains( std::vector{certificate, private_key.key()}))) @@ -4529,8 +4522,8 @@ TEST_P(DeviceFilesStoreTest, StoreLicense) { // Call to Open will return a unique_ptr, freeing this object. MockFile* file = new MockFile(); - EXPECT_CALL(file_system, DoOpen(StrEq(license_path), IsCreateFileFlagSet())) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(license_path), IsCreateFileFlagSet())) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains(expected_substrings))) .WillOnce(ReturnArg<1>()); @@ -4593,8 +4586,8 @@ TEST_F(DeviceFilesTest, StoreLicenses) { // Call to Open will return a unique_ptr, freeing this object. MockFile* file = new MockFile(); - EXPECT_CALL(file_system, DoOpen(StrEq(license_path), IsCreateFileFlagSet())) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(license_path), IsCreateFileFlagSet())) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains(expected_substrings))) @@ -4649,8 +4642,8 @@ TEST_F(DeviceFilesTest, RetrieveLicenses) { .WillOnce(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(license_path))) .WillOnce(Return(size)); - EXPECT_CALL(file_system, DoOpen(StrEq(license_path), _)) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(license_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(size))) .WillOnce( DoAll(SetArrayArgument<0>(kLicenseTestData[i].file_data.begin(), @@ -4720,8 +4713,8 @@ TEST_F(DeviceFilesTest, AppParametersBackwardCompatibility) { EXPECT_CALL(file_system, Exists(StrEq(license_path))).WillOnce(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(license_path))) .WillOnce(Return(size)); - EXPECT_CALL(file_system, DoOpen(StrEq(license_path), _)) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(license_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(size))) .WillOnce(DoAll(SetArrayArgument<0>(test_data->file_data.begin(), test_data->file_data.end()), @@ -4768,8 +4761,8 @@ TEST_F(DeviceFilesTest, UpdateLicenseState) { for (size_t i = 0; i < ArraySize(kLicenseUpdateTestData); i++) { // Call to Open will return a unique_ptr, freeing this object. MockFile* file = new MockFile(); - EXPECT_CALL(file_system, DoOpen(StrEq(license_path), IsCreateFileFlagSet())) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(license_path), IsCreateFileFlagSet())) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenEq(kLicenseUpdateTestData[i].file_data))) .WillOnce(ReturnArg<1>()); @@ -4814,8 +4807,8 @@ TEST_F(DeviceFilesTest, DeleteLicense) { .WillOnce(Return(false)); EXPECT_CALL(file_system, FileSize(StrEq(license_path))) .WillOnce(Return(size)); - EXPECT_CALL(file_system, DoOpen(StrEq(license_path), _)) - .WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(license_path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(size))) .WillOnce(DoAll(SetArrayArgument<0>(kLicenseTestData[0].file_data.begin(), kLicenseTestData[0].file_data.end()), @@ -4870,7 +4863,7 @@ TEST_F(DeviceFilesTest, DeleteLicense) { TEST_F(DeviceFilesTest, ReserveLicenseIdsDoesNotUseFileSystem) { // Validate that ReserveLicenseIds does not touch the file system. MockFileSystem file_system; - EXPECT_CALL(file_system, DoOpen(_, _)).Times(0); + EXPECT_CALL(file_system, Open(_, _)).Times(0); DeviceFiles device_files(&file_system); EXPECT_TRUE(device_files.Init(kSecurityLevelL1)); @@ -4961,7 +4954,8 @@ TEST_F(DeviceFilesTest, OkpInfo_StoreAndRetrieve) { const std::string kOkpInfoPath = device_base_path_ + DeviceFiles::GetOkpInfoFileName(); MockFile* file = new MockFile(); - EXPECT_CALL(file_system, DoOpen(kOkpInfoPath, _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(kOkpInfoPath, _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); std::string serialized; EXPECT_CALL(*file, Write(NotNull(), _)) .WillOnce(DoAll(Invoke([&](const char* buf, size_t len) { @@ -4977,7 +4971,8 @@ TEST_F(DeviceFilesTest, OkpInfo_StoreAndRetrieve) { EXPECT_CALL(file_system, Exists(kOkpInfoPath)).WillOnce(Return(true)); EXPECT_CALL(file_system, FileSize(kOkpInfoPath)) .WillOnce(Return(serialized.size())); - EXPECT_CALL(file_system, DoOpen(kOkpInfoPath, _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(kOkpInfoPath, _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), _)) .WillOnce(DoAll(SetArrayArgument<0>(serialized.begin(), serialized.end()), Return(serialized.size()))); @@ -5013,8 +5008,8 @@ TEST_P(DeviceFilesDeleteMultipleUsageInfoTest, DeleteAllButOne) { // File read expectations. MockFile* file_in = new MockFile(); - EXPECT_CALL(file_system, DoOpen(file_path, FileSystem::kReadOnly)) - .WillOnce(Return(file_in)); + EXPECT_CALL(file_system, Open(file_path, FileSystem::kReadOnly)) + .WillOnce(Return(ByMove(std::unique_ptr(file_in)))); Expectation read_original = EXPECT_CALL(*file_in, Read(NotNull(), _)) .WillOnce( @@ -5026,8 +5021,8 @@ TEST_P(DeviceFilesDeleteMultipleUsageInfoTest, DeleteAllButOne) { // File write expectations. MockFile* file_out = new MockFile(); EXPECT_CALL(file_system, - DoOpen(file_path, FileSystem::kCreate | FileSystem::kTruncate)) - .WillOnce(Return(file_out)); + Open(file_path, FileSystem::kCreate | FileSystem::kTruncate)) + .WillOnce(Return(ByMove(std::unique_ptr(file_out)))); EXPECT_CALL(*file_out, Write(StrEq(result_hashed_usage_info_file), _)) .After(read_original) .WillOnce(Return(result_hashed_usage_info_file.size())); @@ -5067,8 +5062,8 @@ TEST_F(DeviceFilesDeleteMultipleUsageInfoTest, DeleteAllKeySetIds) { EXPECT_CALL(file_system, Exists(_)).WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(file_path)) .WillOnce(Return(kHashedUsageInfoFileWithThreeKeySetIds.size())); - EXPECT_CALL(file_system, DoOpen(file_path, FileSystem::kReadOnly)) - .WillOnce(Return(file_in)); + EXPECT_CALL(file_system, Open(file_path, FileSystem::kReadOnly)) + .WillOnce(Return(ByMove(std::unique_ptr(file_in)))); EXPECT_CALL(*file_in, Read(NotNull(), _)) .WillOnce(DoAll( SetArrayArgument<0>(kHashedUsageInfoFileWithThreeKeySetIds.cbegin(), @@ -5104,8 +5099,8 @@ TEST_F(DeviceFilesDeleteMultipleUsageInfoTest, DeleteNone) { MockFile* file_in = new MockFile(); EXPECT_CALL(file_system, FileSize(file_path)) .WillOnce(Return(kHashedUsageInfoFileWithThreeKeySetIds.size())); - EXPECT_CALL(file_system, DoOpen(file_path, FileSystem::kReadOnly)) - .WillOnce(Return(file_in)); + EXPECT_CALL(file_system, Open(file_path, FileSystem::kReadOnly)) + .WillOnce(Return(ByMove(std::unique_ptr(file_in)))); EXPECT_CALL(*file_in, Read(NotNull(), _)) .WillOnce(DoAll( SetArrayArgument<0>(kHashedUsageInfoFileWithThreeKeySetIds.cbegin(), @@ -5140,8 +5135,8 @@ TEST_F(DeviceFilesDeleteMultipleUsageInfoTest, DeleteOne) { // File read expectations. MockFile* file_in = new MockFile(); - EXPECT_CALL(file_system, DoOpen(file_path, FileSystem::kReadOnly)) - .WillOnce(Return(file_in)); + EXPECT_CALL(file_system, Open(file_path, FileSystem::kReadOnly)) + .WillOnce(Return(ByMove(std::unique_ptr(file_in)))); Expectation read_original = EXPECT_CALL(*file_in, Read(NotNull(), _)) .WillOnce(DoAll( @@ -5152,8 +5147,8 @@ TEST_F(DeviceFilesDeleteMultipleUsageInfoTest, DeleteOne) { // File write expectations. MockFile* file_out = new MockFile(); EXPECT_CALL(file_system, - DoOpen(file_path, FileSystem::kCreate | FileSystem::kTruncate)) - .WillOnce(Return(file_out)); + Open(file_path, FileSystem::kCreate | FileSystem::kTruncate)) + .WillOnce(Return(ByMove(std::unique_ptr(file_out)))); EXPECT_CALL(*file_out, Write(StrEq(kHashedUsageInfoFileWithKeySet1), _)) .After(read_original) .WillOnce(Return(kHashedUsageInfoFileWithKeySet1.size())); @@ -5191,8 +5186,8 @@ TEST_F(DeviceFilesDeleteMultipleUsageInfoTest, BadFile) { EXPECT_CALL(file_system, Exists(_)).WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(file_path)) .WillOnce(Return(kHashlessUsageInfoFile.size())); - EXPECT_CALL(file_system, DoOpen(file_path, FileSystem::kReadOnly)) - .WillOnce(Return(file_in)); + EXPECT_CALL(file_system, Open(file_path, FileSystem::kReadOnly)) + .WillOnce(Return(ByMove(std::unique_ptr(file_in)))); EXPECT_CALL(*file_in, Read(NotNull(), _)) .WillOnce(DoAll(SetArrayArgument<0>(kHashlessUsageInfoFile.cbegin(), kHashlessUsageInfoFile.cend()), @@ -5241,7 +5236,8 @@ TEST_F(DeviceFilesUsageInfoTest, ListUsageIds) { EXPECT_CALL(file_system, FileSize(StrEq(path))) .Times(2) .WillRepeatedly(Return(kUsageInfoTestData[index].file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(kUsageInfoTestData[index].file_data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(file_data.begin(), file_data.end()), @@ -5335,7 +5331,8 @@ TEST_P(DeviceFilesUsageInfoTest, Store) { } } - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains(usage_data_fields))) .WillOnce(ReturnArg<1>()); @@ -5367,7 +5364,8 @@ TEST_P(DeviceFilesUsageInfoTest, Retrieve) { EXPECT_CALL(file_system, FileSize(StrEq(path))) .Times(2) .WillRepeatedly(Return(kUsageInfoTestData[index].file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(kUsageInfoTestData[index].file_data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(file_data.begin(), file_data.end()), @@ -5434,7 +5432,8 @@ TEST_P(DeviceFilesUsageInfoTest, ListKeySetIds) { EXPECT_CALL(file_system, FileSize(StrEq(path))) .Times(2) .WillRepeatedly(Return(kUsageInfoTestData[index].file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(kUsageInfoTestData[index].file_data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(file_data.begin(), file_data.end()), @@ -5484,7 +5483,8 @@ TEST_P(DeviceFilesUsageInfoTest, ListProviderSessionTokenIds) { EXPECT_CALL(file_system, FileSize(StrEq(path))) .Times(2) .WillRepeatedly(Return(kUsageInfoTestData[index].file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(kUsageInfoTestData[index].file_data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(file_data.begin(), file_data.end()), @@ -5541,7 +5541,8 @@ TEST_P(DeviceFilesUsageInfoTest, RetrieveByProviderSessionToken) { EXPECT_CALL(file_system, Exists(StrEq(path))).WillOnce(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(path))) .WillOnce(Return(file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(file_data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(file_data.begin(), file_data.end()), Return(file_data.size()))); @@ -5632,13 +5633,14 @@ TEST_P(DeviceFilesUsageInfoTest, UpdateUsageInfo) { bool write_called = false; if (index < 0) { - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); } else { MockFile* next_file = new MockFile(); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)) + EXPECT_CALL(file_system, Open(StrEq(path), _)) .Times(2) - .WillOnce(Return(file)) - .WillOnce(Return(next_file)); + .WillOnce(Return(ByMove(std::unique_ptr(file)))) + .WillOnce(Return(ByMove(std::unique_ptr(next_file)))); ON_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains(usage_data_fields))) .WillByDefault(DoAll(InvokeWithoutArgs([&write_called]() -> void { @@ -5678,7 +5680,8 @@ TEST_P(DeviceFilesHlsAttributesTest, Read) { EXPECT_CALL(file_system, Exists(StrEq(path))).WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(path))) .WillRepeatedly(Return(param->file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(param->file_data.size()))) .WillOnce(DoAll( SetArrayArgument<0>(param->file_data.begin(), param->file_data.end()), @@ -5707,7 +5710,8 @@ TEST_P(DeviceFilesHlsAttributesTest, Store) { DeviceFiles::GetHlsAttributesFileNameExtension(); EXPECT_CALL(file_system, Exists(StrEq(path))).WillRepeatedly(Return(true)); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs( StrAndLenContains(std::vector{param->media_segment_iv}))) @@ -5758,7 +5762,8 @@ TEST_P(DeviceFilesUsageTableTest, Store) { std::string path = device_base_path_ + DeviceFiles::GetUsageTableFileName(); EXPECT_CALL(file_system, Exists(StrEq(path))).WillRepeatedly(Return(true)); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Write(_, _)) .With(AllArgs(StrAndLenContains(entry_data))) .WillOnce(ReturnArg<1>()); @@ -5782,7 +5787,8 @@ TEST_P(DeviceFilesUsageTableTest, Read) { EXPECT_CALL(file_system, Exists(StrEq(path))).WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(path))) .WillRepeatedly(Return(file_data.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); EXPECT_CALL(*file, Read(NotNull(), Eq(file_data.size()))) .WillOnce(DoAll(SetArrayArgument<0>(file_data.begin(), file_data.end()), Return(file_data.size()))); @@ -5834,7 +5840,8 @@ TEST_F(DeviceFilesUsageTableTest, ReadWithoutLruData) { EXPECT_CALL(file_system, Exists(StrEq(path))).WillRepeatedly(Return(true)); EXPECT_CALL(file_system, FileSize(StrEq(path))) .WillRepeatedly(Return(kUsageTableWithoutLruData.size())); - EXPECT_CALL(file_system, DoOpen(StrEq(path), _)).WillOnce(Return(file)); + EXPECT_CALL(file_system, Open(StrEq(path), _)) + .WillOnce(Return(ByMove(std::unique_ptr(file)))); DeviceFiles device_files(&file_system); EXPECT_TRUE(device_files.Init(kSecurityLevelL1)); From bfd299a4bebe3d42e3c7af70538ac7ae7588f4e6 Mon Sep 17 00:00:00 2001 From: "John \"Juce\" Bruce" Date: Wed, 1 Dec 2021 11:55:32 -0800 Subject: [PATCH 2/4] Add missing override keywords (This is a merge of http://go/wvgerrit/139629.) This patch fixes several places where the override keyword was missing. These were found when future patches that enable stricter checking of the override keyword were enabled. There are two basic categories of missing override: * Destructors found to be overriding a virtual destructor without using the override keyword. * Test methods overriding methods on test-only or mock objects. Some of these were previously marked as virtual, following our pre-C++11 style guidelines, but this is not necessary now that we have override. Bug: 207684988 Test: x86-64 build Change-Id: I09aa499bd3ea80d925e2fc422290d61eb005a769 --- libwvdrmengine/cdm/core/include/policy_timers_v15.h | 2 +- libwvdrmengine/cdm/core/include/policy_timers_v16.h | 2 +- .../cdm/core/test/certificate_provisioning_unittest.cpp | 3 ++- libwvdrmengine/cdm/core/test/crypto_session_unittest.cpp | 2 +- libwvdrmengine/cdm/core/test/device_files_unittest.cpp | 4 ++-- libwvdrmengine/cdm/core/test/ota_keybox_provisioner_test.cpp | 2 +- libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp | 2 +- libwvdrmengine/cdm/core/test/test_base.cpp | 3 ++- libwvdrmengine/cdm/core/test/test_base.h | 2 +- libwvdrmengine/cdm/core/test/usage_table_header_unittest.cpp | 4 ++-- libwvdrmengine/cdm/metrics/test/event_metric_unittest.cpp | 2 +- 11 files changed, 15 insertions(+), 13 deletions(-) diff --git a/libwvdrmengine/cdm/core/include/policy_timers_v15.h b/libwvdrmengine/cdm/core/include/policy_timers_v15.h index ed3ca71a..a9f42c2d 100644 --- a/libwvdrmengine/cdm/core/include/policy_timers_v15.h +++ b/libwvdrmengine/cdm/core/include/policy_timers_v15.h @@ -29,7 +29,7 @@ class PolicyTimersV15 : public PolicyTimers { public: PolicyTimersV15() : grace_period_end_time_(0) {} - virtual ~PolicyTimersV15() {} + ~PolicyTimersV15() override {} // UpdateLicense is used in handling a license response, a renewal response, // or when restoring or releasing a persistent license. diff --git a/libwvdrmengine/cdm/core/include/policy_timers_v16.h b/libwvdrmengine/cdm/core/include/policy_timers_v16.h index 6efa7c69..2e0c472d 100644 --- a/libwvdrmengine/cdm/core/include/policy_timers_v16.h +++ b/libwvdrmengine/cdm/core/include/policy_timers_v16.h @@ -26,7 +26,7 @@ class PolicyTimersV16 : public PolicyTimers { public: PolicyTimersV16() {} - virtual ~PolicyTimersV16() {} + ~PolicyTimersV16() override {} // UpdateLicense is used in handling a license response, a renewal response, // or when restoring or releasing a persistent license. diff --git a/libwvdrmengine/cdm/core/test/certificate_provisioning_unittest.cpp b/libwvdrmengine/cdm/core/test/certificate_provisioning_unittest.cpp index 4ba4c3e1..3be65f51 100644 --- a/libwvdrmengine/cdm/core/test/certificate_provisioning_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/certificate_provisioning_unittest.cpp @@ -76,7 +76,8 @@ class MockCryptoSession : public TestCryptoSession { }; class TestStubCryptoSessionFactory : public CryptoSessionFactory { - CryptoSession* MakeCryptoSession(metrics::CryptoMetrics* crypto_metrics) { + CryptoSession* MakeCryptoSession( + metrics::CryptoMetrics* crypto_metrics) override { return new MockCryptoSession(crypto_metrics); } }; diff --git a/libwvdrmengine/cdm/core/test/crypto_session_unittest.cpp b/libwvdrmengine/cdm/core/test/crypto_session_unittest.cpp index 324b97e7..0ed24b84 100644 --- a/libwvdrmengine/cdm/core/test/crypto_session_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/crypto_session_unittest.cpp @@ -252,7 +252,7 @@ class CryptoSessionForTest : public TestCryptoSession, public WvCdmTestBase { using CryptoSession::ExtractSystemIdFromOemCert; CryptoSessionForTest() : TestCryptoSession(metrics_.GetCryptoMetrics()) {} - void SetUp() {} + void SetUp() override {} KeySession* key_session() { return key_session_.get(); } diff --git a/libwvdrmengine/cdm/core/test/device_files_unittest.cpp b/libwvdrmengine/cdm/core/test/device_files_unittest.cpp index 7a1376ce..2d16ac54 100644 --- a/libwvdrmengine/cdm/core/test/device_files_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/device_files_unittest.cpp @@ -3745,7 +3745,7 @@ const std::vector kHashedUsageInfoFileKeySetList = { class MockFile : public File { public: MockFile() {} - ~MockFile() {} + ~MockFile() override {} MOCK_METHOD2(Read, ssize_t(char*, size_t)); MOCK_METHOD2(Write, ssize_t(const char*, size_t)); @@ -3754,7 +3754,7 @@ class MockFile : public File { class MockFileSystem : public FileSystem { public: MockFileSystem() {} - ~MockFileSystem() {} + ~MockFileSystem() override {} MOCK_METHOD2(Open, std::unique_ptr(const std::string&, int flags)); MOCK_METHOD0(IsFactoryReset, bool()); diff --git a/libwvdrmengine/cdm/core/test/ota_keybox_provisioner_test.cpp b/libwvdrmengine/cdm/core/test/ota_keybox_provisioner_test.cpp index daaab941..a102619b 100644 --- a/libwvdrmengine/cdm/core/test/ota_keybox_provisioner_test.cpp +++ b/libwvdrmengine/cdm/core/test/ota_keybox_provisioner_test.cpp @@ -47,7 +47,7 @@ const std::string kAnotherFakeOtaProvisioningResponse = class MockCryptoSession : public CryptoSession { public: MockCryptoSession() : CryptoSession(&crypto_metrics_) {} - ~MockCryptoSession() {} + ~MockCryptoSession() override {} bool IsOpen() override { return is_open_; } void SetIsOpen(bool is_open) { is_open_ = is_open; } diff --git a/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp b/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp index dd70537c..a7249a67 100644 --- a/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/service_certificate_unittest.cpp @@ -79,7 +79,7 @@ class StubCdmClientPropertySet : public CdmClientPropertySet { } uint32_t session_sharing_id() const override { return session_sharing_id_; } - virtual bool use_atsc_mode() const { return false; } + bool use_atsc_mode() const override { return false; } void set_session_sharing_id(uint32_t id) override { session_sharing_id_ = id; diff --git a/libwvdrmengine/cdm/core/test/test_base.cpp b/libwvdrmengine/cdm/core/test/test_base.cpp index 1a487f08..6de0fe2c 100644 --- a/libwvdrmengine/cdm/core/test/test_base.cpp +++ b/libwvdrmengine/cdm/core/test/test_base.cpp @@ -248,7 +248,8 @@ CdmResponseType TestCryptoSession::GenerateNonce(uint32_t* nonce) { } class TestCryptoSessionFactory : public CryptoSessionFactory { - CryptoSession* MakeCryptoSession(metrics::CryptoMetrics* crypto_metrics) { + CryptoSession* MakeCryptoSession( + metrics::CryptoMetrics* crypto_metrics) override { // We need to add extra locking here because we need to make sure that there // are no other OEMCrypto calls between OEMCrypto_Initialize and // InstallTestRootOfTrust. OEMCrypto_Initialize is called in the production diff --git a/libwvdrmengine/cdm/core/test/test_base.h b/libwvdrmengine/cdm/core/test/test_base.h index 58cea6c9..f98b8e37 100644 --- a/libwvdrmengine/cdm/core/test/test_base.h +++ b/libwvdrmengine/cdm/core/test/test_base.h @@ -105,7 +105,7 @@ class TestCryptoSession : public CryptoSession { explicit TestCryptoSession(metrics::CryptoMetrics* crypto_metrics); // This intercepts nonce flood errors, which is useful for tests that request // many nonces and are not time critical. - CdmResponseType GenerateNonce(uint32_t* nonce); + CdmResponseType GenerateNonce(uint32_t* nonce) override; }; // A holder for a license. Users of this class will first open a session with diff --git a/libwvdrmengine/cdm/core/test/usage_table_header_unittest.cpp b/libwvdrmengine/cdm/core/test/usage_table_header_unittest.cpp index 0c8994ea..304c46cb 100644 --- a/libwvdrmengine/cdm/core/test/usage_table_header_unittest.cpp +++ b/libwvdrmengine/cdm/core/test/usage_table_header_unittest.cpp @@ -445,8 +445,8 @@ class MockCryptoSession : public TestCryptoSession { // Fake method for testing. Having an EXPECT_CALL causes complexities // for getting table capacity during initialization. - virtual bool GetMaximumUsageTableEntries(SecurityLevel /*security_level*/, - size_t* number_of_entries) { + bool GetMaximumUsageTableEntries(SecurityLevel /*security_level*/, + size_t* number_of_entries) override { if (number_of_entries == nullptr || !maximum_usage_table_entries_set_) return false; *number_of_entries = maximum_usage_table_entries_; diff --git a/libwvdrmengine/cdm/metrics/test/event_metric_unittest.cpp b/libwvdrmengine/cdm/metrics/test/event_metric_unittest.cpp index 7e2b978f..bb51037f 100644 --- a/libwvdrmengine/cdm/metrics/test/event_metric_unittest.cpp +++ b/libwvdrmengine/cdm/metrics/test/event_metric_unittest.cpp @@ -16,7 +16,7 @@ using drm_metrics::TestMetrics; class EventMetricTest : public ::testing::Test { public: - void SetUp() {} + void SetUp() override {} protected: }; From c43b9fc3de4313e44f1885cc7a4b97ec6da16cbf Mon Sep 17 00:00:00 2001 From: "John \"Juce\" Bruce" Date: Wed, 1 Dec 2021 11:58:05 -0800 Subject: [PATCH 3/4] Use nullptr in more places (This is a merge of http://go/wvgerrit/139630.) This patch fixes a few places that were using NULL or 0 instead of nullptr. Bug: 207702482 Test: x86-64 build Change-Id: I10e19febebd093fe4445208a082216002d9a4482 --- libwvdrmengine/cdm/core/src/privacy_crypto_boringssl.cpp | 2 +- libwvdrmengine/oemcrypto/test/oec_session_util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libwvdrmengine/cdm/core/src/privacy_crypto_boringssl.cpp b/libwvdrmengine/cdm/core/src/privacy_crypto_boringssl.cpp index 522a8837..86ddea95 100644 --- a/libwvdrmengine/cdm/core/src/privacy_crypto_boringssl.cpp +++ b/libwvdrmengine/cdm/core/src/privacy_crypto_boringssl.cpp @@ -324,7 +324,7 @@ bool ExtractExtensionValueFromCertificate(const std::string& cert, reinterpret_cast(cert.data()); long cert_size = static_cast(cert.size()); boringssl_ptr pkcs7( - d2i_PKCS7(NULL, &cert_data, cert_size)); + d2i_PKCS7(nullptr, &cert_data, cert_size)); if (!pkcs7) { LOGE("Error parsing PKCS7 message"); return false; diff --git a/libwvdrmengine/oemcrypto/test/oec_session_util.h b/libwvdrmengine/oemcrypto/test/oec_session_util.h index 49716bf9..4ec33c15 100644 --- a/libwvdrmengine/oemcrypto/test/oec_session_util.h +++ b/libwvdrmengine/oemcrypto/test/oec_session_util.h @@ -602,7 +602,7 @@ class Session { // order to verify signatures. void GenerateReport(const std::string& pst, OEMCryptoResult expected_result = OEMCrypto_SUCCESS, - Session* other = 0); + Session* other = nullptr); // Move this usage entry to a new index. void MoveUsageEntry(uint32_t new_index, std::vector* header_buffer, OEMCryptoResult expect_result = OEMCrypto_SUCCESS); From 8fd728e9927ce93215e8af245b9262077d9a1bc6 Mon Sep 17 00:00:00 2001 From: "John \"Juce\" Bruce" Date: Wed, 1 Dec 2021 14:30:37 -0800 Subject: [PATCH 4/4] Consistent result variable in CdmSession::RestoreOfflineSession (This is a merge of http://go/wvgerrit/139632.) While fixing a compiler error about shadowed variables in CdmSession, I noticed that this function had two result variables with different names as well. This patch consolidates down to one result variable. Bug: 207684988 Test: x86-64 Change-Id: Iaf6d742ef3409d85a1c364b486909d2497093112 --- libwvdrmengine/cdm/core/src/cdm_session.cpp | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/libwvdrmengine/cdm/core/src/cdm_session.cpp b/libwvdrmengine/cdm/core/src/cdm_session.cpp index e9a07aef..0f07860d 100644 --- a/libwvdrmengine/cdm/core/src/cdm_session.cpp +++ b/libwvdrmengine/cdm/core/src/cdm_session.cpp @@ -249,9 +249,9 @@ CdmResponseType CdmSession::RestoreOfflineSession(const CdmKeySetId& key_set_id, usage_entry_ = std::move(license_data.usage_entry); usage_entry_number_ = license_data.usage_entry_number; - CdmResponseType status = LoadPrivateOrLegacyKey( + CdmResponseType result = LoadPrivateOrLegacyKey( license_data.drm_certificate, license_data.wrapped_private_key); - if (status != NO_ERROR) return status; + if (result != NO_ERROR) return result; // Attempts to restore a released offline license are treated as a release // retry. @@ -309,13 +309,11 @@ CdmResponseType CdmSession::RestoreOfflineSession(const CdmKeySetId& key_set_id, std::string license_request_signature; // Sign a fake message so that OEMCrypto will start the rental clock. The // signature and generated core message are ignored. - const CdmResponseType status = - crypto_session_->PrepareAndSignLicenseRequest( - fake_message, &core_message, &license_request_signature); - if (status != NO_ERROR) return status; + result = crypto_session_->PrepareAndSignLicenseRequest( + fake_message, &core_message, &license_request_signature); + if (result != NO_ERROR) return result; } - CdmResponseType result; if (license_type == kLicenseTypeRelease) { result = license_parser_->RestoreLicenseForRelease( license_data.drm_certificate, key_request_, key_response_);