Create "persistent memory allocator" for persisting and sharing objects.

base/memory/shared_memory_allocator.h

+// 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.
+
+#ifndef BASE_MEMORY_SHARED_MEMORY_ALLOCATOR_H_
+#define BASE_MEMORY_SHARED_MEMORY_ALLOCATOR_H_
+
+#include <stdint.h>
+
+#include "base/atomicops.h"
+#include "base/base_export.h"
+#include "base/macros.h"
+
+namespace base {
+
+// Simple allocator for pieces of a memory block that may be shared across
+// multiple processes.
+//
+// This class provides for thread-secure (i.e. safe against other threads
+// or processes that may be compromised and thus have malicious intent)
+// allocation of memory within a designated block and also a mechanism by
+// which other threads can learn of the allocations with any additional
+// shared information.
+//
+// There is (currently) no way to release an allocated block of data because
+// doing so would risk invalidating pointers held by other processes and
+// greatly complicate the allocation algorithm.
+//
+// Construction of this object can accept new, clean (i.e. zeroed) memory
+// or previously initialized memory. In the first case, construction must
+// be allowed to complete before letting other allocators attach to the same
+// segment. In other words, don't share the segment until at least one
+// allocator has been attached to it.
+//
+// It should be noted that memory doesn't need to actually have zeros written
+// throughout; it just needs to read as zero until something diffferent is
+// written to a location. This is an important distinction as it supports the
+// use-case of non-pinned memory, such as from a demand-allocated region by
+// the OS or a memory-mapped file that auto-grows from a starting size of zero.
+class BASE_EXPORT SharedMemoryAllocator {
+ public:
+  struct Iterator {
+    int32_t last;
+    uint32_t niter;
+  };
+
+  struct MemoryInfo {
+    int32_t total;
+    int32_t free;
+  };

[chrisha, 2015/11/03 18:20:51]

Each of these structs is worth of a one line comment describing what they
are/will be used for.

[bcwhite, 2015/11/03 22:32:17]

Done.

+
+  // The allocator operates on any arbitrary block of memory. Creation and
+  // sharing of that block with another process is the responsibility of the
+  // caller. The allocator needs to know only the block's |base| address, the
+  // total |size| of the block, and any internal |page| size (zero if not
+  // paged) across which allocations should not span.
+  //
+  // SharedMemoryAllocator does NOT take ownership of this memory block. The
+  // caller must manage it and ensure it stays available throughout the lifetime
+  // of this object.
+  SharedMemoryAllocator(void* base, int32_t size, int32_t page_size);

[chrisha, 2015/11/03 18:20:51]

The comments for this still aren't clear.

The description earlier up states that construction must complete before it is
shared to others, but there's no comment of this fact here. This constructor is
also able to be both the 'initializing' constructor and an 'attaching'
constructor. Maybe document that fact very clearly?

(I still prefer separate constructors, or an additional parameter that declares
whether the constructor is expected to initialize the memory or attach to an
already initialized allocator.)

[bcwhite, 2015/11/03 22:32:17]

Comment updated and extended.

Regarding multiple ctors...  They would have the same nominal signature making
it difficult to distinguish them when viewing the code.  Also, it may be that
the caller doesn't know whether the segment is new or existing; it seems best to
leave the test for that in code that knows what it's doing.  Fewer things to go
wrong.

+  ~SharedMemoryAllocator();
+
+  // Get an object referenced by an |offset|. For safety reasons, the |type|
+  // code and size-of(|T|) are compared to ensure the reference is valid
+  // and cannot return an object outside of the memory segment. A |type| of
+  // zero will match any though the size is still checked. NULL is returned
+  // if any problem is detected, such as corrupted storage or incorrect
+  // parameters. Callers MUST check that the returned value is not-null EVERY
+  // TIME before accessing it or risk crashing!  Once dereferenced, the pointer
+  // is safe to reuse forever.
+  //
+  // NOTE: Though this method will guarantee that an object of the specified
+  // type can be accessed without going outside the bounds of the memory
+  // segment, it makes not guarantees of the validity of the data within the
+  // object itself. If it is expected that the contents of the segment could
+  // be compromised with malicious intent, the object must be hardened as well.
+  template <typename T>
+  T* GetType(int32_t offset, int32_t type) {

[chrisha, 2015/11/03 18:20:51]

This is poorly named, as it's not actually getting a type. It's getting a
pointer to an object, and validating its type.

GetAsType?
GetAtOffset?

Also, 'type' is overloading here and throughout this class. A type means
something very specific in C++, and that is not what |type| is. Maybe you want
|type_id|? Or maybe you want to make this more generic and call this a |cookie|?

And given that there's a special value (0), I'd like a static constant (well,
typed enum is the new hotness) for that magic value: kAnyCookie, or kAnyTypeId?

This function also wants a counterpart to convert a typed pointer to an offset,
that also checks validity, size, etc.

[bcwhite, 2015/11/03 22:32:17]

It was originally "GetObject" but it turns out that this name gets mangled by
some VS header files into "GetObjectW".  <sigh>

How about GetAsObject<T>?
Done.
Done.

+    return static_cast<T*>(GetBlockData(offset, type, sizeof(T), false));
+  }
+
+  // Reserve space in the memory segment of the desired |size| and |type|.
+  // A return value of zero indicates the allocation failed, otherwise the
+  // returned offset can be used by any process to get a real pointer via
+  // the GetType() call.

[chrisha, 2015/11/03 18:20:51]

Comment that the user is expected to Commit/MakeIterable the returned allocation
once it is initialized/readable.

Comment that the user should check the flags for failure reason?

[bcwhite, 2015/11/03 22:32:17]

Done.

+  int32_t Allocate(int32_t size, int32_t type);
+
+  // Get the information about the amount of free space in the allocator. The
+  // amount of free space should be treated as approximate due to extras from
+  // alignment and metadata, but will never return less than could actually
+  // be allocated.

[chrisha, 2015/11/03 18:20:51]

Also note that this is inherently 'racy' in the sense that the information is
only valid instantaneously, and may no longer be true due to activity on other
threads. Could also mention any guarantees this gives under single threaded use.

[bcwhite, 2015/11/03 22:32:17]

Done.

+  void GetMemoryInfo(MemoryInfo* meminfo);
+
+  // Allocated objects can be added to an internal list that can then be
+  // iterated over by other processes.
+  void MakeIterable(int32_t offset);

[chrisha, 2015/11/03 18:20:51]

Do we want to use the seemingly standard 'Allocate' and 'Commit' language?

I'd also move this declaration directly below Allocate, as they are intimately
related.

[bcwhite, 2015/11/03 22:32:17]

I moved it.  It's not necessary to commit all allocations, just those you want
iterable.

+
+  // Iterating uses a |state| structure (initialized by CreateIterator) and
+  // returns both the offset reference to the object as well as the |type| of
+  // that object. A zero return value indicates there are currently no more
+  // objects to be found but future attempts can be made without having to
+  // reset the iterator to "first".
+  void CreateIterator(Iterator* state);
+  int32_t GetNextIterable(Iterator* state, int32_t* type);

[chrisha, 2015/11/03 18:20:51]

Is it worth making this have STL iterator semantics so it can be used in STL
algorithms and for-range constructs? Not a lot more complexity for much more
power.

[bcwhite, 2015/11/03 22:32:17]

I'm not sure.  STL iterators have a designated "end()" beyond which they cannot
iterate.  We insert iterable objects just before "end" so we couldn't be able to
continue iteration and get those new objects.

+
+  // If there is some indication that the shared memory has become corrupted,
+  // calling this will attempt to prevent further damage by indicating to
+  // all processes that something is not as expected.
+  void SetCorrupted();
+  bool IsCorrupted();

[chrisha, 2015/11/03 18:20:51]

Does SetCorrupted need to be exposed for anyone to call? Or does it really only
need to private, and called internally?

[bcwhite, 2015/11/03 22:32:17]

I was thinking that the caller might want to report it if it detects corruption
within data objects.  I'm flexible either way.

+
+  // Flag set if an allocation has failed because memory was full.
+  bool IsFull();
+
+ private:
+  struct SharedMetadata;
+  struct BlockHeader;
+
+  BlockHeader* GetBlock(int32_t offset, int32_t type, int32_t size,
+                        bool special);
+  void* GetBlockData(int32_t offset, int32_t type, int32_t size, bool special);
+
+  SharedMetadata* shared_meta_;
+  char* mem_base_;  // char because sizeof guaranteed 1 -- easy offset calc
+  int32_t mem_size_;
+  int32_t mem_page_;

[chrisha, 2015/11/03 18:20:51]

Should these be stored in this object, or in the header itself? As is it's
possible for different threads/processes to use different values for these...

[bcwhite, 2015/11/03 22:32:17]

They need to be stored in both places.  We want to compare them during
attachment to ensure everything is still in good shape but we can't trust the
shared values when verifying boundary conditions.

I'm assuming that the mechanism passing the shared segments around provides
reliable size information provided by the OS and the page size will likely be a
compile-time constant.

+  int32_t last_seen_;

[chrisha, 2015/11/03 18:20:51]

Comment for what this is? While you're at it, comment all of these member
variables and functions.

[bcwhite, 2015/11/03 22:32:17]

Done.

+  subtle::Atomic32 corrupted_;  // TODO(bcwhite): Use std::atomic<char> when ok.
+
+  DISALLOW_COPY_AND_ASSIGN(SharedMemoryAllocator);
+};
+
+}  // namespace base
+
+#endif  // BASE_MEMORY_SHARED_MEMORY_ALLOCATOR_H_