From 9e3dc06d201dc00332094c4f462e70a57c72a8bc Mon Sep 17 00:00:00 2001 From: KongQun Yang Date: Tue, 1 Aug 2017 15:34:20 -0700 Subject: [PATCH] Fix file permission issue with manifests ImportantFileWriter::WriteFileAtomically uses mkstemp internally, which set file permission to 0600, which is not what we want. Update the code to not use mkstemp instead. Also updated temporary file name logic to include process id in the name so it can be unique across processes. Fixes #259 Change-Id: I2d5a375925cf552bc0db5269f409d7522e63fca5 --- packager/file/file.cc | 62 +++++++++++++++++++++++----------- packager/file/file.h | 9 ++++- packager/file/file_unittest.cc | 9 +++++ packager/file/file_util.cc | 7 ++-- 4 files changed, 64 insertions(+), 23 deletions(-) diff --git a/packager/file/file.cc b/packager/file/file.cc index 35273a4163..a0f5aa62ac 100644 --- a/packager/file/file.cc +++ b/packager/file/file.cc @@ -9,9 +9,10 @@ #include #include #include -#include "packager/base/files/important_file_writer.h" +#include "packager/base/files/file_util.h" #include "packager/base/logging.h" #include "packager/base/strings/string_piece.h" +#include "packager/file/file_util.h" #include "packager/file/local_file.h" #include "packager/file/memory_file.h" #include "packager/file/threaded_io_file.h" @@ -61,8 +62,21 @@ bool DeleteLocalFile(const char* file_name) { bool WriteLocalFileAtomically(const char* file_name, const std::string& contents) { - return base::ImportantFileWriter::WriteFileAtomically( - base::FilePath::FromUTF8Unsafe(file_name), contents); + const base::FilePath file_path = base::FilePath::FromUTF8Unsafe(file_name); + const std::string dir_name = file_path.DirName().AsUTF8Unsafe(); + std::string temp_file_name; + if (!TempFilePath(dir_name, &temp_file_name)) + return false; + if (!File::WriteStringToFile(temp_file_name.c_str(), contents)) + return false; + base::File::Error replace_file_error = base::File::FILE_OK; + if (!base::ReplaceFile(base::FilePath::FromUTF8Unsafe(temp_file_name), + file_path, &replace_file_error)) { + LOG(ERROR) << "Failed to replace file '" << file_name << "' with '" + << temp_file_name << "', error: " << replace_file_error; + return false; + } + return true; } File* CreateUdpFile(const char* file_name, const char* mode) { @@ -202,23 +216,8 @@ bool File::ReadFileToString(const char* file_name, std::string* contents) { return len == 0; } -bool File::WriteFileAtomically(const char* file_name, - const std::string& contents) { - base::StringPiece real_file_name; - const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name); - DCHECK(file_type); - if (file_type->atomic_write_function) - return file_type->atomic_write_function(real_file_name.data(), contents); - - // Provide a default implementation which may not be atomic unfortunately. - - // Skip the warning message for memory files, which is meant for testing - // anyway.. - if (strncmp(file_name, kMemoryFilePrefix, strlen(kMemoryFilePrefix)) != 0) { - LOG(WARNING) << "Writing to " << file_name - << " is not guaranteed to be atomic."; - } - +bool File::WriteStringToFile(const char* file_name, + const std::string& contents) { std::unique_ptr file(File::Open(file_name, "w")); if (!file) { LOG(ERROR) << "Failed to open file " << file_name; @@ -236,9 +235,32 @@ bool File::WriteFileAtomically(const char* file_name, << contents.size() << " bytes."; return false; } + if (!file->Flush()) { + LOG(ERROR) << "Failed to flush the file '" << file_name << "'."; + return false; + } return true; } +bool File::WriteFileAtomically(const char* file_name, + const std::string& contents) { + base::StringPiece real_file_name; + const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name); + DCHECK(file_type); + if (file_type->atomic_write_function) + return file_type->atomic_write_function(real_file_name.data(), contents); + + // Provide a default implementation which may not be atomic unfortunately. + + // Skip the warning message for memory files, which is meant for testing + // anyway.. + if (strncmp(file_name, kMemoryFilePrefix, strlen(kMemoryFilePrefix)) != 0) { + LOG(WARNING) << "Writing to " << file_name + << " is not guaranteed to be atomic."; + } + return WriteStringToFile(file_name, contents); +} + bool File::Copy(const char* from_file_name, const char* to_file_name) { std::string content; if (!ReadFileToString(from_file_name, &content)) { diff --git a/packager/file/file.h b/packager/file/file.h index 28c5687850..a429689e38 100644 --- a/packager/file/file.h +++ b/packager/file/file.h @@ -97,12 +97,19 @@ class File { /// The file will be opened and closed in the process. static int64_t GetFileSize(const char* file_name); - /// Read the file into string. + /// Read the contents of a file into string. /// @param file_name is the file to be read. It should be a valid file. /// @param contents[out] points to the output string. Should not be NULL. /// @return true on success, false otherwise. static bool ReadFileToString(const char* file_name, std::string* contents); + /// Writes the data to file. + /// @param file_name is the file to write to. + /// @param contents is the data to write. + /// @return true on success, false otherwise. + static bool WriteStringToFile(const char* file_name, + const std::string& contents); + /// Save `contents` to `file_name` in an atomic manner. /// @param file_name is the destination file name. /// @param contents is the data to be saved. diff --git a/packager/file/file_unittest.cc b/packager/file/file_unittest.cc index 460dd443fb..fa6774f9e6 100644 --- a/packager/file/file_unittest.cc +++ b/packager/file/file_unittest.cc @@ -154,6 +154,15 @@ TEST_F(LocalFileTest, WriteRead) { EXPECT_EQ(data_, read_data); } +TEST_F(LocalFileTest, WriteStringReadString) { + ASSERT_TRUE( + File::WriteStringToFile(local_file_name_no_prefix_.c_str(), data_)); + std::string read_data; + ASSERT_TRUE( + File::ReadFileToString(local_file_name_no_prefix_.c_str(), &read_data)); + EXPECT_EQ(data_, read_data); +} + // There is no easy way to test if a write operation is atomic. This test only // ensures the data is written correctly. TEST_F(LocalFileTest, AtomicWriteRead) { diff --git a/packager/file/file_util.cc b/packager/file/file_util.cc index 7e1ecca715..7eb4e55546 100644 --- a/packager/file/file_util.cc +++ b/packager/file/file_util.cc @@ -10,17 +10,20 @@ #include "packager/base/files/file_path.h" #include "packager/base/files/file_util.h" +#include "packager/base/process/process_handle.h" #include "packager/base/strings/stringprintf.h" #include "packager/base/threading/platform_thread.h" #include "packager/base/time/time.h" namespace shaka { namespace { -// Create a temp file name using process/thread id and current time. +// Create a temp file name using process id, thread id and current time. std::string TempFileName() { + const int32_t pid = static_cast(base::GetCurrentProcId()); const int32_t tid = static_cast(base::PlatformThread::CurrentId()); const int64_t current_time = base::Time::Now().ToInternalValue(); - return base::StringPrintf("packager-tempfile-%x-%" PRIx64, tid, current_time); + return base::StringPrintf("packager-tempfile-%x-%x-%" PRIx64, pid, tid, + current_time); } } // namespace