Bug 1662564 - Use RAII for the memory mapping in IPC base::SharedMemory, and default its constructors. r=nika

Differential Revision: https://phabricator.services.mozilla.com/D90602

ipc/chromium/src/base/shared_memory.h

@@ -36,7 +36,7 @@ typedef FileDescriptor SharedMemoryHandle;
 class SharedMemory {
  public:
   // Create a new SharedMemory object.
-  SharedMemory();
+  SharedMemory() = default;
 
   // Create a new SharedMemory object from an existing, open
   // shared memory file.
@@ -46,7 +46,7 @@ class SharedMemory {
   }
 
   // Move constructor; transfers ownership.
-  SharedMemory(SharedMemory&& other);
+  SharedMemory(SharedMemory&& other) = default;
 
   // Destructor.  Will close any open files.
   ~SharedMemory();
@@ -85,9 +85,7 @@ class SharedMemory {
   bool Map(size_t bytes, void* fixed_address = nullptr);
 
   // Unmaps the shared memory from the caller's address space.
-  // Returns true if successful; returns false on error or if the
-  // memory is not mapped.
-  bool Unmap();
+  void Unmap() { memory_ = nullptr; }
 
   // Get the size of the opened shared memory backing file.
   // Note:  This size is only available to the creator of the
@@ -98,7 +96,7 @@ class SharedMemory {
 
   // Gets a pointer to the opened memory space if it has been
   // Mapped via Map().  Returns NULL if it is not mapped.
-  void* memory() const { return memory_; }
+  void* memory() const { return memory_.get(); }
 
   // Extracts the underlying file handle; similar to
   // GiveToProcess(GetCurrentProcId(), ...) but returns a RAII type.
@@ -196,19 +194,33 @@ class SharedMemory {
 
   bool CreateInternal(size_t size, bool freezeable);
 
-  void* memory_;
-  size_t max_size_;
+  // Unmapping shared memory requires the mapped size on Unix but not
+  // Windows; this encapsulates that difference.
+  struct MappingDeleter {
+#ifdef OS_POSIX
+    // A default-constructed deleter must be used only with nullptr
+    // (to allow default-constructing UniqueMapping).  A deleter with
+    // a size must be used at most once.
+    size_t mapped_size_ = 0;
+    explicit MappingDeleter(size_t size) : mapped_size_(size) {}
+#endif
+    MappingDeleter() = default;
+    void operator()(void* ptr);
+  };
+  using UniqueMapping = mozilla::UniquePtr<void, MappingDeleter>;
+
+  UniqueMapping memory_;
+  size_t max_size_ = 0;
   mozilla::UniqueFileHandle mapped_file_;
 #if defined(OS_WIN)
   // If true indicates this came from an external source so needs extra checks
   // before being mapped.
-  bool external_section_;
+  bool external_section_ = false;
 #elif defined(OS_POSIX)
   mozilla::UniqueFileHandle frozen_file_;
-  size_t mapped_size_;
 #endif
-  bool read_only_;
-  bool freezeable_;
+  bool read_only_ = false;
+  bool freezeable_ = false;
 
   DISALLOW_EVIL_CONSTRUCTORS(SharedMemory);
 };

ipc/chromium/src/base/shared_memory_posix.cc

@@ -26,29 +26,23 @@
 
 namespace base {
 
-SharedMemory::SharedMemory()
-    : memory_(nullptr),
-      max_size_(0),
-      mapped_file_(nullptr),
-      frozen_file_(nullptr),
-      mapped_size_(0),
-      read_only_(false),
-      freezeable_(false) {}
-
-SharedMemory::SharedMemory(SharedMemory&& other) {
-  memory_ = other.memory_;
-  max_size_ = other.max_size_;
-  mapped_file_ = std::move(other.mapped_file_);
-  frozen_file_ = std::move(other.frozen_file_);
-  mapped_size_ = other.mapped_size_;
-  read_only_ = other.read_only_;
-  freezeable_ = other.freezeable_;
-
-  other.mapped_size_ = 0;
-  other.memory_ = nullptr;
+void SharedMemory::MappingDeleter::operator()(void* ptr) {
+  // Check that this isn't a default-constructed deleter.  (`munmap`
+  // is specified to fail with `EINVAL` if the length is 0, so this
+  // assertion isn't load-bearing.)
+  DCHECK(mapped_size_ != 0);
+  munmap(ptr, mapped_size_);
+  // Guard against multiple calls of the same deleter, which shouldn't
+  // happen (but could, if `UniquePtr::reset` were used).  Calling
+  // `munmap` with an incorrect non-zero length would be bad.
+  mapped_size_ = 0;
 }
 
-SharedMemory::~SharedMemory() { Close(); }
+SharedMemory::~SharedMemory() {
+  // This is almost equal to the default destructor, except for the
+  // warning message about unfrozen freezable memory.
+  Close();
+}
 
 bool SharedMemory::SetHandle(SharedMemoryHandle handle, bool read_only) {
   DCHECK(!mapped_file_);
@@ -267,17 +261,7 @@ bool SharedMemory::Map(size_t bytes, void* fixed_address) {
     return false;
   }
 
-  memory_ = mem;
-  mapped_size_ = bytes;
-  return true;
-}
-
-bool SharedMemory::Unmap() {
-  if (memory_ == NULL) return false;
-
-  munmap(memory_, mapped_size_);
-  memory_ = NULL;
-  mapped_size_ = 0;
+  memory_ = UniqueMapping(mem, MappingDeleter(bytes));
   return true;
 }
 

ipc/chromium/src/base/shared_memory_win.cc

@@ -58,29 +58,11 @@ bool IsSectionSafeToMap(HANDLE handle) {
 
 namespace base {
 
-SharedMemory::SharedMemory()
-    : memory_(nullptr),
-      max_size_(0),
-      mapped_file_(nullptr),
-      external_section_(false),
-      read_only_(false),
-      freezeable_(false) {}
-
-SharedMemory::SharedMemory(SharedMemory&& other) {
-  memory_ = other.memory_;
-  max_size_ = other.max_size_;
-  mapped_file_ = std::move(other.mapped_file_);
-  external_section_ = other.external_section_;
-  read_only_ = other.read_only_;
-  freezeable_ = other.freezeable_;
-
-  other.memory_ = nullptr;
+void SharedMemory::MappingDeleter::operator()(void* ptr) {
+  UnmapViewOfFile(ptr);
 }
 
-SharedMemory::~SharedMemory() {
-  external_section_ = true;
-  Close();
-}
+SharedMemory::~SharedMemory() = default;
 
 bool SharedMemory::SetHandle(SharedMemoryHandle handle, bool read_only) {
   DCHECK(!mapped_file_);
@@ -188,26 +170,19 @@ bool SharedMemory::Map(size_t bytes, void* fixed_address) {
     return false;
   }
 
-  memory_ = MapViewOfFileEx(
+  void* mem = MapViewOfFileEx(
       mapped_file_.get(),
       read_only_ ? FILE_MAP_READ : FILE_MAP_READ | FILE_MAP_WRITE, 0, 0, bytes,
       fixed_address);
-  if (memory_ != NULL) {
-    MOZ_ASSERT(!fixed_address || memory_ == fixed_address,
+  if (mem) {
+    MOZ_ASSERT(!fixed_address || mem == fixed_address,
                "MapViewOfFileEx returned an expected address");
+    memory_.reset(mem);
     return true;
   }
   return false;
 }
 
-bool SharedMemory::Unmap() {
-  if (memory_ == NULL) return false;
-
-  UnmapViewOfFile(memory_);
-  memory_ = NULL;
-  return true;
-}
-
 void* SharedMemory::FindFreeAddressSpace(size_t size) {
   void* memory = VirtualAlloc(NULL, size, MEM_RESERVE, PAGE_NOACCESS);
   if (memory) {