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

@@ -9,9 +9,10 @@
 #include <gflags/gflags.h>
 #include <algorithm>
 #include <memory>
-#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, FileCloser> 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)) {

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.

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) {

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<int32_t>(base::GetCurrentProcId());
   const int32_t tid = static_cast<int32_t>(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