Allow POSIX callers to specify a new file's mode.

content/browser/download/base_file_posix.cc

Index: content/browser/download/base_file_posix.cc
diff --git a/content/browser/download/base_file_posix.cc b/content/browser/download/base_file_posix.cc
index b5d8e01442244b0f0a4fb45b60d3060618bb4786..ebdb92e3bead8d76eeaf047a91c09d0d88194f8a 100644
--- a/content/browser/download/base_file_posix.cc
+++ b/content/browser/download/base_file_posix.cc
@@ -11,31 +11,32 @@ namespace content {
 
 DownloadInterruptReason BaseFile::MoveFileAndAdjustPermissions(
     const base::FilePath& new_path) {
-  // Similarly, on Unix, we're moving a temp file created with permissions 600
-  // to |new_path|. Here, we try to fix up the destination file with appropriate
-  // permissions.
-  struct stat st;
-  // First check the file existence and create an empty file if it doesn't
-  // exist.
+  // Move a temporary file created with mode 0600 to |new_path|. If
+  // |new_path| does not already exist, set the correct mode on the new file
+  // (honoring the user's umask).
+  mode_t mask = umask(0);

[asanka, 2014/10/23 06:36:16]

Ruh roh. I think we should avoid a umask(0) call since our worker pool etc.
might be busy creating files elsewhere.

AIUI, creat() will honor the umask. So a stat() following a
WriteFileWithMode(...0666) should have sufficed?

[palmer, 2014/10/23 23:25:24]

Oh yeah, you're right. Sorry about that.

The bummer is there is no way to simply *get* the umask; you have to change it
and then change it back. Between those 2 actions, other threads might have done
various things. So, I don't think we can effectively stat-and-check without
creating the same race condition. Also, you are right that the umask is applied
as part of creat(2), so unless the kernel misbehaves, we got what we want.

+  (void) umask(mask);
+  mode_t mode = 0666 & ~mask;
+
   if (!base::PathExists(new_path)) {
-    int write_error = base::WriteFile(new_path, "", 0);
-    if (write_error < 0)
+    if (!base::WriteFileWithMode(new_path, "", 0, mode))
       return LogSystemError("WriteFile", errno);
   }
-  int stat_error = stat(new_path.value().c_str(), &st);
-  bool stat_succeeded = (stat_error == 0);
-  if (!stat_succeeded)
-    LogSystemError("stat", errno);
-
-  if (!base::Move(full_path_, new_path))
-    return LogSystemError("Move", errno);
-
-  if (stat_succeeded) {
-    // On Windows file systems (FAT, NTFS), chmod fails.  This is OK.
-    int chmod_error = chmod(new_path.value().c_str(), st.st_mode);
-    if (chmod_error < 0)
-      LogSystemError("chmod", errno);
+
+  // If rename(2) fails, fall back to base::Move.
+  if (rename(full_path_.value().c_str(), new_path.value().c_str())) {
+    if (!base::Move(full_path_, new_path))
+      return LogSystemError("Move", errno);
   }
+
+  // If |base::Move| had to copy the file (e.g. because the source is on a
+  // different volume than |new_path|, we must re-set the mode. This is
+  // racy but may be the best we can do.
+  //
+  // On Windows file systems (FAT, NTFS), chmod fails. This is OK.
+  if (chmod(new_path.value().c_str(), mode))
+    (void) LogSystemError("chmod", errno);
+
   return DOWNLOAD_INTERRUPT_REASON_NONE;
 }