shill: profile: Mark invalid profiles as corrupted and ignore
If we fail to open a profile due to a failure in loading
the keyfile store, move the broken file out of the way, since
we will never be able to load this file in later attempts.
In situations where this is the default profile, this will
allow us to start up successfully the next time.
BUG=chromium-os:33822
TEST=Unit tests + manual (copy default.profile from the bug
and ensure shill restarts itself and moves the corrupted
profile out of the way.)
Change-Id: Ia3b7716b36b52c559193f6f6e28c9b185a77c01e
Reviewed-on: https://gerrit.chromium.org/gerrit/31374
Commit-Ready: Paul Stewart <pstew@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/key_file_store.cc b/key_file_store.cc
index 26fe317..5117b66 100644
--- a/key_file_store.cc
+++ b/key_file_store.cc
@@ -18,6 +18,8 @@
namespace shill {
+const char KeyFileStore::kCorruptSuffix[] = ".corrupted";
+
KeyFileStore::KeyFileStore(GLib *glib)
: glib_(glib),
crypto_(glib),
@@ -104,6 +106,21 @@
return success;
}
+bool KeyFileStore::MarkAsCorrupted() {
+ LOG(INFO) << "In " << __func__ << " for " << path_.value();
+ if (path_.empty()) {
+ LOG(ERROR) << "Empty key file path.";
+ return false;
+ }
+ string corrupted_path = path_.value() + kCorruptSuffix;
+ int ret = rename(path_.value().c_str(), corrupted_path.c_str());
+ if (ret != 0) {
+ PLOG(ERROR) << "File rename failed";
+ return false;
+ }
+ return true;
+}
+
set<string> KeyFileStore::GetGroups() const {
CHECK(key_file_);
gsize length = 0;
diff --git a/key_file_store.h b/key_file_store.h
index bcbc7bc..4d962b5 100644
--- a/key_file_store.h
+++ b/key_file_store.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -41,6 +41,11 @@
// that has not been opened successfully or has been closed already.
bool Close();
+ // Mark the underlying file store as corrupted, moving the data file
+ // to a new filename. This will prevent the file from being re-opened
+ // the next time Open() is called.
+ bool MarkAsCorrupted();
+
// Inherited from StoreInterface.
virtual bool Flush();
virtual std::set<std::string> GetGroups() const;
@@ -89,6 +94,8 @@
FRIEND_TEST(KeyFileStoreTest, OpenClose);
FRIEND_TEST(KeyFileStoreTest, OpenFail);
+ static const char kCorruptSuffix[];
+
void ReleaseKeyFile();
GLib *glib_;
diff --git a/key_file_store_unittest.cc b/key_file_store_unittest.cc
index 1cc3edd..bd0c166 100644
--- a/key_file_store_unittest.cc
+++ b/key_file_store_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -102,6 +102,19 @@
EXPECT_FALSE(store_.key_file_);
}
+TEST_F(KeyFileStoreTest, MarkAsCorrupted) {
+ EXPECT_FALSE(store_.MarkAsCorrupted());
+ EXPECT_FALSE(store_.IsNonEmpty());
+ WriteKeyFile("garbage\n");
+ EXPECT_TRUE(store_.IsNonEmpty());
+ EXPECT_TRUE(file_util::PathExists(store_.path()));
+ EXPECT_TRUE(store_.MarkAsCorrupted());
+ EXPECT_FALSE(store_.IsNonEmpty());
+ EXPECT_FALSE(file_util::PathExists(store_.path()));
+ EXPECT_TRUE(
+ file_util::PathExists(FilePath(store_.path().value() + ".corrupted")));
+}
+
TEST_F(KeyFileStoreTest, GetGroups) {
static const char kGroupA[] = "g-a";
static const char kGroupB[] = "g-b";
diff --git a/profile.cc b/profile.cc
index 52746b7..95b8956 100644
--- a/profile.cc
+++ b/profile.cc
@@ -87,6 +87,12 @@
Error::PopulateAndLog(error, Error::kInternalError,
base::StringPrintf("Could not open profile storage for %s:%s",
name_.user.c_str(), name_.identifier.c_str()));
+ if (already_exists) {
+ // The profile contents is corrupt, or we do not have access to
+ // this file. Move this file out of the way so a future open attempt
+ // will succeed, assuming the failure reason was the former.
+ storage->MarkAsCorrupted();
+ }
return false;
}
if (!already_exists) {
diff --git a/profile_unittest.cc b/profile_unittest.cc
index 2522fbb..768edc3 100644
--- a/profile_unittest.cc
+++ b/profile_unittest.cc
@@ -8,6 +8,7 @@
#include <vector>
#include <base/file_path.h>
+#include <base/file_util.h>
#include <base/memory/scoped_ptr.h>
#include <base/stringprintf.h>
#include <base/string_util.h>
@@ -354,6 +355,22 @@
// Then test that with "create or open" we succeed.
EXPECT_TRUE(ProfileInitStorage(id2, Profile::kCreateOrOpenExisting, false,
Error::kSuccess));
+
+ // Corrupt the profile storage.
+ string suffix(base::StringPrintf("/%s.profile", id.identifier.c_str()));
+ FilePath final_path(
+ base::StringPrintf(storage_path().c_str(), id.user.c_str()) + suffix);
+ string data = "]corrupt_data[";
+ EXPECT_EQ(data.size(),
+ file_util::WriteFile(final_path, data.data(), data.size()));
+
+ // Then test that we fail to open this file.
+ EXPECT_FALSE(ProfileInitStorage(id, Profile::kOpenExisting, false,
+ Error::kInternalError));
+
+ // But then on a second try the file no longer exists.
+ ASSERT_FALSE(ProfileInitStorage(id, Profile::kOpenExisting, false,
+ Error::kNotFound));
}
TEST_F(ProfileTest, UpdateDevice) {