From ff2f683f3cd8fa8cb594f94b5af813f25ba34f42 Mon Sep 17 00:00:00 2001 From: "haitaol@chromium.org" Date: Thu, 24 Jul 2014 13:34:49 +0000 Subject: [PATCH] Fix use-after-free in sync backup/rollback manager. Shouldn't use member variable if initialization fails because SyncBackendHostCore would destroy sync manager when receiving failure notification. BUG=395410 Review URL: https://codereview.chromium.org/407093009 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285229 0039d316-1c4b-4281-b951-d872f2087c98 --- sync/internal_api/sync_backup_manager.cc | 32 ++++++---------- sync/internal_api/sync_rollback_manager.cc | 37 +++++++------------ .../sync_rollback_manager_base.cc | 28 ++++---------- .../internal_api/sync_rollback_manager_base.h | 27 ++++---------- .../sync_rollback_manager_base_unittest.cc | 30 ++++++++++++++- 5 files changed, 68 insertions(+), 86 deletions(-) diff --git a/sync/internal_api/sync_backup_manager.cc b/sync/internal_api/sync_backup_manager.cc index a8a377e21770ca..32955683dd9ee2 100644 --- a/sync/internal_api/sync_backup_manager.cc +++ b/sync/internal_api/sync_backup_manager.cc @@ -38,27 +38,17 @@ void SyncBackupManager::Init( ReportUnrecoverableErrorFunction report_unrecoverable_error_function, CancelationSignal* cancelation_signal) { - SyncRollbackManagerBase::Init(database_location, event_handler, - sync_server_and_path, sync_server_port, - use_ssl, post_factory.Pass(), - workers, extensions_activity, change_delegate, - credentials, invalidator_client_id, - restored_key_for_bootstrapping, - restored_keystore_key_for_bootstrapping, - internal_components_factory, encryptor, - unrecoverable_error_handler.Pass(), - report_unrecoverable_error_function, - cancelation_signal); - - if (!initialized()) - return; - - GetUserShare()->directory->CollectMetaHandleCounts( - &status_.num_entries_by_type, - &status_.num_to_delete_entries_by_type); - - HideSyncPreference(PRIORITY_PREFERENCES); - HideSyncPreference(PREFERENCES); + if (SyncRollbackManagerBase::InitInternal( + database_location, + internal_components_factory, + unrecoverable_error_handler.Pass(), + report_unrecoverable_error_function)) { + GetUserShare()->directory->CollectMetaHandleCounts( + &status_.num_entries_by_type, &status_.num_to_delete_entries_by_type); + + HideSyncPreference(PRIORITY_PREFERENCES); + HideSyncPreference(PREFERENCES); + } } void SyncBackupManager::SaveChanges() { diff --git a/sync/internal_api/sync_rollback_manager.cc b/sync/internal_api/sync_rollback_manager.cc index 9a54f039d8b627..c5a6f75226574a 100644 --- a/sync/internal_api/sync_rollback_manager.cc +++ b/sync/internal_api/sync_rollback_manager.cc @@ -41,31 +41,22 @@ void SyncRollbackManager::Init( ReportUnrecoverableErrorFunction report_unrecoverable_error_function, CancelationSignal* cancelation_signal) { - SyncRollbackManagerBase::Init(database_location, event_handler, - sync_server_and_path, sync_server_port, - use_ssl, post_factory.Pass(), - workers, extensions_activity, change_delegate, - credentials, invalidator_client_id, - restored_key_for_bootstrapping, - restored_keystore_key_for_bootstrapping, - internal_components_factory, encryptor, - unrecoverable_error_handler.Pass(), - report_unrecoverable_error_function, - cancelation_signal); - - if (!initialized()) - return; - - change_delegate_ = change_delegate; + if (SyncRollbackManagerBase::InitInternal( + database_location, + internal_components_factory, + unrecoverable_error_handler.Pass(), + report_unrecoverable_error_function)) { + change_delegate_ = change_delegate; + + for (size_t i = 0; i < workers.size(); ++i) { + ModelSafeGroup group = workers[i]->GetModelSafeGroup(); + CHECK(workers_.find(group) == workers_.end()); + workers_[group] = workers[i]; + } - for (size_t i = 0; i < workers.size(); ++i) { - ModelSafeGroup group = workers[i]->GetModelSafeGroup(); - CHECK(workers_.find(group) == workers_.end()); - workers_[group] = workers[i]; + rollback_ready_types_ = GetUserShare()->directory->InitialSyncEndedTypes(); + rollback_ready_types_.RetainAll(BackupTypes()); } - - rollback_ready_types_ = GetUserShare()->directory->InitialSyncEndedTypes(); - rollback_ready_types_.RetainAll(BackupTypes()); } void SyncRollbackManager::StartSyncingNormally( diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc index 6f864a3938122b..39ee0013bbb8e5 100644 --- a/sync/internal_api/sync_rollback_manager_base.cc +++ b/sync/internal_api/sync_rollback_manager_base.cc @@ -50,36 +50,22 @@ SyncRollbackManagerBase::SyncRollbackManagerBase() SyncRollbackManagerBase::~SyncRollbackManagerBase() { } -void SyncRollbackManagerBase::Init( - const base::FilePath& database_location, - const WeakHandle& event_handler, - const std::string& sync_server_and_path, - int sync_server_port, - bool use_ssl, - scoped_ptr post_factory, - const std::vector >& workers, - ExtensionsActivity* extensions_activity, - SyncManager::ChangeDelegate* change_delegate, - const SyncCredentials& credentials, - const std::string& invalidator_client_id, - const std::string& restored_key_for_bootstrapping, - const std::string& restored_keystore_key_for_bootstrapping, - InternalComponentsFactory* internal_components_factory, - Encryptor* encryptor, - scoped_ptr unrecoverable_error_handler, - ReportUnrecoverableErrorFunction - report_unrecoverable_error_function, - CancelationSignal* cancelation_signal) { +bool SyncRollbackManagerBase::InitInternal( + const base::FilePath& database_location, + InternalComponentsFactory* internal_components_factory, + scoped_ptr unrecoverable_error_handler, + ReportUnrecoverableErrorFunction report_unrecoverable_error_function) { unrecoverable_error_handler_ = unrecoverable_error_handler.Pass(); report_unrecoverable_error_function_ = report_unrecoverable_error_function; if (!InitBackupDB(database_location, internal_components_factory)) { NotifyInitializationFailure(); - return; + return false; } initialized_ = true; NotifyInitializationSuccess(); + return true; } ModelTypeSet SyncRollbackManagerBase::InitialSyncEndedTypes() { diff --git a/sync/internal_api/sync_rollback_manager_base.h b/sync/internal_api/sync_rollback_manager_base.h index 9aaf02b406bd76..6d682f529de36e 100644 --- a/sync/internal_api/sync_rollback_manager_base.h +++ b/sync/internal_api/sync_rollback_manager_base.h @@ -35,26 +35,6 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase : virtual ~SyncRollbackManagerBase(); // SyncManager implementation. - virtual void Init( - const base::FilePath& database_location, - const WeakHandle& event_handler, - const std::string& sync_server_and_path, - int sync_server_port, - bool use_ssl, - scoped_ptr post_factory, - const std::vector >& workers, - ExtensionsActivity* extensions_activity, - SyncManager::ChangeDelegate* change_delegate, - const SyncCredentials& credentials, - const std::string& invalidator_client_id, - const std::string& restored_key_for_bootstrapping, - const std::string& restored_keystore_key_for_bootstrapping, - InternalComponentsFactory* internal_components_factory, - Encryptor* encryptor, - scoped_ptr unrecoverable_error_handler, - ReportUnrecoverableErrorFunction - report_unrecoverable_error_function, - CancelationSignal* cancelation_signal) OVERRIDE; virtual ModelTypeSet InitialSyncEndedTypes() OVERRIDE; virtual ModelTypeSet GetTypesWithEmptyProgressMarkerToken( ModelTypeSet types) OVERRIDE; @@ -115,6 +95,13 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase : protected: ObserverList* GetObservers(); + // Initialize sync backup DB. + bool InitInternal( + const base::FilePath& database_location, + InternalComponentsFactory* internal_components_factory, + scoped_ptr unrecoverable_error_handler, + ReportUnrecoverableErrorFunction report_unrecoverable_error_function); + virtual void RegisterDirectoryTypeDebugInfoObserver( syncer::TypeDebugInfoObserver* observer) OVERRIDE; virtual void UnregisterDirectoryTypeDebugInfoObserver( diff --git a/sync/internal_api/sync_rollback_manager_base_unittest.cc b/sync/internal_api/sync_rollback_manager_base_unittest.cc index 39217f5d5b7474..1d4c2dec029ce0 100644 --- a/sync/internal_api/sync_rollback_manager_base_unittest.cc +++ b/sync/internal_api/sync_rollback_manager_base_unittest.cc @@ -18,6 +18,34 @@ void OnConfigDone(bool success) { EXPECT_TRUE(success); } +class SyncTestRollbackManager : public SyncRollbackManagerBase { + public: + virtual void Init( + const base::FilePath& database_location, + const WeakHandle& event_handler, + const std::string& sync_server_and_path, + int sync_server_port, + bool use_ssl, + scoped_ptr post_factory, + const std::vector >& workers, + ExtensionsActivity* extensions_activity, + ChangeDelegate* change_delegate, + const SyncCredentials& credentials, + const std::string& invalidator_client_id, + const std::string& restored_key_for_bootstrapping, + const std::string& restored_keystore_key_for_bootstrapping, + InternalComponentsFactory* internal_components_factory, + Encryptor* encryptor, + scoped_ptr unrecoverable_error_handler, + ReportUnrecoverableErrorFunction report_unrecoverable_error_function, + CancelationSignal* cancelation_signal) OVERRIDE { + SyncRollbackManagerBase::InitInternal(database_location, + internal_components_factory, + unrecoverable_error_handler.Pass(), + report_unrecoverable_error_function); + } +}; + class SyncRollbackManagerBaseTest : public testing::Test { protected: virtual void SetUp() OVERRIDE { @@ -32,7 +60,7 @@ class SyncRollbackManagerBaseTest : public testing::Test { NULL, NULL); } - SyncRollbackManagerBase manager_; + SyncTestRollbackManager manager_; base::MessageLoop loop_; // Needed for WeakHandle };