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

content/browser/download/base_file_posix.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/base_file.h"
 
 #include "base/files/file_util.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 
 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, create it. The kernel will apply the
+  // user's umask to the mode 0666.
+  mode_t mode = 0600;
   if (!base::PathExists(new_path)) {
-    int write_error = base::WriteFile(new_path, "", 0);
-    if (write_error < 0)
+    struct stat status;
+    if (!base::WriteFileWithMode(new_path, "", 0, 0666) ||

[asanka, 2014/10/24 18:15:46]

I misspoke in my earlier comment regarding using 0666 as the initial mode. What
about 0644 to be conservative? I believe that's what it used to be before the
change to base::WriteFile().

[palmer, 2014/10/24 18:35:35]

The bug this CL fixes is to respect the user's umask for downloads. 0644 does
not do that.

We now have WriteFile and WriteFileMode; WriteFile remains conservative, using
mode 0640.

[asanka, 2014/10/28 21:24:20]

Acknowledged.

+        stat(new_path.value().c_str(), &status)) {
       return LogSystemError("WriteFile", errno);
+    }
+    mode = status.st_mode & 0777;

[asanka, 2014/10/24 18:15:46]

If a file exists at |new_path| we should use the mode from that file rather than
using 0600. That way we won't change the mode if the user chooses to overwrite
an existing file.

[palmer, 2014/10/24 18:35:35]

I think that would surprise people.

[asanka, 2014/10/28 21:24:20]

Would it? Let's say someone is downloading foo.txt to their downloads directory
and overwriting each time. The first time it would get a mode of 0666 & ~umask
(say it's 0640). The second time around, the file mode would be 0600.

[palmer, 2014/10/29 01:12:38]

Ahh, yes. I see what you mean now. You are right. New patchset coming up.

   }
-  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 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 (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 |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;
 }
 
 }  // namespace content