Skip to content

Commit

Permalink
Fix use-after-free in sync backup/rollback manager.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
haitaol@chromium.org committed Jul 24, 2014
1 parent 3d06586 commit ff2f683
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 86 deletions.
32 changes: 11 additions & 21 deletions sync/internal_api/sync_backup_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
37 changes: 14 additions & 23 deletions sync/internal_api/sync_rollback_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
28 changes: 7 additions & 21 deletions sync/internal_api/sync_rollback_manager_base.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,36 +50,22 @@ SyncRollbackManagerBase::SyncRollbackManagerBase()
SyncRollbackManagerBase::~SyncRollbackManagerBase() {
}

void SyncRollbackManagerBase::Init(
const base::FilePath& database_location,
const WeakHandle<JsEventHandler>& event_handler,
const std::string& sync_server_and_path,
int sync_server_port,
bool use_ssl,
scoped_ptr<HttpPostProviderFactory> post_factory,
const std::vector<scoped_refptr<ModelSafeWorker> >& 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<UnrecoverableErrorHandler> 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<UnrecoverableErrorHandler> 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() {
Expand Down
27 changes: 7 additions & 20 deletions sync/internal_api/sync_rollback_manager_base.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,26 +35,6 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
virtual ~SyncRollbackManagerBase();

// SyncManager implementation.
virtual void Init(
const base::FilePath& database_location,
const WeakHandle<JsEventHandler>& event_handler,
const std::string& sync_server_and_path,
int sync_server_port,
bool use_ssl,
scoped_ptr<HttpPostProviderFactory> post_factory,
const std::vector<scoped_refptr<ModelSafeWorker> >& 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<UnrecoverableErrorHandler> unrecoverable_error_handler,
ReportUnrecoverableErrorFunction
report_unrecoverable_error_function,
CancelationSignal* cancelation_signal) OVERRIDE;
virtual ModelTypeSet InitialSyncEndedTypes() OVERRIDE;
virtual ModelTypeSet GetTypesWithEmptyProgressMarkerToken(
ModelTypeSet types) OVERRIDE;
Expand Down Expand Up @@ -115,6 +95,13 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
protected:
ObserverList<SyncManager::Observer>* GetObservers();

// Initialize sync backup DB.
bool InitInternal(
const base::FilePath& database_location,
InternalComponentsFactory* internal_components_factory,
scoped_ptr<UnrecoverableErrorHandler> unrecoverable_error_handler,
ReportUnrecoverableErrorFunction report_unrecoverable_error_function);

virtual void RegisterDirectoryTypeDebugInfoObserver(
syncer::TypeDebugInfoObserver* observer) OVERRIDE;
virtual void UnregisterDirectoryTypeDebugInfoObserver(
Expand Down
30 changes: 29 additions & 1 deletion sync/internal_api/sync_rollback_manager_base_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<JsEventHandler>& event_handler,
const std::string& sync_server_and_path,
int sync_server_port,
bool use_ssl,
scoped_ptr<HttpPostProviderFactory> post_factory,
const std::vector<scoped_refptr<ModelSafeWorker> >& 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<UnrecoverableErrorHandler> 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 {
Expand All @@ -32,7 +60,7 @@ class SyncRollbackManagerBaseTest : public testing::Test {
NULL, NULL);
}

SyncRollbackManagerBase manager_;
SyncTestRollbackManager manager_;
base::MessageLoop loop_; // Needed for WeakHandle
};

Expand Down

0 comments on commit ff2f683

Please sign in to comment.