Fix a bug where bbstoreaccounts check could hang or crash.

It's not safe to use an iterator after the underlying collection has
been modified. We need to restart iterating over the directory in that
case. Otherwise we could loop forever looking for an end() that we've
already passed, or start accessing unallocated memory.

2 files changed, 215 insertions, 25 deletions

diff --git a/lib/backupstore/BackupStoreCheck2.cpp b/lib/backupstore/BackupStoreCheck2.cpp
index 8d633895..19c79304 100644
--- a/lib/backupstore/BackupStoreCheck2.cpp
+++ b/lib/backupstore/BackupStoreCheck2.cpp
@@ -706,7 +706,12 @@ bool BackupStoreDirectory::CheckAndFix()
 	bool changed = false;
 	
 	// Check that if a file depends on a new version, that version is in this directory
+	bool restart;
+
+	do
 	{
+		restart = false;
+
 		std::vector<Entry*>::iterator i(mEntries.begin());
 		for(; i != mEntries.end(); ++i)
 		{
@@ -717,7 +722,7 @@ bool BackupStoreDirectory::CheckAndFix()
 				if(newerEn == 0)
 				{
 					// Depends on something, but it isn't there.
-					BOX_TRACE("Entry id " << FMT_i <<
+					BOX_WARNING("Entry id " << FMT_i <<
 						" removed because depends "
 						"on newer version " <<
 						FMT_OID(dependsNewer) <<
@@ -727,11 +732,12 @@ bool BackupStoreDirectory::CheckAndFix()
 					delete *i;
 					mEntries.erase(i);
 					
-					// Start again at the beginning of the vector, the iterator is now invalid
-					i = mEntries.begin();
-					
 					// Mark as changed
 					changed = true;
+
+					// Start again at the beginning of the vector, the iterator is now invalid
+					restart = true;
+					break;
 				}
 				else
 				{
@@ -753,6 +759,7 @@ bool BackupStoreDirectory::CheckAndFix()
 			}
 		}
 	}
+	while(restart);
 	
 	// Check that if a file has a dependency marked, it exists, and remove it if it doesn't
 	{
diff --git a/test/backupstorefix/testbackupstorefix.cpp b/test/backupstorefix/testbackupstorefix.cpp
index 8cc5c8aa..818e836d 100644
--- a/test/backupstorefix/testbackupstorefix.cpp
+++ b/test/backupstorefix/testbackupstorefix.cpp
@@ -201,19 +201,35 @@ void check_dir_dep(BackupStoreDirectory &dir, checkdepinfoen *ck)
 
 void test_dir_fixing()
 {
+	// Test that entries pointing to nonexistent entries are removed
 	{
-		MEMLEAKFINDER_NO_LEAKS;
-		fnames[0].SetAsClearFilename("x1");
-		fnames[1].SetAsClearFilename("x2");
-		fnames[2].SetAsClearFilename("x3");
+		BackupStoreDirectory dir;
+		BackupStoreDirectory::Entry* e = dir.AddEntry(fnames[0], 12,
+			2 /* id */, 1, BackupStoreDirectory::Entry::Flags_File |
+			BackupStoreDirectory::Entry::Flags_OldVersion, 2);
+		e->SetDependsNewer(3);
+		
+		TEST_THAT(dir.CheckAndFix() == true);
+		TEST_THAT(dir.CheckAndFix() == false);
+
+		dir_en_check ck[] = {
+			{-1, 0, 0}
+		};
+		
+		check_dir(dir, ck);
 	}
 
 	{
 		BackupStoreDirectory dir;
-		dir.AddEntry(fnames[0], 12, 2 /* id */, 1, BackupStoreDirectory::Entry::Flags_File, 2);
-		dir.AddEntry(fnames[1], 12, 2 /* id */, 1, BackupStoreDirectory::Entry::Flags_File, 2);
-		dir.AddEntry(fnames[0], 12, 3 /* id */, 1, BackupStoreDirectory::Entry::Flags_File, 2);
-		dir.AddEntry(fnames[0], 12, 5 /* id */, 1, BackupStoreDirectory::Entry::Flags_File | BackupStoreDirectory::Entry::Flags_OldVersion, 2);
+		dir.AddEntry(fnames[0], 12, 2 /* id */, 1, 
+			BackupStoreDirectory::Entry::Flags_File, 2);
+		dir.AddEntry(fnames[1], 12, 2 /* id */, 1,
+			BackupStoreDirectory::Entry::Flags_File, 2);
+		dir.AddEntry(fnames[0], 12, 3 /* id */, 1,
+			BackupStoreDirectory::Entry::Flags_File, 2);
+		dir.AddEntry(fnames[0], 12, 5 /* id */, 1,
+			BackupStoreDirectory::Entry::Flags_File | 
+			BackupStoreDirectory::Entry::Flags_OldVersion, 2);
 		
 		dir_en_check ck[] = {
 			{1, 2, BackupStoreDirectory::Entry::Flags_File},
@@ -250,20 +266,26 @@ void test_dir_fixing()
 	// Test dependency fixing
 	{
 		BackupStoreDirectory dir;
-		BackupStoreDirectory::Entry *e2
-		  = dir.AddEntry(fnames[0], 12, 2 /* id */, 1, BackupStoreDirectory::Entry::Flags_File | BackupStoreDirectory::Entry::Flags_OldVersion, 2);
+		BackupStoreDirectory::Entry *e2 = dir.AddEntry(fnames[0], 12,
+			2 /* id */, 1,
+			BackupStoreDirectory::Entry::Flags_File |
+			BackupStoreDirectory::Entry::Flags_OldVersion, 2);
 		TEST_THAT(e2 != 0);
 		e2->SetDependsNewer(3);
-		BackupStoreDirectory::Entry *e3
-		  = dir.AddEntry(fnames[0], 12, 3 /* id */, 1, BackupStoreDirectory::Entry::Flags_File | BackupStoreDirectory::Entry::Flags_OldVersion, 2);
+		BackupStoreDirectory::Entry *e3 = dir.AddEntry(fnames[0], 12,
+			3 /* id */, 1,
+			BackupStoreDirectory::Entry::Flags_File |
+			BackupStoreDirectory::Entry::Flags_OldVersion, 2);
 		TEST_THAT(e3 != 0);
 		e3->SetDependsNewer(4); e3->SetDependsOlder(2);
-		BackupStoreDirectory::Entry *e4
-		  = dir.AddEntry(fnames[0], 12, 4 /* id */, 1, BackupStoreDirectory::Entry::Flags_File | BackupStoreDirectory::Entry::Flags_OldVersion, 2);
+		BackupStoreDirectory::Entry *e4 = dir.AddEntry(fnames[0], 12,
+			4 /* id */, 1,
+			BackupStoreDirectory::Entry::Flags_File |
+			BackupStoreDirectory::Entry::Flags_OldVersion, 2);
 		TEST_THAT(e4 != 0);
 		e4->SetDependsNewer(5); e4->SetDependsOlder(3);
-		BackupStoreDirectory::Entry *e5
-		  = dir.AddEntry(fnames[0], 12, 5 /* id */, 1, BackupStoreDirectory::Entry::Flags_File, 2);
+		BackupStoreDirectory::Entry *e5 = dir.AddEntry(fnames[0], 12,
+			5 /* id */, 1, BackupStoreDirectory::Entry::Flags_File, 2);
 		TEST_THAT(e5 != 0);
 		e5->SetDependsOlder(4);
 		
@@ -355,7 +377,167 @@ int test(int argc, const char *argv[])
 		"create 01234567 0 10000B 20000B") == 0);
 	TestRemoteProcessMemLeaks("bbstoreaccounts.memleaks");
 
+	// Run the perl script to create the initial directories
+	TEST_THAT_ABORTONFAIL(::system(PERL_EXECUTABLE 
+		" testfiles/testbackupstorefix.pl init") == 0);
+
+	BOX_INFO("Test that an entry pointing to a file that doesn't exist "
+		"is really deleted");
+
+	{
+		// Check that the initial situation matches our expectations.
+		std::string rootDirName;
+		StoreStructure::MakeObjectFilename(1 /* root */, storeRoot,
+			discSetNum, rootDirName, 
+			true /* EnsureDirectoryExists */);
+		std::auto_ptr<RaidFileRead> file(RaidFileRead::Open(discSetNum,
+			rootDirName));
+
+		file = RaidFileRead::Open(discSetNum, rootDirName);
+		BackupStoreDirectory dir;
+		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);
+		dir_en_check start_entries[] = {{-1, 0, 0}};
+		check_dir(dir, start_entries);
+		static checkdepinfoen start_deps[] = {{-1, 0, 0}};
+		check_dir_dep(dir, start_deps);
+
+		BackupStoreContext ctx(0x01234567,
+			*(HousekeepingInterface *)NULL,
+			"test" /* rConnectionDetails */);
+		ctx.SetClientHasAccount(storeRoot, discSetNum);
+
+		BackupProtocolLocal client(ctx);
+		client.QueryVersion(BACKUP_STORE_SERVER_VERSION);
+		client.QueryLogin(0x01234567, 0 /* read/write */);
+
+		std::string file_path = "testfiles/TestDir1/cannes/ict/metegoguered/oats";
+		int x1id = fake_upload(client, file_path, 0);
+
+		file = RaidFileRead::Open(discSetNum, rootDirName);
+		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);
+		
+		// Everything should be OK at the moment
+		TEST_THAT(dir.CheckAndFix() == false);
+
+		// Check that we've ended up with the right preconditions
+		// for the tests below.
+		dir_en_check before_entries[] = {
+			{0, x1id, BackupStoreDirectory::Entry::Flags_File},
+			{-1, 0, 0}
+		};
+		check_dir(dir, before_entries);
+		static checkdepinfoen before_deps[] = {{x1id, 0, 0}, {-1, 0, 0}};
+		check_dir_dep(dir, before_deps);
+
+		// Now break the reverse dependency by deleting x1 (the file,
+		// not the directory entry)
+		std::string x1FileName;
+		StoreStructure::MakeObjectFilename(x1id, storeRoot, discSetNum,
+			x1FileName, true /* EnsureDirectoryExists */);
+		RaidFileWrite deleteX1(discSetNum, x1FileName);
+		deleteX1.Delete();
+
+		// Check the store, check that the error is detected and
+		// repaired, by removing x1 from the directory.
+		BackupStoreCheck check(storeRoot, discSetNum,
+			0x01234567 /* AccountID */, true /* FixErrors */,
+			false /* Quiet */);
+		check.Check();
+		
+		// Read the directory back in, check that it's empty
+		file = RaidFileRead::Open(discSetNum, rootDirName);
+		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);
+		dir_en_check after_entries[] = {{-1, 0, 0}};
+		check_dir(dir, after_entries);
+		static checkdepinfoen after_deps[] = {{-1, 0, 0}};
+		check_dir_dep(dir, after_deps);
+	}
+
+	BOX_INFO("Test that an entry pointing to another that doesn't exist "
+		"is really deleted");
+
+	{
+		// Check that the initial situation matches our expectations.
+		std::string rootDirName;
+		StoreStructure::MakeObjectFilename(1 /* root */, storeRoot,
+			discSetNum, rootDirName, 
+			true /* EnsureDirectoryExists */);
+		std::auto_ptr<RaidFileRead> file(RaidFileRead::Open(discSetNum,
+			rootDirName));
+
+		file = RaidFileRead::Open(discSetNum, rootDirName);
+		BackupStoreDirectory dir;
+		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);
+		dir_en_check start_entries[] = {{-1, 0, 0}};
+		check_dir(dir, start_entries);
+		static checkdepinfoen start_deps[] = {{-1, 0, 0}};
+		check_dir_dep(dir, start_deps);
+
+		BackupStoreContext ctx(0x01234567,
+			*(HousekeepingInterface *)NULL,
+			"test" /* rConnectionDetails */);
+		ctx.SetClientHasAccount(storeRoot, discSetNum);
+
+		BackupProtocolLocal client(ctx);
+		client.QueryVersion(BACKUP_STORE_SERVER_VERSION);
+		client.QueryLogin(0x01234567, 0 /* read/write */);
+
+		std::string file_path = "testfiles/TestDir1/cannes/ict/metegoguered/oats";
+		int x1id = fake_upload(client, file_path, 0);
+
+		// Make a small change to the file
+		FileStream fs(file_path, O_WRONLY | O_APPEND);
+		const char* more = " and more oats!";
+		fs.Write(more, strlen(more));
+		fs.Close();
+
+		int x1aid = fake_upload(client, file_path, x1id);
+		file = RaidFileRead::Open(discSetNum, rootDirName);
+		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);
+		
+		// Everything should be OK at the moment
+		TEST_THAT(dir.CheckAndFix() == false);
+
+		// Check that we've ended up with the right preconditions
+		// for the tests below.
+		dir_en_check before_entries[] = {
+			{0, x1id, BackupStoreDirectory::Entry::Flags_File |
+				BackupStoreDirectory::Entry::Flags_OldVersion},
+			{0, x1aid, BackupStoreDirectory::Entry::Flags_File},
+			{-1, 0, 0}
+		};
+		check_dir(dir, before_entries);
+		static checkdepinfoen before_deps[] = {{x1id, x1aid, 0},
+			{x1aid, 0, x1id}, {-1, 0, 0}};
+		check_dir_dep(dir, before_deps);
+
+		// Now break the reverse dependency by deleting x1a (the file,
+		// not the directory entry)
+		std::string x1aFileName;
+		StoreStructure::MakeObjectFilename(x1aid, storeRoot, discSetNum,
+			x1aFileName, true /* EnsureDirectoryExists */);
+		RaidFileWrite deleteX1a(discSetNum, x1aFileName);
+		deleteX1a.Delete();
+
+		// Check the store, check that the error is detected and
+		// repaired, by removing x1 from the directory.
+		BackupStoreCheck check(storeRoot, discSetNum,
+			0x01234567 /* AccountID */, true /* FixErrors */,
+			false /* Quiet */);
+		check.Check();
+		
+		// Read the directory back in, check that it's empty
+		file = RaidFileRead::Open(discSetNum, rootDirName);
+		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);
+		dir_en_check after_entries[] = {{-1, 0, 0}};
+		check_dir(dir, after_entries);
+		static checkdepinfoen after_deps[] = {{-1, 0, 0}};
+		check_dir_dep(dir, after_deps);
+	}
+
 	// Start the bbstored server
+	BOX_TRACE("Starting bbstored server: " BBSTORED 
+		" testfiles/bbstored.conf");
 	int bbstored_pid = LaunchServer(BBSTORED " testfiles/bbstored.conf", 
 		"testfiles/bbstored.pid");
 	TEST_THAT(bbstored_pid > 0);
@@ -364,10 +546,6 @@ int test(int argc, const char *argv[])
 	::sleep(1);
 	TEST_THAT(ServerIsAlive(bbstored_pid));
 
-	// Run the perl script to create the initial directories
-	TEST_THAT_ABORTONFAIL(::system(PERL_EXECUTABLE 
-		" testfiles/testbackupstorefix.pl init") == 0);
-
 	std::string cmd = BBACKUPD " " + bbackupd_args +
 		" testfiles/bbackupd.conf";
 	int bbackupd_pid = LaunchServer(cmd, "testfiles/bbackupd.pid");
@@ -420,7 +598,12 @@ int test(int argc, const char *argv[])
 		BackupStoreCheck checker(storeRoot, discSetNum,
 			0x01234567, true /* FixErrors */, false /* Quiet */);
 		checker.Check();
-		TEST_EQUAL(1, checker.GetNumErrorsFound());
+
+		// Should just be greater than 1 really, we don't know quite
+		// how good the checker is (or will become) at spotting errors!
+		// But this will help us catch changes in checker behaviour,
+		// so it's not a bad thing to test.
+		TEST_EQUAL(5, checker.GetNumErrorsFound());
 
 		file = RaidFileRead::Open(discSetNum, fn);
 		dir.ReadFromStream(*file, IOStream::TimeOutInfinite);