Make RenameKeyDir() use IsSameFile()
Comparing paths is error-prone (e.g. "/foo/bar" vs "/foo//bar"), so
entries in key_dirs_to_commit are compared using inode and device
number. However RenameKeyDir() breaks this rule and compares raw paths.
Avoid this quirk by finding the entry in the list to replace before
doing the rename.
This doesn't fix any known problem, as vold is fairly consistent with
its paths in practice; this is just a robustness improvement.
Bug: 190398249
Change-Id: I3ce2c0119cb2012ac9d12849570e56600bc23867
diff --git a/KeyStorage.cpp b/KeyStorage.cpp
index 472e6b1..af624e3 100644
--- a/KeyStorage.cpp
+++ b/KeyStorage.cpp
@@ -216,6 +216,7 @@
// List of key directories that have had their Keymaster key upgraded during
// this boot and written to "keymaster_key_blob_upgraded", but replacing the old
// key was delayed due to an active checkpoint. Protected by key_upgrade_lock.
+// A directory can be in this list at most once.
static std::vector<std::string> key_dirs_to_commit;
// Replaces |dir|/keymaster_key_blob with |dir|/keymaster_key_blob_upgraded and
@@ -267,8 +268,9 @@
return false;
}
-// Schedules the upgraded Keymaster key in |dir| to be committed later.
-// Assumes that key_upgrade_lock is held.
+// Schedules the upgraded Keymaster key in |dir| to be committed later. Assumes
+// that key_upgrade_lock is held and that a commit isn't already pending for the
+// directory.
static void ScheduleKeyCommit(const std::string& dir) {
if (key_dirs_to_commit.empty()) std::thread(DeferredCommitKeys).detach();
key_dirs_to_commit.push_back(dir);
@@ -288,19 +290,21 @@
// Renames a key directory. Also updates the deferred commit vector
// (key_dirs_to_commit) appropriately.
-//
-// However, @old_name must be the path to the directory that was used to put that
-// directory into the deferred commit list in the first place (since this function
-// directly compares paths instead of using IsSameFile()).
static bool RenameKeyDir(const std::string& old_name, const std::string& new_name) {
std::lock_guard<std::mutex> lock(key_upgrade_lock);
+ // Find the entry in key_dirs_to_commit (if any) for this directory so that
+ // we can update it if the rename succeeds. We don't allow duplicates in
+ // this list, so there can be at most one such entry.
+ auto it = key_dirs_to_commit.begin();
+ for (; it != key_dirs_to_commit.end(); it++) {
+ if (IsSameFile(old_name, *it)) break;
+ }
+
if (rename(old_name.c_str(), new_name.c_str()) != 0) return false;
- // IsSameFile() doesn't work here since we just renamed @old_name.
- for (auto it = key_dirs_to_commit.begin(); it != key_dirs_to_commit.end(); it++) {
- if (*it == old_name) *it = new_name;
- }
+ if (it != key_dirs_to_commit.end()) *it = new_name;
+
return true;
}