Harden allocator for file-backed memory.

base/metrics/persistent_memory_allocator.cc

 // Copyright (c) 2015 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/metrics/persistent_memory_allocator.h"
 
 #include <assert.h>
 #include <algorithm>
 
 #if defined(OS_WIN)
 #include "winbase.h"
 #elif defined(OS_POSIX)
 #include <sys/mman.h>
 #endif
 
 #include "base/files/memory_mapped_file.h"
 #include "base/logging.h"
 #include "base/memory/shared_memory.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/threading/thread_restrictions.h"
 
 namespace {
 
 // Limit of memory segment size. It has to fit in an unsigned 32-bit number
 // and should be a power of 2 in order to accomodate almost any page size.
 const uint32_t kSegmentMaxSize = 1 << 30;  // 1 GiB
 
 // A constant (random) value placed in the shared metadata to identify
 // an already initialized memory segment.
 const uint32_t kGlobalCookie = 0x408305DC;
 
 // The current version of the metadata. If updates are made that change
 // the metadata, the version number can be queried to operate in a backward-
 // compatible manner until the memory segment is completely re-initalized.
-const uint32_t kGlobalVersion = 1;
+const uint32_t kGlobalVersion = 2;
 
 // Constant values placed in the block headers to indicate its state.
 const uint32_t kBlockCookieFree = 0;
 const uint32_t kBlockCookieQueue = 1;
 const uint32_t kBlockCookieWasted = (uint32_t)-1;
 const uint32_t kBlockCookieAllocated = 0xC8799269;
 
 // TODO(bcwhite): When acceptable, consider moving flags to std::atomic<char>
 // types rather than combined bitfield.
 
-// Flags stored in the flags_ field of the SharedMetaData structure below.
+// Flags stored in the flags_ field of the SharedMetadata structure below.
 enum : int {
   kFlagCorrupt = 1 << 0,
   kFlagFull    = 1 << 1
 };
 
 // Errors that are logged in "errors" histogram.
 enum AllocatorError : int {
   kMemoryIsCorrupt = 1,
 };
 
@@ skipping 36 matching lines @@
 // The block-header is placed at the top of every allocation within the
 // segment to describe the data that follows it.
 struct PersistentMemoryAllocator::BlockHeader {
   uint32_t size;       // Number of bytes in this block, including header.
   uint32_t cookie;     // Constant value indicating completed allocation.
   std::atomic<uint32_t> type_id;  // Arbitrary number indicating data type.
   std::atomic<uint32_t> next;     // Pointer to the next block when iterating.
 };
 
 // The shared metadata exists once at the top of the memory segment to
-// describe the state of the allocator to all processes.
+// describe the state of the allocator to all processes. The size of this
+// structure must be a multiple of 64-bits to ensure compatibility between
+// architectures.

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

Can you have a static_assert about that?

I know you have one checking for 64 explicitly, but this is more specific - i.e.
that other assert doesn't indicate that it shouldn't be updated. So maybe have a
static_assert specifically for the % 64? Also, can it be right after the
structure rather than elsewhere in some function?

[bcwhite, 2017/03/15 19:21:48]

It's effectively on line 324 which ensures the same size on all architectures.

 struct PersistentMemoryAllocator::SharedMetadata {
   uint32_t cookie;     // Some value that indicates complete initialization.
   uint32_t size;       // Total size of memory segment.
   uint32_t page_size;  // Paging size within memory segment.
   uint32_t version;    // Version code so upgrades don't break.
   uint64_t id;         // Arbitrary ID number given by creator.
   uint32_t name;       // Reference to stored name string.
+  uint32_t padding1;   // Pad-out read-only data to 64-bit alignment.
 
   // Above is read-only after first construction. Below may be changed and
   // so must be marked "volatile" to provide correct inter-process behavior.
 
+  // State of the memory, plus some padding to keep alignment.

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

What does "state of the memory" mean? Is it referencing the enum you have
defined elsewhere? If so, mention this.

Also why char and not uint8_t?

[bcwhite, 2017/03/15 19:21:47]

Yes, MemoryState in the .h file.
Done.
"char" is the only standard type with a guaranteed sizeof 1 on all
architectures.

[Alexei Svitkine (slow), 2017/03/15 20:05:29]

I think on all platforms Chrome supports uint8_t should also be sizeof 1. I
think it's clearer to use that and be consistent in types between here and other
places - and there are sizeof checks to ensure there's no problems with it.

[bcwhite, 2017/03/16 15:53:03]

Done.

+  volatile std::atomic<char> memory_state;
+  char padding2[3];
+
   // Bitfield of information flags. Access to this should be done through
   // the CheckFlag() and SetFlag() methods defined above.
   volatile std::atomic<uint32_t> flags;
 
   // Offset/reference to first free space in segment.
   volatile std::atomic<uint32_t> freeptr;
 
   // The "iterable" queue is an M&S Queue as described here, append-only:
   // https://www.research.ibm.com/people/m/michael/podc-1996.pdf
+  // |queue| needs to be 64-bit aligned and is itself a multiple of 64 bits.
   volatile std::atomic<uint32_t> tailptr;  // Last block of iteration queue.
   volatile BlockHeader queue;   // Empty block for linked-list head/tail.
 };
 
 // The "queue" block header is used to detect "last node" so that zero/null
 // can be used to indicate that it hasn't been added at all. It is part of
 // the SharedMetadata structure which itself is always located at offset zero.
 const PersistentMemoryAllocator::Reference
     PersistentMemoryAllocator::kReferenceQueue =
         offsetof(SharedMetadata, queue);
@@ skipping 171 matching lines @@
       readonly_(readonly),
       corrupt_(0),
       allocs_histogram_(nullptr),
       used_histogram_(nullptr),
       errors_histogram_(nullptr) {
   // These asserts ensure that the structures are 32/64-bit agnostic and meet
   // all the requirements of use within the allocator. They access private
   // definitions and so cannot be moved to the global scope.
   static_assert(sizeof(PersistentMemoryAllocator::BlockHeader) == 16,
                 "struct is not portable across different natural word widths");
-  static_assert(sizeof(PersistentMemoryAllocator::SharedMetadata) == 56,
+  static_assert(sizeof(PersistentMemoryAllocator::SharedMetadata) == 64,
                 "struct is not portable across different natural word widths");
 
   static_assert(sizeof(BlockHeader) % kAllocAlignment == 0,
                 "BlockHeader is not a multiple of kAllocAlignment");
   static_assert(sizeof(SharedMetadata) % kAllocAlignment == 0,
                 "SharedMetadata is not a multiple of kAllocAlignment");
   static_assert(kReferenceQueue % kAllocAlignment == 0,
                 "\"queue\" is not aligned properly; must be at end of struct");
 
   // Ensure that memory segment is of acceptable size.
@@ skipping 51 matching lines @@
     shared_meta()->tailptr.store(kReferenceQueue, std::memory_order_release);
 
     // Allocate space for the name so other processes can learn it.
     if (!name.empty()) {
       const size_t name_length = name.length() + 1;
       shared_meta()->name = Allocate(name_length, 0);
       char* name_cstr = GetAsArray<char>(shared_meta()->name, 0, name_length);
       if (name_cstr)
         memcpy(name_cstr, name.data(), name.length());
     }
+
+    shared_meta()->memory_state.store(MEMORY_INITIALIZED,
+                                      std::memory_order_release);
   } else {
-    if (shared_meta()->size == 0 ||
-        shared_meta()->version == 0 ||
+    if (shared_meta()->size == 0 || shared_meta()->version != kGlobalVersion ||

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

So we would previously not use kGlobalVersion field?

[bcwhite, 2017/03/15 19:21:48]

There has only been one version before this so not really a problem.

         shared_meta()->freeptr.load(std::memory_order_relaxed) == 0 ||
-        shared_meta()->tailptr == 0 ||
-        shared_meta()->queue.cookie == 0 ||
+        shared_meta()->tailptr == 0 || shared_meta()->queue.cookie == 0 ||
         shared_meta()->queue.next.load(std::memory_order_relaxed) == 0) {
       SetCorrupt();
     }
     if (!readonly) {
       // The allocator is attaching to a previously initialized segment of
       // memory. If the initialization parameters differ, make the best of it
       // by reducing the local construction parameters to match those of
       // the actual memory area. This ensures that the local object never
       // tries to write outside of the original bounds.
       // Because the fields are const to ensure that no code other than the
@@ skipping 60 matching lines @@
   used_histogram_ = LinearHistogram::FactoryGet(
       "UMA.PersistentAllocator." + name_string + ".UsedPct", 1, 101, 21,
       HistogramBase::kUmaTargetedHistogramFlag);
 
   DCHECK(!errors_histogram_);
   errors_histogram_ = SparseHistogram::FactoryGet(
       "UMA.PersistentAllocator." + name_string + ".Errors",
       HistogramBase::kUmaTargetedHistogramFlag);
 }
 
+void PersistentMemoryAllocator::SetMemoryState(uint8_t memory_state) {
+  shared_meta()->memory_state.store(memory_state, std::memory_order_relaxed);
+  Flush(sizeof(SharedMetadata), false);
+}
+
+uint8_t PersistentMemoryAllocator::GetMemoryState() {
+  return shared_meta()->memory_state.load(std::memory_order_relaxed);
+}
+
 size_t PersistentMemoryAllocator::used() const {
   return std::min(shared_meta()->freeptr.load(std::memory_order_relaxed),
                   mem_size_);
 }
 
 PersistentMemoryAllocator::Reference PersistentMemoryAllocator::GetAsReference(
     const void* memory,
     uint32_t type_id) const {
   uintptr_t address = reinterpret_cast<uintptr_t>(memory);
   if (address < reinterpret_cast<uintptr_t>(mem_base_))
@@ skipping 328 matching lines @@
   if (ref < (queue_ok ? kReferenceQueue : sizeof(SharedMetadata)))
     return nullptr;
   if (ref % kAllocAlignment != 0)
     return nullptr;
   size += sizeof(BlockHeader);
   if (ref + size > mem_size_)
     return nullptr;
 
   // Validation of referenced block-header.
   if (!free_ok) {
-    uint32_t freeptr = std::min(
-        shared_meta()->freeptr.load(std::memory_order_relaxed), mem_size_);
-    if (ref + size > freeptr)
-      return nullptr;
     const volatile BlockHeader* const block =
         reinterpret_cast<volatile BlockHeader*>(mem_base_ + ref);
+    if (ref != kReferenceQueue && block->cookie != kBlockCookieAllocated)

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

Shouldn't you check ref != kReferenceQueue before adding ref to mem_base_ above?
I know you're not derefing the pointer yet, but still - maybe separate the if
into two.

[bcwhite, 2017/03/15 19:21:48]

kReferenceQueue is still an offset of mem_base_ so it's necessary to do the add
before any dereference below.  But I guess it's simpler to check for kReference
Queue once at the top that have special conditions within.

Done.

+      return nullptr;
     if (block->size < size)
       return nullptr;
-    if (ref + block->size > freeptr)
-      return nullptr;
-    if (ref != kReferenceQueue && block->cookie != kBlockCookieAllocated)
+    if (ref + block->size > mem_size_)
       return nullptr;
     if (type_id != 0 &&
         block->type_id.load(std::memory_order_relaxed) != type_id) {
       return nullptr;
     }
   }
 
   // Return pointer to block data.
   return reinterpret_cast<const volatile BlockHeader*>(mem_base_ + ref);
 }
 
+void PersistentMemoryAllocator::Flush(size_t length, bool sync) {}

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

Nit: Maybe add a comment for why it makes sense to be a no-op.

[bcwhite, 2017/03/15 19:21:48]

Done.

+
 void PersistentMemoryAllocator::RecordError(int error) const {
   if (errors_histogram_)
     errors_histogram_->Add(error);
 }
 
 const volatile void* PersistentMemoryAllocator::GetBlockData(
     Reference ref,
     uint32_t type_id,
     uint32_t size) const {
   DCHECK(size > 0);
@@ skipping 120 matching lines @@
     uint64_t id,
     base::StringPiece name,
     bool read_only)
     : PersistentMemoryAllocator(
           Memory(const_cast<uint8_t*>(file->data()), MEM_FILE),
           max_size != 0 ? max_size : file->length(),
           0,
           id,
           name,
           read_only),
-      mapped_file_(std::move(file)) {}
+      mapped_file_(std::move(file)) {
+  // Ensure the disk-copy of the data reflects the fully-initialized memory as
+  // there is no guarantee as to what order the pages might be auto-flushed by
+  // the OS in the future.
+  Flush(used(), true);
+}
 
 FilePersistentMemoryAllocator::~FilePersistentMemoryAllocator() {}
 
 // static
 bool FilePersistentMemoryAllocator::IsFileAcceptable(
     const MemoryMappedFile& file,
     bool read_only) {
   return IsMemoryAcceptable(file.data(), file.length(), 0, read_only);
 }
+
+void FilePersistentMemoryAllocator::Flush(size_t length, bool sync) {
+  if (sync)
+    ThreadRestrictions::AssertIOAllowed();
+
+#if defined(OS_WIN)
+  // Windows doesn't support a synchronous flush.
+  ::FlushViewOfFile(data(), length);
+#elif defined(OS_POSIX)
+  ::msync(const_cast<void*>(data()), length,
+          MS_INVALIDATE | (sync ? MS_SYNC : MS_ASYNC));

[Alexei Svitkine (slow), 2017/03/15 15:47:20]

From man msync:

"The MS_ASYNC flag is not permitted to be combined with other flags."

[bcwhite, 2017/03/15 19:21:48]

Where did you read that?

http://man7.org/linux/man-pages/man2/msync.2.html
  The flags argument should specify exactly one of MS_ASYNC and
  MS_SYNC, and may additionally include the MS_INVALIDATE bit. 

[Alexei Svitkine (slow), 2017/03/15 20:05:29]

I typed "man msync" on my Mac. So sounds like it's platform specific.

[bcwhite, 2017/03/16 15:53:03]

Done.

+#else
+#error Unsupported OS.
+#endif
+}
 #endif  // !defined(OS_NACL)
 
 }  // namespace base