From f44388c045a1953c255ab4ac01803e7708a7f590 Mon Sep 17 00:00:00 2001 From: "grt@chromium.org" Date: Wed, 2 Mar 2011 01:37:37 +0000 Subject: Add rollback support to DeleteRegKeyWorkItem. BUG=none TEST=new test added to installer_util_unittests.exe Review URL: http://codereview.chromium.org/6598065 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@76496 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/installer/util/delete_reg_key_work_item.cc | 320 ++++++++++++++++++++- chrome/installer/util/delete_reg_key_work_item.h | 11 +- .../util/delete_reg_key_work_item_unittest.cc | 177 ++++++++++++ chrome/installer/util/work_item_list.cc | 4 +- 4 files changed, 493 insertions(+), 19 deletions(-) create mode 100644 chrome/installer/util/delete_reg_key_work_item_unittest.cc (limited to 'chrome/installer') diff --git a/chrome/installer/util/delete_reg_key_work_item.cc b/chrome/installer/util/delete_reg_key_work_item.cc index 6c3096d..b044f44 100644 --- a/chrome/installer/util/delete_reg_key_work_item.cc +++ b/chrome/installer/util/delete_reg_key_work_item.cc @@ -2,13 +2,267 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include - #include "chrome/installer/util/delete_reg_key_work_item.h" +#include +#include +#include +#include + #include "base/logging.h" +#include "base/rand_util.h" +#include "base/stringprintf.h" +#include "base/win/registry.h" #include "chrome/installer/util/logging_installer.h" +using base::win::RegKey; + +namespace { +const REGSAM kKeyReadNoNotify = (KEY_READ) & ~(KEY_NOTIFY); +} + +// A container for a registry key, its values, and its subkeys. We don't use +// more obvious methods for various reasons: +// - RegCopyTree isn't supported pre-Vista, so we'd have to do something +// different for XP anyway. +// - SHCopyKey can't copy subkeys into a volatile destination, so we'd have to +// worry about polluting the registry. +// We don't persist security attributes since we only delete keys that we own, +// and we don't set custom attributes on them anyway. +class DeleteRegKeyWorkItem::RegKeyBackup { + public: + RegKeyBackup(); + bool Initialize(const RegKey& key); + bool WriteTo(RegKey* key) const; + + private: + // A container for a registry value. + class RegValueBackup { + public: + RegValueBackup(); + void Initialize(const wchar_t* name_buffer, DWORD name_size, + DWORD type, const uint8* data, DWORD data_size); + const std::wstring& name_str() const { return name_; } + const wchar_t* name() const { return name_.empty() ? NULL : name_.c_str(); } + DWORD type() const { return type_; } + const uint8* data() const { return data_.empty() ? NULL : &data_[0]; } + DWORD data_len() const { return static_cast(data_.size()); } + + private: + std::wstring name_; + std::vector data_; + DWORD type_; + + DISALLOW_COPY_AND_ASSIGN(RegValueBackup); + }; + + scoped_array values_; + scoped_array subkey_names_; + scoped_array subkeys_; + ptrdiff_t num_values_; + ptrdiff_t num_subkeys_; + + DISALLOW_COPY_AND_ASSIGN(RegKeyBackup); +}; + +DeleteRegKeyWorkItem::RegKeyBackup::RegValueBackup::RegValueBackup() + : type_(REG_NONE) { +} + +void DeleteRegKeyWorkItem::RegKeyBackup::RegValueBackup::Initialize( + const wchar_t* name_buffer, + DWORD name_size, + DWORD type, const uint8* data, + DWORD data_size) { + name_.assign(name_buffer, name_size); + type_ = type; + data_.assign(data, data + data_size); +} + +DeleteRegKeyWorkItem::RegKeyBackup::RegKeyBackup() + : num_values_(0), + num_subkeys_(0) { +} + +// Initializes this object by reading the values and subkeys of |key|. +// Security descriptors are not backed up. +bool DeleteRegKeyWorkItem::RegKeyBackup::Initialize(const RegKey& key) { + DCHECK(key.Valid()); + + scoped_array values; + scoped_array subkey_names; + scoped_array subkeys; + + DWORD num_subkeys = 0; + DWORD max_subkey_name_len = 0; + DWORD num_values = 0; + DWORD max_value_name_len = 0; + DWORD max_value_len = 0; + LONG result = RegQueryInfoKey(key.Handle(), NULL, NULL, NULL, + &num_subkeys, &max_subkey_name_len, NULL, + &num_values, &max_value_name_len, + &max_value_len, NULL, NULL); + if (result != ERROR_SUCCESS) { + LOG(ERROR) << "Failed getting info of key to backup, result: " << result; + return false; + } + if (max_subkey_name_len >= std::numeric_limits::max() - 1 || + max_value_name_len >= std::numeric_limits::max() - 1) { + LOG(ERROR) + << "Failed backing up key; subkeys and/or names are out of range."; + return false; + } + DWORD max_name_len = std::max(max_subkey_name_len, max_value_name_len) + 1; + scoped_array name_buffer(new wchar_t[max_name_len]); + + // Backup the values. + if (num_values != 0) { + values.reset(new RegValueBackup[num_values]); + scoped_array value_buffer(new uint8[max_value_len]); + DWORD name_size = 0; + DWORD value_type = REG_NONE; + DWORD value_size = 0; + + for (DWORD i = 0; i < num_values; ) { + name_size = max_name_len; + value_size = max_value_len; + result = RegEnumValue(key.Handle(), i, name_buffer.get(), &name_size, + NULL, &value_type, value_buffer.get(), &value_size); + switch (result) { + case ERROR_NO_MORE_ITEMS: + num_values = i; + break; + case ERROR_SUCCESS: + values[i].Initialize(name_buffer.get(), name_size, value_type, + value_buffer.get(), value_size); + ++i; + break; + case ERROR_MORE_DATA: + if (value_size > max_value_len) { + max_value_len = value_size; + value_buffer.reset(new uint8[max_value_len]); + } else { + DCHECK(max_name_len - 1 < name_size); + if (name_size >= std::numeric_limits::max() - 1) { + LOG(ERROR) << "Failed backing up key; value name out of range."; + return false; + } + max_name_len = name_size + 1; + name_buffer.reset(new wchar_t[max_name_len]); + } + break; + default: + LOG(ERROR) << "Failed backing up value " << i << ", result: " + << result; + return false; + } + } + DLOG_IF(WARNING, RegEnumValue(key.Handle(), num_values, name_buffer.get(), + &name_size, NULL, &value_type, NULL, + NULL) != ERROR_NO_MORE_ITEMS) + << "Concurrent modifications to registry key during backup operation."; + } + + // Backup the subkeys. + if (num_subkeys != 0) { + subkey_names.reset(new std::wstring[num_subkeys]); + subkeys.reset(new RegKeyBackup[num_subkeys]); + DWORD name_size = 0; + + // Get the names of them. + for (DWORD i = 0; i < num_subkeys; ) { + name_size = max_name_len; + result = RegEnumKeyEx(key.Handle(), i, name_buffer.get(), &name_size, + NULL, NULL, NULL, NULL); + switch (result) { + case ERROR_NO_MORE_ITEMS: + num_subkeys = i; + break; + case ERROR_SUCCESS: + subkey_names[i].assign(name_buffer.get(), name_size); + ++i; + break; + case ERROR_MORE_DATA: + if (name_size >= std::numeric_limits::max() - 1) { + LOG(ERROR) << "Failed backing up key; subkey name out of range."; + return false; + } + max_name_len = name_size + 1; + name_buffer.reset(new wchar_t[max_name_len]); + break; + default: + LOG(ERROR) << "Failed getting name of subkey " << i + << " for backup, result: " << result; + return false; + } + } + DLOG_IF(WARNING, + RegEnumKeyEx(key.Handle(), num_subkeys, NULL, &name_size, NULL, + NULL, NULL, NULL) != ERROR_NO_MORE_ITEMS) + << "Concurrent modifications to registry key during backup operation."; + + // Get their values. + RegKey subkey; + for (DWORD i = 0; i < num_subkeys; ++i) { + result = subkey.Open(key.Handle(), subkey_names[i].c_str(), + kKeyReadNoNotify); + if (result != ERROR_SUCCESS) { + LOG(ERROR) << "Failed opening subkey \"" << subkey_names[i] + << "\" for backup, result: " << result; + return false; + } + if (!subkeys[i].Initialize(subkey)) { + LOG(ERROR) << "Failed backing up subkey \"" << subkey_names[i] << "\""; + return false; + } + } + } + + values_.swap(values); + subkey_names_.swap(subkey_names); + subkeys_.swap(subkeys); + num_values_ = num_values; + num_subkeys_ = num_subkeys; + + return true; +} + +// Writes the values and subkeys of this object into |key|. +bool DeleteRegKeyWorkItem::RegKeyBackup::WriteTo(RegKey* key) const { + LONG result = ERROR_SUCCESS; + + // Write the values. + for (int i = 0; i < num_values_; ++i) { + const RegValueBackup& value = values_[i]; + result = RegSetValueEx(key->Handle(), value.name(), 0, value.type(), + value.data(), value.data_len()); + if (result != ERROR_SUCCESS) { + LOG(ERROR) << "Failed writing value \"" << value.name_str() + << "\", result: " << result; + return false; + } + } + + // Write the subkeys. + RegKey subkey; + for (int i = 0; i < num_subkeys_; ++i) { + const std::wstring& name = subkey_names_[i]; + + result = subkey.Create(key->Handle(), name.c_str(), KEY_WRITE); + if (result != ERROR_SUCCESS) { + LOG(ERROR) << "Failed creating subkey \"" << name << "\", result: " + << result; + return false; + } + if (!subkeys_[i].WriteTo(&subkey)) { + LOG(ERROR) << "Failed writing subkey \"" << name << "\", result: " + << result; + return false; + } + } + + return true; +} DeleteRegKeyWorkItem::~DeleteRegKeyWorkItem() { } @@ -17,28 +271,68 @@ DeleteRegKeyWorkItem::DeleteRegKeyWorkItem(HKEY predefined_root, const std::wstring& path) : predefined_root_(predefined_root), path_(path) { + // It's a safe bet that we don't want to delete one of the root trees. + DCHECK(!path.empty()); } bool DeleteRegKeyWorkItem::Do() { - // TODO(robertshield): Implement rollback for this work item. Consider using - // RegSaveKey or RegCopyKey. + scoped_ptr backup; + + // Only try to make a backup if we're not configured to ignore failures. if (!ignore_failure_) { - NOTREACHED(); - LOG(ERROR) << "Unexpected configuration in DeleteRegKeyWorkItem."; - return false; + RegKey original_key; + + // Does the key exist? + LONG result = original_key.Open(predefined_root_, path_.c_str(), + kKeyReadNoNotify); + if (result == ERROR_SUCCESS) { + backup.reset(new RegKeyBackup()); + if (!backup->Initialize(original_key)) { + LOG(ERROR) << "Failed to backup key at " << path_; + return ignore_failure_; + } + } else if (result != ERROR_FILE_NOT_FOUND) { + LOG(ERROR) << "Failed to open key at " << path_ + << " to create backup, result: " << result; + return ignore_failure_; + } } - LSTATUS result = SHDeleteKey(predefined_root_, path_.c_str()); - if (result != ERROR_SUCCESS) { - LOG(ERROR) << "Failed to delete key at " << path_ << ", result: " << result; + // Delete the key. + LONG result = SHDeleteKey(predefined_root_, path_.c_str()); + if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) { + LOG(ERROR) << "Failed to delete key at " << path_ << ", result: " + << result; + return ignore_failure_; } + // We've succeeded, so remember any backup we may have made. + backup_.swap(backup); + return true; } void DeleteRegKeyWorkItem::Rollback() { - if (ignore_failure_) + if (ignore_failure_ || backup_.get() == NULL) return; - NOTREACHED(); -} + // Delete anything in the key before restoring the backup in case someone else + // put new data in the key after Do(). + LONG result = SHDeleteKey(predefined_root_, path_.c_str()); + if (result != ERROR_SUCCESS && result != ERROR_FILE_NOT_FOUND) { + LOG(ERROR) << "Failed to delete key at " << path_ << " in rollback, " + "result: " << result; + } + + // Restore the old contents. The restoration takes on its default security + // attributes; any custom attributes are lost. + RegKey original_key; + result = original_key.Create(predefined_root_, path_.c_str(), KEY_WRITE); + if (result != ERROR_SUCCESS) { + LOG(ERROR) << "Failed to create original key at " << path_ + << " in rollback, result: " << result; + } else { + if (!backup_->WriteTo(&original_key)) + LOG(ERROR) << "Failed to restore key in rollback, result: " << result; + } +} diff --git a/chrome/installer/util/delete_reg_key_work_item.h b/chrome/installer/util/delete_reg_key_work_item.h index 3d42bf2..74806ed 100644 --- a/chrome/installer/util/delete_reg_key_work_item.h +++ b/chrome/installer/util/delete_reg_key_work_item.h @@ -11,9 +11,12 @@ #include #include "base/basictypes.h" +#include "base/scoped_ptr.h" #include "chrome/installer/util/work_item.h" -// A WorkItem subclass that deletes a registry key at the given path. +// A WorkItem subclass that deletes a registry key at the given path. Be aware +// that in the event of rollback the key's values and subkeys are restored but +// the key and its subkeys take on their default security descriptors. class DeleteRegKeyWorkItem : public WorkItem { public: virtual ~DeleteRegKeyWorkItem(); @@ -23,6 +26,7 @@ class DeleteRegKeyWorkItem : public WorkItem { virtual void Rollback(); private: + class RegKeyBackup; friend class WorkItem; DeleteRegKeyWorkItem(HKEY predefined_root, const std::wstring& path); @@ -34,10 +38,9 @@ class DeleteRegKeyWorkItem : public WorkItem { // Path of the key to be deleted. std::wstring path_; - // Path in the registry that the key is backed up to. - std::wstring backup_path_; + // Backup of the deleted key. + scoped_ptr backup_; - private: DISALLOW_COPY_AND_ASSIGN(DeleteRegKeyWorkItem); }; diff --git a/chrome/installer/util/delete_reg_key_work_item_unittest.cc b/chrome/installer/util/delete_reg_key_work_item_unittest.cc new file mode 100644 index 0000000..7e80a9a --- /dev/null +++ b/chrome/installer/util/delete_reg_key_work_item_unittest.cc @@ -0,0 +1,177 @@ +// Copyright (c) 2011 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include +#include // NOLINT +#include "base/logging.h" +#include "base/scoped_ptr.h" +#include "base/win/registry.h" +#include "chrome/installer/util/delete_reg_key_work_item.h" +#include "chrome/installer/util/work_item.h" +#include "testing/gtest/include/gtest/gtest.h" + +using base::win::RegKey; + +namespace { +wchar_t test_root[] = L"SOFTWARE\\TmpTmp"; +} + +class DeleteRegKeyWorkItemTest : public testing::Test { + protected: + virtual void SetUp() { + // Create a temporary key for testing + RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS); + key.DeleteKey(test_root); + ASSERT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, test_root, KEY_READ)); + ASSERT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, test_root, + KEY_READ)); + } + + virtual void TearDown() { + logging::CloseLogFile(); + // Clean up the temporary key + RegKey key(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS); + ASSERT_EQ(ERROR_SUCCESS, key.DeleteKey(test_root)); + } +}; + +// Test that deleting a key that doesn't exist succeeds, and that rollback does +// nothing. +TEST_F(DeleteRegKeyWorkItemTest, TestNoKey) { + RegKey key; + std::wstring key_name(std::wstring(test_root) + L"\\NoKeyHere"); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + scoped_ptr item( + WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name)); + EXPECT_TRUE(item->Do()); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + item->Rollback(); + item.reset(); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); +} + +// Test that deleting a subkey of a key that doesn't exist succeeds, and that +// rollback does nothing. +TEST_F(DeleteRegKeyWorkItemTest, TestNoSubkey) { + RegKey key; + std::wstring key_name(std::wstring(test_root) + L"\\NoKeyHere\\OrHere"); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + scoped_ptr item( + WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name)); + EXPECT_TRUE(item->Do()); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + item->Rollback(); + item.reset(); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); +} + +// Test that deleting an empty key succeeds, and that rollback brings it back. +TEST_F(DeleteRegKeyWorkItemTest, TestEmptyKey) { + RegKey key; + std::wstring key_name(std::wstring(test_root) + L"\\EmptyKey"); + EXPECT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, key_name.c_str(), + KEY_WRITE)); + key.Close(); + scoped_ptr item( + WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name)); + EXPECT_TRUE(item->Do()); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + item->Rollback(); + item.reset(); + EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); +} + +// Test that deleting a key with subkeys and values succeeds, and that rollback +// brings them all back. +TEST_F(DeleteRegKeyWorkItemTest, TestNonEmptyKey) { + RegKey key; + std::wstring key_name(std::wstring(test_root) + L"\\NonEmptyKey"); + EXPECT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, key_name.c_str(), + KEY_WRITE)); + EXPECT_EQ(ERROR_SUCCESS, key.WriteValue(NULL, key_name.c_str())); + EXPECT_EQ(ERROR_SUCCESS, key.CreateKey(L"Subkey", KEY_WRITE)); + EXPECT_EQ(ERROR_SUCCESS, key.WriteValue(L"SomeValue", 1U)); + key.Close(); + scoped_ptr item( + WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name)); + EXPECT_TRUE(item->Do()); + EXPECT_NE(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + item->Rollback(); + item.reset(); + EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_READ)); + std::wstring str_value; + EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &str_value)); + EXPECT_EQ(key_name, str_value); + EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey", KEY_READ)); + DWORD dw_value = 0; + EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"SomeValue", &dw_value)); + EXPECT_EQ(1U, dw_value); +} + +// Test that deleting a key with subkeys we can't delete fails, and that +// everything is there after rollback. +TEST_F(DeleteRegKeyWorkItemTest, TestUndeletableKey) { + RegKey key; + std::wstring key_name(std::wstring(test_root) + L"\\UndeletableKey"); + EXPECT_EQ(ERROR_SUCCESS, key.Create(HKEY_CURRENT_USER, key_name.c_str(), + KEY_WRITE)); + EXPECT_EQ(ERROR_SUCCESS, key.WriteValue(NULL, key_name.c_str())); + DWORD dw_value = 1; + RegKey subkey; + RegKey subkey2; + EXPECT_EQ(ERROR_SUCCESS, subkey.Create(key.Handle(), L"Subkey", + KEY_WRITE | WRITE_DAC)); + EXPECT_EQ(ERROR_SUCCESS, subkey.WriteValue(L"SomeValue", 1U)); + EXPECT_EQ(ERROR_SUCCESS, subkey2.Create(subkey.Handle(), L"Subkey2", + KEY_WRITE | WRITE_DAC)); + EXPECT_EQ(ERROR_SUCCESS, subkey2.WriteValue(L"", 2U)); + CSecurityDesc sec_desc; + sec_desc.FromString(L"D:PAI(A;OICI;KR;;;BU)"); // builtin users read + EXPECT_EQ(ERROR_SUCCESS, + RegSetKeySecurity(subkey.Handle(), DACL_SECURITY_INFORMATION, + const_cast( + sec_desc.GetPSECURITY_DESCRIPTOR()))); + sec_desc.FromString(L"D:PAI(A;OICI;KA;;;BU)"); // builtin users all access + EXPECT_EQ(ERROR_SUCCESS, + RegSetKeySecurity(subkey2.Handle(), DACL_SECURITY_INFORMATION, + const_cast( + sec_desc.GetPSECURITY_DESCRIPTOR()))); + subkey2.Close(); + subkey.Close(); + key.Close(); + scoped_ptr item( + WorkItem::CreateDeleteRegKeyWorkItem(HKEY_CURRENT_USER, key_name)); + EXPECT_FALSE(item->Do()); + EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_QUERY_VALUE)); + item->Rollback(); + item.reset(); + EXPECT_EQ(ERROR_SUCCESS, key.Open(HKEY_CURRENT_USER, key_name.c_str(), + KEY_QUERY_VALUE)); + std::wstring str_value; + EXPECT_EQ(ERROR_SUCCESS, key.ReadValue(NULL, &str_value)); + EXPECT_EQ(key_name, str_value); + EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey", KEY_READ | WRITE_DAC)); + dw_value = 0; + EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"SomeValue", &dw_value)); + EXPECT_EQ(1U, dw_value); + // Give users all access to the subkey so it can be deleted. + EXPECT_EQ(ERROR_SUCCESS, + RegSetKeySecurity(key.Handle(), DACL_SECURITY_INFORMATION, + const_cast( + sec_desc.GetPSECURITY_DESCRIPTOR()))); + EXPECT_EQ(ERROR_SUCCESS, key.OpenKey(L"Subkey2", KEY_QUERY_VALUE)); + EXPECT_EQ(ERROR_SUCCESS, key.ReadValueDW(L"", &dw_value)); + EXPECT_EQ(2U, dw_value); +} diff --git a/chrome/installer/util/work_item_list.cc b/chrome/installer/util/work_item_list.cc index 3156f7d..958d478 100644 --- a/chrome/installer/util/work_item_list.cc +++ b/chrome/installer/util/work_item_list.cc @@ -208,7 +208,8 @@ bool NoRollbackWorkItemList::Do() { executed_list_.push_front(work_item); work_item->set_ignore_failure(true); if (!work_item->Do()) { - LOG(ERROR) << "NoRollbackWorkItemList: item execution failed."; + LOG(ERROR) << "NoRollbackWorkItemList: item execution failed " + << work_item->log_message(); result = false; } } @@ -223,4 +224,3 @@ bool NoRollbackWorkItemList::Do() { void NoRollbackWorkItemList::Rollback() { NOTREACHED() << "Can't rollback a NoRollbackWorkItemList"; } - -- cgit v1.1