Add POSIX shared memory support for Mac

base/memory/shared_memory_helper.cc

+// Copyright 2016 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_helper.h"
+
+#include "base/threading/thread_restrictions.h"
+
+namespace base {
+
+#if !defined(OS_ANDROID)
+bool CreateAnonymousSharedMemory(const SharedMemoryCreateOptions& options,
+                                 ScopedFILE* fp,
+                                 ScopedFD* readonly_fd,
+                                 FilePath* path) {
+#if !(defined(OS_MACOSX) && !defined(OS_IOS))
+  // It doesn't make sense to have a open-existing private piece of shmem
+  DCHECK(!options.open_existing_deprecated);
+#endif  // !(defined(OS_MACOSX) && !defined(OS_IOS)
+  // Q: Why not use the shm_open() etc. APIs?
+  // A: Because they're limited to 4mb on OS X.  FFFFFFFUUUUUUUUUUU
+  FilePath directory;
+  ScopedPathUnlinker path_unlinker;
+  if (GetShmemTempDir(options.executable, &directory)) {
+    fp->reset(base::CreateAndOpenTemporaryFileInDir(directory, path));
+
+    // 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)
+      path_unlinker.reset(path);

[Mark Mentovai, 2016/12/09 19:53:52]

So you unlink path here…

+  }
+
+  if (*fp && options.share_read_only) {

[Mark Mentovai, 2016/12/09 21:21:30]

OK, I see. A lot of related but distinct objects in here, it’s hard to keep
track.

Now I’m concerned that *fp might not be false if GetShmemTempDir() failed. Since
so much in here is done with out-parameters, they can arrive in this function
with any caller-controlled value. I think we should play it safer.

[lawrencewu, 2016/12/09 22:08:22]

OK, I restructured the code so it uses more early returns, and directly check
for GetShmemTempDir().

+    // Also open as readonly so that we can ShareReadOnlyToProcess.
+    readonly_fd->reset(HANDLE_EINTR(open(path->value().c_str(), O_RDONLY)));

[Mark Mentovai, 2016/12/09 19:53:52]

…then will you try to reopen it here?

[lawrencewu, 2016/12/09 20:32:12]

I think we have to re-open it as readonly here. Are you suggesting having two
separate codepaths so one doesn't have to re-open?

[Mark Mentovai, 2016/12/09 20:54:13]

lawrencewu wrote:
I mean that it may have been unlinked by this point.

[lawrencewu, 2016/12/09 21:14:31]

So the code is kind of confusing but I don't think the path gets unlinked on
line 31. The reset will abort before trying to free/unlink the path.

+    if (!readonly_fd->is_valid()) {
+      DPLOG(ERROR) << "open(\"" << path->value() << "\", O_RDONLY) failed";
+      fp->reset();
+      return false;
+    }
+  }
+  return true;

[Mark Mentovai, 2016/12/09 21:21:30]

Another problem is that if (!*fp), we probably don’t want to return true.

How about restructuring this function to use early returns more instead of so
much nested conditional stuff? Aside from options.share_read_only, the only real
conditions that are happening in here seem to be testing for failure.

[lawrencewu, 2016/12/09 22:08:22]

Done.

+}
+
+bool PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd, int* mapped_file,
+                    int* readonly_mapped_file) {
+  DCHECK_EQ(-1, *mapped_file);
+  DCHECK_EQ(-1, *readonly_mapped_file);
+  if (fp == NULL)
+    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;
+
+  struct stat st = {};
+  if (fstat(fileno(fp.get()), &st))

[Mark Mentovai, 2016/12/09 19:53:52]

What’s the point of doing this if readonly_fd.is_valid() isn’t true?

[lawrencewu, 2016/12/09 20:32:12]

Moved to inside the if statement.

+    NOTREACHED();
+  if (readonly_fd.is_valid()) {
+    struct stat readonly_st = {};
+    if (fstat(readonly_fd.get(), &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 = HANDLE_EINTR(dup(fileno(fp.get())));
+  if (*mapped_file == -1) {
+    if (errno == EMFILE) {

[Mark Mentovai, 2016/12/09 19:53:52]

Is it worth distinguishing EMFILE? dup() failing is bad no matter what.

[lawrencewu, 2016/12/09 20:32:12]

Removed the check for EMFILE.

+      LOG(WARNING) << "Shared memory creation failed; out of file descriptors";
+      return false;
+    }
+    NOTREACHED() << "Call to dup failed, errno=" << errno;
+  }
+  *readonly_mapped_file = readonly_fd.release();
+
+  return true;
+}
+#endif  // !defined(OS_ANDROID)
+
+}  // namespace base