Implement SharedMemory::NewAnonymousReadOnly(contents).

base/memory/shared_memory_posix.cc

Index: base/memory/shared_memory_posix.cc
diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
index efb0caf5bc9a29d805700c3abdad18ad6671a199..3443963aecf75b4709c2fe518fea4874ce4702f6 100644
--- a/base/memory/shared_memory_posix.cc
+++ b/base/memory/shared_memory_posix.cc
@@ -30,6 +30,9 @@
 #include "third_party/ashmem/ashmem.h"
 #endif
 
+using file_util::ScopedFD;
+using file_util::ScopedFILE;
+
 namespace base {
 
 namespace {
@@ -40,6 +43,7 @@ LazyInstance<Lock>::Leaky g_thread_lock_ = LAZY_INSTANCE_INITIALIZER;
 
 SharedMemory::SharedMemory()
     : mapped_file_(-1),
+      readonly_mapped_file_(-1),
       inode_(0),
       mapped_size_(0),
       memory_(NULL),
@@ -49,6 +53,7 @@ SharedMemory::SharedMemory()
 
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
     : mapped_file_(handle.fd),
+      readonly_mapped_file_(-1),
       inode_(0),
       mapped_size_(0),
       memory_(NULL),
@@ -65,6 +70,7 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
                            ProcessHandle process)
     : mapped_file_(handle.fd),
+      readonly_mapped_file_(-1),
       inode_(0),
       mapped_size_(0),
       memory_(NULL),
@@ -124,8 +130,10 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
   // and be deleted before they ever make it out to disk.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
 
-  FILE *fp;
+  ScopedFILE fp;
   bool fix_size = true;
+  int readonly_fd_storage = -1;
+  ScopedFD readonly_fd(&readonly_fd_storage);
 
   FilePath path;
   if (options.name == NULL || options.name->empty()) {
@@ -133,12 +141,21 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     DCHECK(!options.open_existing);
     // Q: Why not use the shm_open() etc. APIs?
     // A: Because they're limited to 4mb on OS X.  FFFFFFFUUUUUUUUUUU
-    fp = file_util::CreateAndOpenTemporaryShmemFile(&path, options.executable);
+    fp.reset(
+        file_util::CreateAndOpenTemporaryShmemFile(&path, options.executable));
 
-    // Deleting the file prevents anyone else from mapping it in (making it
-    // private), and prevents the need for cleanup (once the last fd is closed,
-    // it is truly freed).
     if (fp) {
+      // Also open as readonly so that we can ShareReadOnlyToProcess.
+      *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+      if (*readonly_fd < 0) {
+        DPLOG(ERROR) << "open(\"" << path.value().c_str()
+                     << "\", O_RDONLY) failed";
+        fp.reset();
+      }
+      // Deleting the file prevents anyone else from mapping it in (making it
+      // private), and prevents the need for cleanup (once the last fd is
+      // closed,

[Mark Mentovai, 2013/11/20 14:21:41]

Reflow this comment, this word doesn’t need to be so lonely.

[Jeffrey Yasskin, 2013/11/20 18:02:09]

Done.

+      // it is truly freed).
       if (unlink(path.value().c_str()))
         PLOG(WARNING) << "unlink";
     }
@@ -182,25 +199,29 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
       // An existing file was opened, so its size should not be fixed.
       fix_size = false;
     }
-    fp = NULL;
+
+    // Also open as readonly so that we can ShareReadOnlyToProcess.
+    *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+    if (*readonly_fd < 0) {
+      DPLOG(ERROR) << "open(\"" << path.value().c_str()
+                   << "\", O_RDONLY) failed";
+      HANDLE_EINTR(close(fd));
+      fd = -1;
+    }
     if (fd >= 0) {
       // "a+" is always appropriate: if it's a new file, a+ is similar to w+.
-      fp = fdopen(fd, "a+");
+      fp.reset(fdopen(fd, "a+"));
     }
   }
   if (fp && fix_size) {
     // Get current size.
     struct stat stat;
-    if (fstat(fileno(fp), &stat) != 0) {
-      file_util::CloseFile(fp);
+    if (fstat(fileno(fp.get()), &stat) != 0)
       return false;
-    }
     const size_t current_size = stat.st_size;
     if (current_size != options.size) {
-      if (HANDLE_EINTR(ftruncate(fileno(fp), options.size)) != 0) {
-        file_util::CloseFile(fp);
+      if (HANDLE_EINTR(ftruncate(fileno(fp.get()), options.size)) != 0)
         return false;
-      }
     }
     requested_size_ = options.size;
   }
@@ -221,7 +242,7 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     return false;
   }
 
-  return PrepareMapFile(fp);
+  return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
 }
 
 // Our current implementation of shmem is with mmap()ing of files.
@@ -247,8 +268,12 @@ bool SharedMemory::Open(const std::string& name, bool read_only) {
   read_only_ = read_only;
 
   const char *mode = read_only ? "r" : "r+";
-  FILE *fp = file_util::OpenFile(path, mode);
-  return PrepareMapFile(fp);
+  ScopedFILE fp(file_util::OpenFile(path, mode));
+  int readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+  if (readonly_fd < 0) {
+    DPLOG(ERROR) << "open(\"" << path.value().c_str() << "\", O_RDONLY) failed";
+  }
+  return PrepareMapFile(fp.Pass(), ScopedFD(&readonly_fd));

[Mark Mentovai, 2013/11/20 14:21:41]

Move the ScopedFD up (like you did above) to better handle early returns. These
can both be .Pass().

[Jeffrey Yasskin, 2013/11/20 18:02:09]

Done.

 }
 
 #endif  // !defined(OS_ANDROID)
@@ -309,6 +334,11 @@ void SharedMemory::Close() {
       PLOG(ERROR) << "close";
     mapped_file_ = -1;
   }
+  if (readonly_mapped_file_ > 0) {
+    if (HANDLE_EINTR(close(readonly_mapped_file_)) < 0)

[Mark Mentovai, 2013/11/20 14:21:41]

I have determined that it’s never correct to HANDLE_EINTR close.

This is always true on Linux. The story is sadder on Mac, but the bottom line is
that I did some heinous stuff to make it true there too. More details (if you’re
interested) at http://crbug.com/269623 .

I still haven’t done a cleanup of all of the HANDLE_EINTR(close) sites yet, but
new code should be written correctly, so you can fix this line, line 195, line
208, and line 333 while you’re here.

[Jeffrey Yasskin, 2013/11/20 18:02:09]

Oh, ok; done. It was very slightly easier to unwrap all the close()s in the
file, if that's all right.

+      PLOG(ERROR) << "close";
+    readonly_mapped_file_ = -1;
+  }
 }
 
 void SharedMemory::Lock() {
@@ -322,18 +352,28 @@ void SharedMemory::Unlock() {
 }
 
 #if !defined(OS_ANDROID)
-bool SharedMemory::PrepareMapFile(FILE *fp) {
+bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
   DCHECK_EQ(-1, mapped_file_);
-  if (fp == NULL) return false;
+  DCHECK_EQ(-1, readonly_mapped_file_);
+  if (fp == NULL || *readonly_fd < 0) return false;
 
   // This function theoretically can block on the disk, but realistically
   // the temporary files we create will just go into the buffer cache
   // and be deleted before they ever make it out to disk.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
 
-  file_util::ScopedFILE file_closer(fp);
+  struct stat st;
+  struct stat readonly_st;
+  if (fstat(fileno(fp.get()), &st))
+    NOTREACHED();
+  if (fstat(*readonly_fd, &readonly_st))
+    NOTREACHED();
+  if (st.st_dev != readonly_st.st_dev || st.st_ino != readonly_st.st_ino) {
+    LOG(ERROR) << "writable and read-only inodes don't match; bailing";
+    return false;
+  }
 
-  mapped_file_ = dup(fileno(fp));
+  mapped_file_ = dup(fileno(fp.get()));
   if (mapped_file_ == -1) {
     if (errno == EMFILE) {
       LOG(WARNING) << "Shared memory creation failed; out of file descriptors";
@@ -342,11 +382,8 @@ bool SharedMemory::PrepareMapFile(FILE *fp) {
       NOTREACHED() << "Call to dup failed, errno=" << errno;
     }
   }
-
-  struct stat st;
-  if (fstat(mapped_file_, &st))
-    NOTREACHED();
   inode_ = st.st_ino;
+  readonly_mapped_file_ = *readonly_fd.release();
 
   return true;
 }
@@ -399,9 +436,23 @@ void SharedMemory::LockOrUnlockCommon(int function) {
 }
 
 bool SharedMemory::ShareToProcessCommon(ProcessHandle process,
-                                        SharedMemoryHandle *new_handle,
-                                        bool close_self) {
-  const int new_fd = dup(mapped_file_);
+                                        SharedMemoryHandle* new_handle,
+                                        bool close_self,
+                                        ShareMode share_mode) {
+  int handle_to_dup = -1;
+  switch(share_mode) {
+    case SHARE_CURRENT_MODE:
+      handle_to_dup = mapped_file_;
+      break;
+    case SHARE_READONLY:
+      // We could imagine re-opening the file from /dev/fd, but that can't make
+      // it readonly on Mac: https://codereview.chromium.org/27265002/#msg10
+      CHECK(readonly_mapped_file_ >= 0);
+      handle_to_dup = readonly_mapped_file_;
+      break;
+  }
+
+  const int new_fd = dup(handle_to_dup);
   if (new_fd < 0) {
     DPLOG(ERROR) << "dup() failed.";
     return false;