Heap-allocate MinidumpContextAMD64Writer objects with proper alignment

While making crashpad_minidump_test run in Chromium’s try- and buildbots
(https://crbug.com/779790), crashes in the
MinidumpThreadWriter.OneThread_AMD64_Stack test were observed in 32-bit
x86 Windows builds produced by Clang in the release configuration. These
crashes occurred in crashpad::test::InitializeMinidumpContextAMD64,
which heap-allocates a MinidumpContextAMD64Writer object. These objects
have an alignment requirement of 16, based on the alignment requirement
of their MinidumpContextAMD64 member.

Although this problem was never observed with MSVC, Clang was making use
of the known strict alignment and producing code that depended on it.
This code crashed if the requirement was not met. MSVC had raised a
warning about this usage (C4316), but the warning was disabled as it did
not appear to have any ill effect on code produced by that compiler.

The problem surfaced in test code, but heap-allocated
MinidumpContextAMD64Writer objects are created in non-test code as well.
The impact is limited, because a 32-bit Windows Crashpad handler would
not have a need to allocate one of these objects.

As a fix, MinidumpContextAMD64Writer is given a custom allocation
function (a static “operator new()” member and matching “operator
delete()”) that returns properly aligned memory.

Change-Id: I0cb924da91716eb01b88ec2ae952a69262cc2de6
Reviewed-on: https://chromium-review.googlesource.com/746539
Reviewed-by: Leonard Mosescu <mosescu@chromium.org>

minidump/minidump_context_writer.cc

@@ -23,6 +23,7 @@
 #include "base/logging.h"
 #include "snapshot/cpu_context.h"
 #include "util/file/file_writer.h"
+#include "util/stdlib/aligned_allocator.h"
 
 namespace crashpad {
 
@@ -65,10 +66,8 @@ MinidumpContextWriter::CreateFromSnapshot(const CPUContext* context_snapshot) {
     }
 
     case kCPUArchitectureX86_64: {
-      MSVC_PUSH_DISABLE_WARNING(4316);  // Object on heap may not be aligned.
       MinidumpContextAMD64Writer* context_amd64 =
           new MinidumpContextAMD64Writer();
-      MSVC_POP_WARNING();  // C4316
       context.reset(context_amd64);
       context_amd64->InitializeFromSnapshot(context_snapshot->x86_64);
       break;
@@ -152,6 +151,12 @@ size_t MinidumpContextX86Writer::ContextSize() const {
   return sizeof(context_);
 }
 
+static_assert(alignof(MinidumpContextAMD64) >= 16,
+              "MinidumpContextAMD64 alignment");
+static_assert(alignof(MinidumpContextAMD64Writer) >=
+                  alignof(MinidumpContextAMD64),
+              "MinidumpContextAMD64Writer alignment");
+
 MinidumpContextAMD64Writer::MinidumpContextAMD64Writer()
     : MinidumpContextWriter(), context_() {
   context_.context_flags = kMinidumpContextAMD64;
@@ -160,6 +165,20 @@ MinidumpContextAMD64Writer::MinidumpContextAMD64Writer()
 MinidumpContextAMD64Writer::~MinidumpContextAMD64Writer() {
 }
 
+// static
+void* MinidumpContextAMD64Writer::operator new(size_t size) {
+  // MinidumpContextAMD64 requests an alignment of 16, which can be larger than
+  // what standard new provides. This may trigger MSVC warning C4316. As a
+  // workaround to this language deficiency, provide a custom allocation
+  // function to allocate a block meeting the alignment requirement.
+  return AlignedAllocate(alignof(MinidumpContextAMD64Writer), size);
+}
+
+// static
+void MinidumpContextAMD64Writer::operator delete(void* pointer) {
+  return AlignedFree(pointer);
+}
+
 void MinidumpContextAMD64Writer::InitializeFromSnapshot(
     const CPUContextX86_64* context_snapshot) {
   DCHECK_EQ(state(), kStateMutable);

minidump/minidump_context_writer.h

@@ -110,6 +110,16 @@ class MinidumpContextAMD64Writer final : public MinidumpContextWriter {
   MinidumpContextAMD64Writer();
   ~MinidumpContextAMD64Writer() override;
 
+  // Ensure proper alignment of heap-allocated objects. This should not be
+  // necessary in C++17.
+  static void* operator new(size_t size);
+  static void operator delete(void* ptr);
+
+  // Prevent unaligned heap-allocated arrays. Provisions could be made to allow
+  // these if necessary, but there is currently no use for them.
+  static void* operator new[](size_t size) = delete;
+  static void operator delete[](void* ptr) = delete;
+
   //! \brief Initializes the MinidumpContextAMD64 based on \a context_snapshot.
   //!
   //! \param[in] context_snapshot The context snapshot to use as source data.

minidump/minidump_context_writer_test.cc

@@ -69,6 +69,15 @@ TEST(MinidumpContextWriter, MinidumpContextX86Writer) {
 }
 
 TEST(MinidumpContextWriter, MinidumpContextAMD64Writer) {
+  {
+    // Make sure that a heap-allocated context writer has the proper alignment,
+    // because it may be nonstandard.
+    auto context_writer = std::make_unique<MinidumpContextAMD64Writer>();
+    EXPECT_EQ(reinterpret_cast<uintptr_t>(context_writer.get()) &
+                  (alignof(MinidumpContextAMD64Writer) - 1),
+              0u);
+  }
+
   StringFile string_file;
 
   {

minidump/minidump_thread_writer_test.cc

@@ -220,13 +220,7 @@ TEST(MinidumpThreadWriter, OneThread_AMD64_Stack) {
       kMemoryBase, kMemorySize, kMemoryValue);
   thread_writer->SetStack(std::move(memory_writer));
 
-  // Object allocated on heap may not be aligned.
-  MSVC_PUSH_DISABLE_WARNING(4316);
-  // This would use std::make_unique, but since the “new” would be in <memory>
-  // and not here, MSVC_PUSH_DISABLE_WARNING wouldn’t have the intended effect.
-  std::unique_ptr<MinidumpContextAMD64Writer> context_amd64_writer(
-      new MinidumpContextAMD64Writer());
-  MSVC_POP_WARNING();  // C4316.
+  auto context_amd64_writer = std::make_unique<MinidumpContextAMD64Writer>();
   InitializeMinidumpContextAMD64(context_amd64_writer->context(), kSeed);
   thread_writer->SetContext(std::move(context_amd64_writer));
 

util/stdlib/aligned_allocator.cc

@@ -42,7 +42,6 @@ void ThrowBadAlloc() {
 }  // namespace
 
 namespace crashpad {
-namespace internal {
 
 void* AlignedAllocate(size_t alignment, size_t size) {
 #if defined(OS_POSIX)
@@ -74,5 +73,4 @@ void AlignedFree(void* pointer) {
 #endif  // OS_POSIX
 }
 
-}  // namespace internal
 }  // namespace crashpad

util/stdlib/aligned_allocator.h

@@ -24,7 +24,6 @@
 #include <vector>
 
 namespace crashpad {
-namespace internal {
 
 //! \brief Allocates memory with the specified alignment constraint.
 //!
@@ -37,8 +36,6 @@ void* AlignedAllocate(size_t alignment, size_t size);
 //! This function wraps `free()` or `_aligned_free()`.
 void AlignedFree(void* pointer);
 
-}  // namespace internal
-
 //! \brief A standard allocator that aligns its allocations as requested,
 //!     suitable for use as an allocator in standard containers.
 //!
@@ -74,10 +71,10 @@ struct AlignedAllocator {
 
   pointer allocate(size_type n, std::allocator<void>::const_pointer hint = 0) {
     return reinterpret_cast<pointer>(
-        internal::AlignedAllocate(Alignment, sizeof(value_type) * n));
+        AlignedAllocate(Alignment, sizeof(value_type) * n));
   }
 
-  void deallocate(pointer p, size_type n) { internal::AlignedFree(p); }
+  void deallocate(pointer p, size_type n) { AlignedFree(p); }
 
   size_type max_size() const noexcept {
     return std::numeric_limits<size_type>::max() / sizeof(value_type);