From 58e79baef59eb99bf2139ef4021308f36f8164a1 Mon Sep 17 00:00:00 2001 From: Denis Kuznetsov Date: Tue, 12 Dec 2017 21:38:55 +0100 Subject: Make session_manager use proper synchronization primitives. Moved a bunch of file utils from cryptohome::Platform to libbrillo/file_utils. BUG=chromium:557833 TEST=cros_workon_make cryptohome --test TEST=cros_workon_make libbrillo --test Change-Id: Ia5ee99a790ea010fc2cd0cac37bb100c81b3ca8c Reviewed-on: https://chromium-review.googlesource.com/822614 Commit-Ready: Denis Kuznetsov Tested-by: Denis Kuznetsov Reviewed-by: Dan Erat --- brillo/file_utils.cc | 161 ++++++++++++++++++++++++++++++++++++++++-- brillo/file_utils.h | 48 +++++++++++++ brillo/file_utils_unittest.cc | 130 +++++++++++++++++++++++++++++++--- 3 files changed, 324 insertions(+), 15 deletions(-) diff --git a/brillo/file_utils.cc b/brillo/file_utils.cc index 24ed347..8faa1b7 100644 --- a/brillo/file_utils.cc +++ b/brillo/file_utils.cc @@ -12,11 +12,17 @@ #include #include #include +#include +#include +#include namespace brillo { namespace { +// Log sync(), fsync(), etc. calls that take this many seconds or longer. +constexpr const base::TimeDelta kLongSync = base::TimeDelta::FromSeconds(10); + enum { kPermissions600 = S_IRUSR | S_IWUSR, kPermissions777 = S_IRWXU | S_IRWXG | S_IRWXO @@ -112,11 +118,9 @@ bool TouchFileInternal(const base::FilePath& path, } // Create the file as owner-only initially. - base::ScopedFD scoped_fd( - HANDLE_EINTR(openat(AT_FDCWD, - path.value().c_str(), - O_RDONLY | O_NOFOLLOW | O_CREAT | O_EXCL | O_CLOEXEC, - kPermissions600))); + base::ScopedFD scoped_fd(HANDLE_EINTR(openat( + AT_FDCWD, path.value().c_str(), + O_RDONLY | O_NOFOLLOW | O_CREAT | O_EXCL | O_CLOEXEC, kPermissions600))); if (scoped_fd == -1) { PLOG(WARNING) << "Failed to create file \"" << path.value() << '"'; return false; @@ -128,6 +132,24 @@ bool TouchFileInternal(const base::FilePath& path, return true; } +std::string GetRandomSuffix() { + const int kBufferSize = 6; + unsigned char buffer[kBufferSize]; + base::RandBytes(buffer, arraysize(buffer)); + std::string suffix; + for (int i = 0; i < kBufferSize; ++i) { + int random_value = buffer[i] % (2 * 26 + 10); + if (random_value < 26) { + suffix.push_back('a' + random_value); + } else if (random_value < 2 * 26) { + suffix.push_back('A' + random_value - 26); + } else { + suffix.push_back('0' + random_value - 2 * 26); + } + } + return suffix; +} + } // namespace bool TouchFile(const base::FilePath& path, @@ -162,4 +184,133 @@ bool TouchFile(const base::FilePath& path) { return TouchFile(path, kPermissions600, geteuid(), getegid()); } +bool WriteBlobToFile(const base::FilePath& path, const Blob& blob) { + return WriteToFile(path, reinterpret_cast(blob.data()), + blob.size()); +} + +bool WriteStringToFile(const base::FilePath& path, const std::string& data) { + return WriteToFile(path, data.data(), data.size()); +} + +bool WriteToFile(const base::FilePath& path, const char* data, size_t size) { + if (!base::DirectoryExists(path.DirName())) { + if (!base::CreateDirectory(path.DirName())) { + LOG(ERROR) << "Cannot create directory: " << path.DirName().value(); + return false; + } + } + // base::WriteFile takes an int size. + if (size > std::numeric_limits::max()) { + LOG(ERROR) << "Cannot write to " << path.value() + << ". Data is too large: " << size << " bytes."; + return false; + } + + int data_written = base::WriteFile(path, data, size); + return data_written == static_cast(size); +} + +bool SyncFileOrDirectory(const base::FilePath& path, + bool is_directory, + bool data_sync) { + const base::TimeTicks start = base::TimeTicks::Now(); + data_sync = data_sync && !is_directory; + + int flags = (is_directory ? O_RDONLY | O_DIRECTORY : O_WRONLY); + int fd = HANDLE_EINTR(open(path.value().c_str(), flags)); + if (fd < 0) { + PLOG(WARNING) << "Could not open " << path.value() << " for syncing"; + return false; + } + // POSIX specifies EINTR as a possible return value of fsync() but not for + // fdatasync(). To be on the safe side, it is handled in both cases. + int result = + (data_sync ? HANDLE_EINTR(fdatasync(fd)) : HANDLE_EINTR(fsync(fd))); + if (result < 0) { + PLOG(WARNING) << "Failed to sync " << path.value(); + close(fd); + return false; + } + // close() may not be retried on error. + result = IGNORE_EINTR(close(fd)); + if (result < 0) { + PLOG(WARNING) << "Failed to close after sync " << path.value(); + return false; + } + + const base::TimeDelta delta = base::TimeTicks::Now() - start; + if (delta > kLongSync) { + LOG(WARNING) << "Long " << (data_sync ? "fdatasync" : "fsync") << "() of " + << path.value() << ": " << delta.InSeconds() << " seconds"; + } + + return true; +} + +bool WriteToFileAtomic(const base::FilePath& path, + const char* data, + size_t size, + mode_t mode) { + if (!base::DirectoryExists(path.DirName())) { + if (!base::CreateDirectory(path.DirName())) { + LOG(ERROR) << "Cannot create directory: " << path.DirName().value(); + return false; + } + } + std::string random_suffix = GetRandomSuffix(); + if (random_suffix.empty()) { + PLOG(WARNING) << "Could not compute random suffix"; + return false; + } + std::string temp_name = path.AddExtension(random_suffix).value(); + int fd = + HANDLE_EINTR(open(temp_name.c_str(), O_CREAT | O_EXCL | O_WRONLY, mode)); + if (fd < 0) { + PLOG(WARNING) << "Could not open " << temp_name << " for atomic write"; + unlink(temp_name.c_str()); + return false; + } + + size_t position = 0; + while (position < size) { + ssize_t bytes_written = + HANDLE_EINTR(write(fd, data + position, size - position)); + if (bytes_written < 0) { + PLOG(WARNING) << "Could not write " << temp_name; + close(fd); + unlink(temp_name.c_str()); + return false; + } + position += bytes_written; + } + + if (HANDLE_EINTR(fdatasync(fd)) < 0) { + PLOG(WARNING) << "Could not fsync " << temp_name; + close(fd); + unlink(temp_name.c_str()); + return false; + } + if (close(fd) < 0) { + PLOG(WARNING) << "Could not close " << temp_name; + unlink(temp_name.c_str()); + return false; + } + + if (rename(temp_name.c_str(), path.value().c_str()) < 0) { + PLOG(WARNING) << "Could not close " << temp_name; + unlink(temp_name.c_str()); + return false; + } + + return true; +} + +bool WriteBlobToFileAtomic(const base::FilePath& path, + const Blob& blob, + mode_t mode) { + return WriteToFileAtomic(path, reinterpret_cast(blob.data()), + blob.size(), mode); +} + } // namespace brillo diff --git a/brillo/file_utils.h b/brillo/file_utils.h index 60e0cdc..663d640 100644 --- a/brillo/file_utils.h +++ b/brillo/file_utils.h @@ -9,6 +9,7 @@ #include #include +#include namespace brillo { @@ -30,6 +31,53 @@ BRILLO_EXPORT bool TouchFile(const base::FilePath& path, // bit set. BRILLO_EXPORT bool TouchFile(const base::FilePath& path); +// Writes the entirety of the given data to |path| with 0640 permissions +// (modulo umask). If missing, parent (and parent of parent etc.) directories +// are created with 0700 permissions (modulo umask). Returns true on success. +// +// Parameters +// path - Path of the file to write +// blob/data - blob/string/array to populate from +// (size - array size) +BRILLO_EXPORT bool WriteBlobToFile(const base::FilePath& path, + const Blob& blob); +BRILLO_EXPORT bool WriteStringToFile(const base::FilePath& path, + const std::string& data); +BRILLO_EXPORT bool WriteToFile(const base::FilePath& path, + const char* data, + size_t size); + +// Calls fdatasync() on file if data_sync is true or fsync() on directory or +// file when data_sync is false. Returns true on success. +// +// Parameters +// path - File/directory to be sync'ed +// is_directory - True if |path| is a directory +// data_sync - True if |path| does not need metadata to be synced +BRILLO_EXPORT bool SyncFileOrDirectory(const base::FilePath& path, + bool is_directory, + bool data_sync); + +// Atomically writes the entirety of the given data to |path| with |mode| +// permissions (modulo umask). If missing, parent (and parent of parent etc.) +// directories are created with 0700 permissions (modulo umask). Returns true +// if the file has been written successfully and it has physically hit the +// disk. Returns false if either writing the file has failed or if it cannot +// be guaranteed that it has hit the disk. +// +// Parameters +// path - Path of the file to write +// blob/data - blob/array to populate from +// (size - array size) +// mode - File permission bit-pattern, eg. 0644 for rw-r--r-- +BRILLO_EXPORT bool WriteBlobToFileAtomic(const base::FilePath& path, + const Blob& blob, + mode_t mode); +BRILLO_EXPORT bool WriteToFileAtomic(const base::FilePath& path, + const char* data, + size_t size, + mode_t mode); + } // namespace brillo #endif // LIBBRILLO_BRILLO_FILE_UTILS_H_ diff --git a/brillo/file_utils_unittest.cc b/brillo/file_utils_unittest.cc index f8898a0..96c1fd7 100644 --- a/brillo/file_utils_unittest.cc +++ b/brillo/file_utils_unittest.cc @@ -11,10 +11,28 @@ #include #include +#include +#include #include namespace brillo { +namespace { + +constexpr int kPermissions600 = + base::FILE_PERMISSION_READ_BY_USER | base::FILE_PERMISSION_WRITE_BY_USER; +constexpr int kPermissions700 = base::FILE_PERMISSION_USER_MASK; +constexpr int kPermissions777 = base::FILE_PERMISSION_MASK; + +std::string GetRandomSuffix() { + const int kBufferSize = 6; + unsigned char buffer[kBufferSize]; + base::RandBytes(buffer, arraysize(buffer)); + return base::HexEncode(buffer, arraysize(buffer)); +} + +} // namespace + class FileUtilsTest : public testing::Test { public: FileUtilsTest() { @@ -47,19 +65,13 @@ class FileUtilsTest : public testing::Test { EXPECT_TRUE(base::GetPosixFilePermissions(file_path_, &actual_permissions)); EXPECT_EQ(permissions, actual_permissions); } -}; - -namespace { -enum { - kPermissions600 = - base::FILE_PERMISSION_READ_BY_USER | base::FILE_PERMISSION_WRITE_BY_USER, - kPermissions700 = base::FILE_PERMISSION_USER_MASK, - kPermissions777 = base::FILE_PERMISSION_MASK + // Creates a file with a random name in the temporary directory. + base::FilePath GetTempName() { + return temp_dir_.path().Append(GetRandomSuffix()); + } }; -} // namespace - TEST_F(FileUtilsTest, TouchFileCreate) { EXPECT_TRUE(TouchFile(file_path_)); ExpectFileContains(""); @@ -132,4 +144,102 @@ TEST_F(FileUtilsTest, TouchFileExistingPermissionsUnchanged) { ExpectFilePermissions(kPermissions777); } +TEST_F(FileUtilsTest, WriteFileCanBeReadBack) { + const base::FilePath filename(GetTempName()); + const std::string content("blablabla"); + EXPECT_TRUE(WriteStringToFile(filename, content)); + std::string output; + EXPECT_TRUE(ReadFileToString(filename, &output)); + EXPECT_EQ(content, output); +} + +TEST_F(FileUtilsTest, WriteFileSets0666) { + const mode_t mask = 0000; + const mode_t mode = 0666; + const base::FilePath filename(GetTempName()); + const std::string content("blablabla"); + const mode_t old_mask = umask(mask); + EXPECT_TRUE(WriteStringToFile(filename, content)); + int file_mode = 0; + EXPECT_TRUE(base::GetPosixFilePermissions(filename, &file_mode)); + EXPECT_EQ(mode & ~mask, file_mode & 0777); + umask(old_mask); +} + +TEST_F(FileUtilsTest, WriteFileCreatesMissingParentDirectoriesWith0700) { + const mode_t mask = 0000; + const mode_t mode = 0700; + const base::FilePath dirname(GetTempName()); + const base::FilePath subdirname(dirname.Append(GetRandomSuffix())); + const base::FilePath filename(subdirname.Append(GetRandomSuffix())); + const std::string content("blablabla"); + EXPECT_TRUE(WriteStringToFile(filename, content)); + int dir_mode = 0; + int subdir_mode = 0; + EXPECT_TRUE(base::GetPosixFilePermissions(dirname, &dir_mode)); + EXPECT_TRUE(base::GetPosixFilePermissions(subdirname, &subdir_mode)); + EXPECT_EQ(mode & ~mask, dir_mode & 0777); + EXPECT_EQ(mode & ~mask, subdir_mode & 0777); + const mode_t old_mask = umask(mask); + umask(old_mask); +} + +TEST_F(FileUtilsTest, WriteToFileAtomicCanBeReadBack) { + const base::FilePath filename(GetTempName()); + const std::string content("blablabla"); + EXPECT_TRUE( + WriteToFileAtomic(filename, content.data(), content.size(), 0644)); + std::string output; + EXPECT_TRUE(ReadFileToString(filename, &output)); + EXPECT_EQ(content, output); +} + +TEST_F(FileUtilsTest, WriteToFileAtomicHonorsMode) { + const mode_t mask = 0000; + const mode_t mode = 0616; + const base::FilePath filename(GetTempName()); + const std::string content("blablabla"); + const mode_t old_mask = umask(mask); + EXPECT_TRUE( + WriteToFileAtomic(filename, content.data(), content.size(), mode)); + int file_mode = 0; + EXPECT_TRUE(base::GetPosixFilePermissions(filename, &file_mode)); + EXPECT_EQ(mode & ~mask, file_mode & 0777); + umask(old_mask); +} + +TEST_F(FileUtilsTest, WriteToFileAtomicHonorsUmask) { + const mode_t mask = 0073; + const mode_t mode = 0777; + const base::FilePath filename(GetTempName()); + const std::string content("blablabla"); + const mode_t old_mask = umask(mask); + EXPECT_TRUE( + WriteToFileAtomic(filename, content.data(), content.size(), mode)); + int file_mode = 0; + EXPECT_TRUE(base::GetPosixFilePermissions(filename, &file_mode)); + EXPECT_EQ(mode & ~mask, file_mode & 0777); + umask(old_mask); +} + +TEST_F(FileUtilsTest, + WriteToFileAtomicCreatesMissingParentDirectoriesWith0700) { + const mode_t mask = 0000; + const mode_t mode = 0700; + const base::FilePath dirname(GetTempName()); + const base::FilePath subdirname(dirname.Append(GetRandomSuffix())); + const base::FilePath filename(subdirname.Append(GetRandomSuffix())); + const std::string content("blablabla"); + EXPECT_TRUE( + WriteToFileAtomic(filename, content.data(), content.size(), 0777)); + int dir_mode = 0; + int subdir_mode = 0; + EXPECT_TRUE(base::GetPosixFilePermissions(dirname, &dir_mode)); + EXPECT_TRUE(base::GetPosixFilePermissions(subdirname, &subdir_mode)); + EXPECT_EQ(mode & ~mask, dir_mode & 0777); + EXPECT_EQ(mode & ~mask, subdir_mode & 0777); + const mode_t old_mask = umask(mask); + umask(old_mask); +} + } // namespace brillo -- cgit v1.2.3