From 711ac679ce0788ec778986dbc377b344a2e25d01 Mon Sep 17 00:00:00 2001 From: Chris Wilson Date: Thu, 22 Aug 2013 00:28:23 +0000 Subject: 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. --- test/backupstorefix/testbackupstorefix.cpp | 225 ++++++++++++++++++++++++++--- 1 file changed, 204 insertions(+), 21 deletions(-) (limited to 'test') 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 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 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); -- cgit v1.2.3