[Box Backup-commit] COMMIT r3177 - in box/trunk: lib/backupstore test/backupstorefix

subversion at boxbackup.org subversion at boxbackup.org
Thu Aug 22 01:28:23 BST 2013


Author: chris
Date: 2013-08-22 01:28:23 +0100 (Thu, 22 Aug 2013)
New Revision: 3177

Modified:
   box/trunk/lib/backupstore/BackupStoreCheck2.cpp
   box/trunk/test/backupstorefix/testbackupstorefix.cpp
Log:
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.

Modified: box/trunk/lib/backupstore/BackupStoreCheck2.cpp
===================================================================
--- box/trunk/lib/backupstore/BackupStoreCheck2.cpp	2013-08-22 00:24:24 UTC (rev 3176)
+++ box/trunk/lib/backupstore/BackupStoreCheck2.cpp	2013-08-22 00:28:23 UTC (rev 3177)
@@ -706,7 +706,12 @@
 	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 @@
 				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 @@
 					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 @@
 			}
 		}
 	}
+	while(restart);
 	
 	// Check that if a file has a dependency marked, it exists, and remove it if it doesn't
 	{

Modified: box/trunk/test/backupstorefix/testbackupstorefix.cpp
===================================================================
--- box/trunk/test/backupstorefix/testbackupstorefix.cpp	2013-08-22 00:24:24 UTC (rev 3176)
+++ box/trunk/test/backupstorefix/testbackupstorefix.cpp	2013-08-22 00:28:23 UTC (rev 3177)
@@ -201,19 +201,35 @@
 
 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 @@
 	// 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 @@
 		"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 @@
 	::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,8 +598,13 @@
 		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);
 		TEST_THAT(dir.FindEntryByID(0x1234567890123456LL) == 0);




More information about the Boxbackup-commit mailing list