Posix: fix named SHM mappings permissions.

base/memory/shared_memory_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 "base/memory/shared_memory.h"
 
 #include <errno.h>
 #include <fcntl.h>
+#include <fcntl.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
+#include <sys/stat.h>
+#include <sys/types.h>
 #include <unistd.h>
 
 #include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/process_util.h"
 #include "base/safe_strerror_posix.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/platform_thread.h"
@@ skipping 121 matching lines @@
     // private), and prevents the need for cleanup (once the last fd is closed,
     // it is truly freed).
     if (fp) {
       if (unlink(path.value().c_str()))
         PLOG(WARNING) << "unlink";
     }
   } else {
     if (!FilePathForMemoryName(*options.name, &path))
       return false;
 
-    fp = file_util::OpenFile(path, "w+x");
-    if (fp == NULL && options.open_existing) {
-      // "w+" will truncate if it already exists.
-      fp = file_util::OpenFile(path, "a+");
+    // Make sure that we don't give permissions to access this file
+    // to other users on the system.
+    const mode_t file_mode = S_IRUSR | S_IWUSR;
+    int fd;
+    // First, try to create the file.
+    fd = open(path.value().c_str(), O_RDWR | O_CREAT | O_EXCL, file_mode);
+    if (fd == -1 && options.open_existing) {
+      // If this doesn't work, try again in append mode.
+      fd = open(path.value().c_str(), O_RDWR | O_APPEND, file_mode);

[Markus (顧孟勤), 2013/06/26 12:28:22]

Do not pass in file_mode. That's just misleading your reviewers.

The mode is ignored by open() unless you also set O_CREAT.

Having said that, now might be a good time to call fstat() to verify that the
permissions are set the way we would like them to be set. You might even want to
check the owner, if you are paranoid.

Otherwise, there is a race condition where somebody other than the owner could
create a world-writable file and trick you into using it for your shared memory.

[jln (very slow on Chromium), 2013/07/02 02:37:06]

Done.

       fix_size = false;
     }
+    fp = NULL;
+    if (fd >= 0) {
+      // "a+" is always appropriate: if it's a new file, a+ is similar to w+.
+      fp = fdopen(fd, "a+");
+    }
   }
   if (fp && fix_size) {
     // Get current size.
     struct stat stat;
     if (fstat(fileno(fp), &stat) != 0) {
       file_util::CloseFile(fp);
       return false;
     }
     const size_t current_size = stat.st_size;
     if (current_size != options.size) {

[Markus (顧孟勤), 2013/06/26 12:28:22]

I realize this isn't code that you wrote, but overall this is really silly.

We know with 100% certainty that if we get here, the size of the file is going
to be zero. We really should simplify the code and get rid of the call to
fstat(), get rid of the comparison for sizes, get rid of "fix_size", and always
ftruncate() if open(O_EXCL) succeeded.

That would make this function a lot more readable.

[jln (very slow on Chromium), 2013/07/02 02:37:06]

Unfortunately no, CreateAndOpenTemporaryShmemFile() is another case where
fix_size will be true.

I added a comment to make this code a bit more clear.

       if (HANDLE_EINTR(ftruncate(fileno(fp), options.size)) != 0) {
         file_util::CloseFile(fp);
         return false;
       }
     }
     requested_size_ = options.size;
   }
   if (fp == NULL) {
 #if !defined(OS_MACOSX)
     PLOG(ERROR) << "Creating shared memory in " << path.value() << " failed";
@@ skipping 201 matching lines @@
   new_handle->fd = new_fd;
   new_handle->auto_close = true;
 
   if (close_self)
     Close();
 
   return true;
 }
 
 }  // namespace base