Fix up SharedMemory implementation so that it is more equivalent on Windows vs Posix and enforces exclusive creates.

Clean up some naming to make it clearer what size you are getting by changing max_size to created_size.

BUG=NONE
TEST=BUILD

Review URL: http://codereview.chromium.org/4034006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@64097 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 207 insertions, 135 deletions

diff --git a/base/metrics/stats_table.cc b/base/metrics/stats_table.cc
index 98b2f57..82d6a25 100644
--- a/base/metrics/stats_table.cc
+++ b/base/metrics/stats_table.cc
@@ -161,7 +161,7 @@ StatsTable::Private* StatsTable::Private::New(const std::string& name,
                                               int max_threads,
                                               int max_counters) {
   scoped_ptr<Private> priv(new Private());
-  if (!priv->shared_memory_.Create(name, false, true, size))
+  if (!priv->shared_memory_.CreateNamed(name, true, size))
     return NULL;
   if (!priv->shared_memory_.Map(size))
     return NULL;
diff --git a/base/shared_memory.h b/base/shared_memory.h
index 053026e..79ea8fd 100644
--- a/base/shared_memory.h
+++ b/base/shared_memory.h
@@ -66,14 +66,21 @@ class SharedMemory {
   // Closes a shared memory handle.
   static void CloseHandle(const SharedMemoryHandle& handle);
 
+  // Creates and maps an anonymous shared memory segment of size size.
+  // Returns true on success and false on failure.
+  bool CreateAndMapAnonymous(uint32 size);
+
+  // Creates an anonymous shared memory segment of size size.
+  // Returns true on success and false on failure.
+  bool CreateAnonymous(uint32 size);
+
   // Creates or opens a shared memory segment based on a name.
-  // If read_only is true, opens the memory as read-only.
   // If open_existing is true, and the shared memory already exists,
   // opens the existing shared memory and ignores the size parameter.
-  // If name is the empty string, use a unique name.
+  // If open_existing is false, shared memory must not exist.
+  // size is the size of the block to be created.
   // Returns true on success, false on failure.
-  bool Create(const std::string& name, bool read_only, bool open_existing,
-              uint32 size);
+  bool CreateNamed(const std::string& name, bool open_existing, uint32 size);
 
   // Deletes resources associated with a shared memory segment based on name.
   // Not all platforms require this call.
@@ -81,7 +88,6 @@ class SharedMemory {
 
   // Opens a shared memory segment based on a name.
   // If read_only is true, opens for read-only access.
-  // If name is the empty string, use a unique name.
   // Returns true on success, false on failure.
   bool Open(const std::string& name, bool read_only);
 
@@ -95,12 +101,15 @@ class SharedMemory {
   // memory is not mapped.
   bool Unmap();
 
-  // Get the size of the opened shared memory backing file.
+  // Get the size of the shared memory backing file.
   // Note:  This size is only available to the creator of the
   // shared memory, and not to those that opened shared memory
   // created externally.
-  // Returns 0 if not opened or unknown.
-  uint32 max_size() const { return max_size_; }
+  // Returns 0 if not created or unknown.
+  // Deprecated method, please keep track of the size yourself if you created
+  // it.
+  // http://crbug.com/60821
+  uint32 created_size() const { return created_size_; }
 
   // Gets a pointer to the opened memory space if it has been
   // Mapped via Map().  Returns NULL if it is not mapped.
@@ -161,7 +170,7 @@ class SharedMemory {
 
  private:
 #if defined(OS_POSIX)
-  bool CreateOrOpen(const std::string& name, int posix_flags, uint32 size);
+  bool PrepareMapFile(FILE *fp);
   bool FilePathForMemoryName(const std::string& mem_name, FilePath* path);
   void LockOrUnlockCommon(int function);
 #endif
@@ -174,11 +183,12 @@ class SharedMemory {
   HANDLE             mapped_file_;
 #elif defined(OS_POSIX)
   int                mapped_file_;
+  uint32             mapped_size_;
   ino_t              inode_;
 #endif
   void*              memory_;
   bool               read_only_;
-  uint32             max_size_;
+  uint32             created_size_;
 #if !defined(OS_POSIX)
   SharedMemoryLock   lock_;
 #endif
diff --git a/base/shared_memory_posix.cc b/base/shared_memory_posix.cc
index ae814d7..bc7723d 100644
--- a/base/shared_memory_posix.cc
+++ b/base/shared_memory_posix.cc
@@ -27,18 +27,20 @@ const char kSemaphoreSuffix[] = "-sem";
 
 SharedMemory::SharedMemory()
     : mapped_file_(-1),
+      mapped_size_(0),
       inode_(0),
       memory_(NULL),
       read_only_(false),
-      max_size_(0) {
+      created_size_(0) {
 }
 
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
     : mapped_file_(handle.fd),
+      mapped_size_(0),
       inode_(0),
       memory_(NULL),
       read_only_(read_only),
-      max_size_(0) {
+      created_size_(0) {
   struct stat st;
   if (fstat(handle.fd, &st) == 0) {
     // If fstat fails, then the file descriptor is invalid and we'll learn this
@@ -50,9 +52,11 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
 SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
                            ProcessHandle process)
     : mapped_file_(handle.fd),
+      mapped_size_(0),
+      inode_(0),
       memory_(NULL),
       read_only_(read_only),
-      max_size_(0) {
+      created_size_(0) {
   // We don't handle this case yet (note the ignored parameter); let's die if
   // someone comes calling.
   NOTREACHED();
@@ -78,20 +82,90 @@ void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
   close(handle.fd);
 }
 
-bool SharedMemory::Create(const std::string& name, bool read_only,
-                          bool open_existing, uint32 size) {
-  read_only_ = read_only;
+bool SharedMemory::CreateAndMapAnonymous(uint32 size) {
+  return CreateAnonymous(size) && Map(size);
+}
+
+bool SharedMemory::CreateAnonymous(uint32 size) {
+  return CreateNamed("", false, size);
+}
+
+// Chromium mostly only uses the unique/private shmem as specified by
+// "name == L"". The exception is in the StatsTable.
+// TODO(jrg): there is no way to "clean up" all unused named shmem if
+// we restart from a crash.  (That isn't a new problem, but it is a problem.)
+// In case we want to delete it later, it may be useful to save the value
+// of mem_filename after FilePathForMemoryName().
+bool SharedMemory::CreateNamed(const std::string& name,
+                               bool open_existing, uint32 size) {
+  DCHECK(mapped_file_ == -1);
+  if (size == 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;
 
-  int posix_flags = 0;
-  posix_flags |= read_only ? O_RDONLY : O_RDWR;
-  if (!open_existing || mapped_file_ <= 0)
-    posix_flags |= O_CREAT;
+  FILE *fp;
+  bool fix_size = true;
+
+  FilePath path;
+  if (name.empty()) {
+    // It doesn't make sense to have a open-existing private piece of shmem
+    DCHECK(!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);
 
-  if (!CreateOrOpen(name, posix_flags, size))
+    // 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)
+      file_util::Delete(path, false);
+
+  } else {
+    if (!FilePathForMemoryName(name, &path))
+      return false;
+
+    fp = file_util::OpenFile(path, "w+x");
+    if (fp == NULL && open_existing) {
+      // "w+" will truncate if it already exists.
+      fp = file_util::OpenFile(path, "a+");
+      fix_size = false;
+    }
+  }
+  if (fp && fix_size) {
+    // Get current size.
+    struct stat stat;
+    if (fstat(fileno(fp), &stat) != 0)
+      return false;
+    const uint32 current_size = stat.st_size;
+    if (current_size != size) {
+      if (ftruncate(fileno(fp), size) != 0)
+        return false;
+      if (fseeko(fp, size, SEEK_SET) != 0)
+        return false;
+    }
+    created_size_ = size;
+  }
+  if (fp == NULL) {
+#if !defined(OS_MACOSX)
+    PLOG(ERROR) << "Creating shared memory in " << path.value() << " failed";
+    FilePath dir = path.DirName();
+    if (access(dir.value().c_str(), W_OK | X_OK) < 0) {
+      PLOG(ERROR) << "Unable to access(W_OK|X_OK) " << dir.value();
+      if (dir.value() == "/dev/shm") {
+        LOG(FATAL) << "This is frequently caused by incorrect permissions on "
+        << "/dev/shm.  Try 'sudo chmod 777 /dev/shm' to fix.";
+      }
+    }
+#else
+    PLOG(ERROR) << "Creating shared memory in " << path.value() << " failed";
+#endif
     return false;
+  }
 
-  max_size_ = size;
-  return true;
+  return PrepareMapFile(fp);
 }
 
 // Our current implementation of shmem is with mmap()ing of files.
@@ -111,12 +185,15 @@ bool SharedMemory::Delete(const std::string& name) {
 }
 
 bool SharedMemory::Open(const std::string& name, bool read_only) {
-  read_only_ = read_only;
+  FilePath path;
+  if (!FilePathForMemoryName(name, &path))
+    return false;
 
-  int posix_flags = 0;
-  posix_flags |= read_only ? O_RDONLY : O_RDWR;
+  read_only_ = read_only;
 
-  return CreateOrOpen(name, posix_flags, 0);
+  const char *mode = read_only ? "r" : "r+";
+  FILE *fp = file_util::OpenFile(path, mode);
+  return PrepareMapFile(fp);
 }
 
 // For the given shmem named |mem_name|, return a filename to mmap()
@@ -137,97 +214,16 @@ bool SharedMemory::FilePathForMemoryName(const std::string& mem_name,
   return true;
 }
 
-// Chromium mostly only use the unique/private shmem as specified by
-// "name == L"". The exception is in the StatsTable.
-// TODO(jrg): there is no way to "clean up" all unused named shmem if
-// we restart from a crash.  (That isn't a new problem, but it is a problem.)
-// In case we want to delete it later, it may be useful to save the value
-// of mem_filename after FilePathForMemoryName().
-bool SharedMemory::CreateOrOpen(const std::string& name,
-                                int posix_flags, uint32 size) {
+bool SharedMemory::PrepareMapFile(FILE *fp) {
   DCHECK(mapped_file_ == -1);
+  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;
 
-  file_util::ScopedFILE file_closer;
-  FILE *fp;
-
-  FilePath path;
-  if (name.empty()) {
-    // It doesn't make sense to have a read-only private piece of shmem
-    DCHECK(posix_flags & (O_RDWR | O_WRONLY));
-
-    // 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);
-
-    // 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)
-      file_util::Delete(path, false);
-  } else {
-    if (!FilePathForMemoryName(name, &path))
-      return false;
-
-    std::string mode;
-    switch (posix_flags) {
-      case (O_RDWR | O_CREAT):
-        // Careful: "w+" will truncate if it already exists.
-        mode = "a+";
-        break;
-      case O_RDWR:
-        mode = "r+";
-        break;
-      case O_RDONLY:
-        mode = "r";
-        break;
-      default:
-        NOTIMPLEMENTED();
-        break;
-    }
-
-    fp = file_util::OpenFile(path, mode.c_str());
-  }
-
-  if (fp == NULL) {
-    if (posix_flags & O_CREAT) {
-#if !defined(OS_MACOSX)
-      PLOG(ERROR) << "Creating shared memory in " << path.value() << " failed";
-      FilePath dir = path.DirName();
-      if (access(dir.value().c_str(), W_OK | X_OK) < 0) {
-        PLOG(ERROR) << "Unable to access(W_OK|X_OK) " << dir.value();
-        if (dir.value() == "/dev/shm") {
-          LOG(FATAL) << "This is frequently caused by incorrect permissions on "
-                     << "/dev/shm.  Try 'sudo chmod 777 /dev/shm' to fix.";
-        }
-      }
-#else
-      PLOG(ERROR) << "Creating shared memory in " << path.value() << " failed";
-#endif
-    }
-    return false;
-  }
-
-  file_closer.reset(fp);  // close when we go out of scope
-
-  // Make sure the (new) file is the right size.
-  if (size && (posix_flags & (O_RDWR | O_CREAT))) {
-    // Get current size.
-    struct stat stat;
-    if (fstat(fileno(fp), &stat) != 0)
-      return false;
-    const uint32 current_size = stat.st_size;
-    if (current_size != size) {
-      if (ftruncate(fileno(fp), size) != 0)
-        return false;
-      if (fseeko(fp, size, SEEK_SET) != 0)
-        return false;
-    }
-  }
+  file_util::ScopedFILE file_closer(fp);
 
   mapped_file_ = dup(fileno(fp));
   if (mapped_file_ == -1) {
@@ -255,7 +251,7 @@ bool SharedMemory::Map(uint32 bytes) {
                  MAP_SHARED, mapped_file_, 0);
 
   if (memory_)
-    max_size_ = bytes;
+    mapped_size_ = bytes;
 
   bool mmap_succeeded = (memory_ != (void*)-1);
   DCHECK(mmap_succeeded) << "Call to mmap failed, errno=" << errno;
@@ -266,9 +262,9 @@ bool SharedMemory::Unmap() {
   if (memory_ == NULL)
     return false;
 
-  munmap(memory_, max_size_);
+  munmap(memory_, mapped_size_);
   memory_ = NULL;
-  max_size_ = 0;
+  mapped_size_ = 0;
   return true;
 }
 
diff --git a/base/shared_memory_unittest.cc b/base/shared_memory_unittest.cc
index 53bffb4..f7036e8 100644
--- a/base/shared_memory_unittest.cc
+++ b/base/shared_memory_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/shared_memory.h"
 #include "base/scoped_ptr.h"
 #include "base/test/multiprocess_test.h"
+#include "base/time.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/multiprocess_func_list.h"
 
@@ -36,7 +37,7 @@ class MultipleThreadMain : public PlatformThread::Delegate {
     mac::ScopedNSAutoreleasePool pool;  // noop if not OSX
     const uint32 kDataSize = 1024;
     SharedMemory memory;
-    bool rv = memory.Create(s_test_name_, false, true, kDataSize);
+    bool rv = memory.CreateNamed(s_test_name_, true, kDataSize);
     EXPECT_TRUE(rv);
     rv = memory.Map(kDataSize);
     EXPECT_TRUE(rv);
@@ -82,8 +83,8 @@ class MultipleLockThread : public PlatformThread::Delegate {
     SharedMemoryHandle handle = NULL;
     {
       SharedMemory memory1;
-      EXPECT_TRUE(memory1.Create("SharedMemoryMultipleLockThreadTest",
-                                 false, true, kDataSize));
+      EXPECT_TRUE(memory1.CreateNamed("SharedMemoryMultipleLockThreadTest",
+                                 true, kDataSize));
       EXPECT_TRUE(memory1.ShareToProcess(GetCurrentProcess(), &handle));
       // TODO(paulg): Implement this once we have a posix version of
       // SharedMemory::ShareToProcess.
@@ -128,7 +129,7 @@ TEST(SharedMemoryTest, OpenClose) {
   EXPECT_TRUE(rv);
   rv = memory1.Open(test_name, false);
   EXPECT_FALSE(rv);
-  rv = memory1.Create(test_name, false, false, kDataSize);
+  rv = memory1.CreateNamed(test_name, false, kDataSize);
   EXPECT_TRUE(rv);
   rv = memory1.Map(kDataSize);
   EXPECT_TRUE(rv);
@@ -163,6 +164,58 @@ TEST(SharedMemoryTest, OpenClose) {
   EXPECT_TRUE(rv);
 }
 
+TEST(SharedMemoryTest, OpenExclusive) {
+  const uint32 kDataSize = 1024;
+  const uint32 kDataSize2 = 2048;
+  std::ostringstream test_name_stream;
+  test_name_stream << "SharedMemoryOpenExclusiveTest."
+                   << Time::Now().ToDoubleT();
+  std::string test_name = test_name_stream.str();
+
+  // Open two handles to a memory segment and check that open_existing works
+  // as expected.
+  SharedMemory memory1;
+  bool rv = memory1.CreateNamed(test_name, false, kDataSize);
+  EXPECT_TRUE(rv);
+
+  // Memory1 knows it's size because it created it.
+  EXPECT_EQ(memory1.created_size(), kDataSize);
+
+  rv = memory1.Map(kDataSize);
+  EXPECT_TRUE(rv);
+
+  memset(memory1.memory(), 'G', kDataSize);
+
+  SharedMemory memory2;
+  // Should not be able to create if openExisting is false.
+  rv = memory2.CreateNamed(test_name, false, kDataSize2);
+  EXPECT_FALSE(rv);
+
+  // Should be able to create with openExisting true.
+  rv = memory2.CreateNamed(test_name, true, kDataSize2);
+  EXPECT_TRUE(rv);
+
+  // Memory2 shouldn't know the size because we didn't create it.
+  EXPECT_EQ(memory2.created_size(), 0U);
+
+  // We should be able to map the original size.
+  rv = memory2.Map(kDataSize);
+  EXPECT_TRUE(rv);
+
+  // Verify that opening memory2 didn't truncate or delete memory 1.
+  char *start_ptr = static_cast<char *>(memory2.memory());
+  char *end_ptr = start_ptr + kDataSize;
+  for (char* ptr = start_ptr; ptr < end_ptr; ptr++) {
+    EXPECT_EQ(*ptr, 'G');
+  }
+
+  memory1.Close();
+  memory2.Close();
+
+  rv = memory1.Delete(test_name);
+  EXPECT_TRUE(rv);
+}
+
 // Create a set of N threads to each open a shared memory segment and write to
 // it. Verify that they are always reading/writing consistent data.
 TEST(SharedMemoryTest, MultipleThreads) {
@@ -243,9 +296,7 @@ TEST(SharedMemoryTest, AnonymousPrivate) {
   ASSERT_TRUE(pointers.get());
 
   for (i = 0; i < count; i++) {
-    rv = memories[i].Create("", false, true, kDataSize);
-    EXPECT_TRUE(rv);
-    rv = memories[i].Map(kDataSize);
+    rv = memories[i].CreateAndMapAnonymous(kDataSize);
     EXPECT_TRUE(rv);
     int *ptr = static_cast<int*>(memories[i].memory());
     EXPECT_TRUE(ptr);
@@ -289,7 +340,7 @@ class SharedMemoryProcessTest : public base::MultiProcessTest {
     mac::ScopedNSAutoreleasePool pool;  // noop if not OSX
     const uint32 kDataSize = 1024;
     SharedMemory memory;
-    bool rv = memory.Create(s_test_name_, false, true, kDataSize);
+    bool rv = memory.CreateNamed(s_test_name_, true, kDataSize);
     EXPECT_TRUE(rv);
     if (rv != true)
       errors++;
diff --git a/base/shared_memory_win.cc b/base/shared_memory_win.cc
index f35ada1..a0b2a5a 100644
--- a/base/shared_memory_win.cc
+++ b/base/shared_memory_win.cc
@@ -13,7 +13,7 @@ SharedMemory::SharedMemory()
     : mapped_file_(NULL),
       memory_(NULL),
       read_only_(false),
-      max_size_(0),
+      created_size_(0),
       lock_(NULL) {
 }
 
@@ -21,7 +21,7 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only)
     : mapped_file_(handle),
       memory_(NULL),
       read_only_(read_only),
-      max_size_(0),
+      created_size_(0),
       lock_(NULL) {
 }
 
@@ -30,7 +30,7 @@ SharedMemory::SharedMemory(SharedMemoryHandle handle, bool read_only,
     : mapped_file_(NULL),
       memory_(NULL),
       read_only_(read_only),
-      max_size_(0),
+      created_size_(0),
       lock_(NULL) {
   ::DuplicateHandle(process, handle,
                     GetCurrentProcess(), &mapped_file_,
@@ -61,9 +61,19 @@ void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
   ::CloseHandle(handle);
 }
 
-bool SharedMemory::Create(const std::string& name, bool read_only,
-                          bool open_existing, uint32 size) {
+bool SharedMemory::CreateAndMapAnonymous(uint32 size) {
+  return CreateAnonymous(size) && Map(size);
+}
+
+bool SharedMemory::CreateAnonymous(uint32 size) {
+  return CreateNamed("", false, size);
+}
+
+bool SharedMemory::CreateNamed(const std::string& name,
+                               bool open_existing, uint32 size) {
   DCHECK(mapped_file_ == NULL);
+  if (size == 0)
+    return false;
 
   // NaCl's memory allocator requires 0mod64K alignment and size for
   // shared memory objects.  To allow passing shared memory to NaCl,
@@ -72,20 +82,25 @@ bool SharedMemory::Create(const std::string& name, bool read_only,
   // actual requested size.
   uint32 rounded_size = (size + 0xffff) & ~0xffff;
   name_ = ASCIIToWide(name);
-  read_only_ = read_only;
   mapped_file_ = CreateFileMapping(INVALID_HANDLE_VALUE, NULL,
-      read_only_ ? PAGE_READONLY : PAGE_READWRITE, 0,
-      static_cast<DWORD>(rounded_size),
+      PAGE_READWRITE, 0, static_cast<DWORD>(rounded_size),
       name_.empty() ? NULL : name_.c_str());
   if (!mapped_file_)
     return false;
 
+  created_size_ = size;
+
   // Check if the shared memory pre-exists.
-  if (GetLastError() == ERROR_ALREADY_EXISTS && !open_existing) {
-    Close();
-    return false;
+  if (GetLastError() == ERROR_ALREADY_EXISTS) {
+    // If the file already existed, set created_size_ to 0 to show that
+    // we don't know the size.
+    created_size_ = 0;
+    if (!open_existing) {
+      Close();
+      return false;
+    }
   }
-  max_size_ = size;
+
   return true;
 }