[Sampling profiler] Use the same stack alignment in the stack copy

Updates the stack copying to ensure the copy has the same alignment as
the original stack. This fixes a bug where the copying would attempt to
read past the end of the stack on unaligned frame pointers. It is also
required to ensure the values operated on by the in-stack pointer
rewriting match the alignment of the actual pointer values in the stack.
And it avoids unaligned reads and writes, which will be required for use
with arm.

Bug: 956626, 882450
Change-Id: Ie694704813d523d7db9ecb45eb54c3775d86cbe1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1592698
Commit-Queue: Mike Wittman <wittman@chromium.org>
Reviewed-by: Charlie Andrews <charliea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657793}

base/profiler/stack_sampler_impl.cc

@@ -77,6 +77,7 @@ bool StackSamplerImpl::CopyStack(StackBuffer* stack_buffer,
                                  RegisterContext* thread_context) {
   const uintptr_t top = thread_delegate_->GetStackBaseAddress();
   uintptr_t bottom = 0;
+  const uint8_t* stack_copy_bottom = nullptr;
   {
     // Allocation of the ScopedSuspendThread object itself is OK since it
     // necessarily occurs before the thread is suspended by the object.
@@ -103,19 +104,18 @@ bool StackSamplerImpl::CopyStack(StackBuffer* stack_buffer,
 
     profile_builder->RecordMetadata();
 
-    CopyStackContentsAndRewritePointers(reinterpret_cast<uintptr_t*>(bottom),
-                                        reinterpret_cast<uintptr_t*>(top),
-                                        stack_buffer->buffer());
+    stack_copy_bottom = CopyStackContentsAndRewritePointers(
+        reinterpret_cast<uint8_t*>(bottom), reinterpret_cast<uintptr_t*>(top),
+        stack_buffer->buffer());
   }
 
-  *stack_top =
-      reinterpret_cast<uintptr_t>(stack_buffer->buffer()) + (top - bottom);
+  *stack_top = reinterpret_cast<uintptr_t>(stack_copy_bottom) + (top - bottom);
 
   for (uintptr_t* reg :
        thread_delegate_->GetRegistersToRewrite(thread_context)) {
-    *reg = RewritePointerIfInOriginalStack(reinterpret_cast<uintptr_t*>(bottom),
+    *reg = RewritePointerIfInOriginalStack(reinterpret_cast<uint8_t*>(bottom),
                                            reinterpret_cast<uintptr_t*>(top),
-                                           stack_buffer->buffer(), *reg);
+                                           stack_copy_bottom, *reg);
   }
 
   return true;
@@ -168,11 +168,10 @@ std::vector<Frame> StackSamplerImpl::WalkStack(ModuleCache* module_cache,
 // If the value at |pointer| points to the original stack, rewrite it to point
 // to the corresponding location in the copied stack. NO HEAP ALLOCATIONS.
 // static
-uintptr_t RewritePointerIfInOriginalStack(
-    const uintptr_t* original_stack_bottom,
-    const uintptr_t* original_stack_top,
-    const uintptr_t* stack_copy_bottom,
-    uintptr_t pointer) {
+uintptr_t RewritePointerIfInOriginalStack(const uint8_t* original_stack_bottom,
+                                          const uintptr_t* original_stack_top,
+                                          const uint8_t* stack_copy_bottom,
+                                          uintptr_t pointer) {
   auto original_stack_bottom_uint =
       reinterpret_cast<uintptr_t>(original_stack_bottom);
   auto original_stack_top_uint =
@@ -199,19 +198,58 @@ uintptr_t RewritePointerIfInOriginalStack(
 // to catch all pointers because the stacks are guaranteed by the ABI to be
 // sizeof(uintptr_t*) aligned.
 //
+// |original_stack_bottom| and |original_stack_top| are different pointer types
+// due on their differing guaranteed alignments -- the bottom may only be 1-byte
+// aligned while the top is aligned to double the pointer width.
+//
+// Returns a pointer to the bottom address in the copied stack. This value
+// matches the alignment of |original_stack_bottom| to ensure that the stack
+// contents have the same alignment as in the original stack. As a result the
+// value will be different than |stack_buffer_bottom| if |original_stack_bottom|
+// is not aligned to double the pointer width.
+//
 // NO HEAP ALLOCATIONS.
 //
 // static
 NO_SANITIZE("address")
-void CopyStackContentsAndRewritePointers(const uintptr_t* original_stack_bottom,
-                                         const uintptr_t* original_stack_top,
-                                         uintptr_t* stack_copy_bottom) {
-  const uintptr_t* src = original_stack_bottom;
-  uintptr_t* dst = stack_copy_bottom;
+const uint8_t* CopyStackContentsAndRewritePointers(
+    const uint8_t* original_stack_bottom,
+    const uintptr_t* original_stack_top,
+    uintptr_t* stack_buffer_bottom) {
+  const uint8_t* byte_src = original_stack_bottom;
+  // The first address in the stack with pointer alignment. Pointer-aligned
+  // values from this point to the end of the stack are possibly rewritten using
+  // RewritePointerIfInOriginalStack(). Bytes before this cannot be a pointer
+  // because they occupy less space than a pointer would.
+  const uint8_t* first_aligned_address = reinterpret_cast<uint8_t*>(
+      (reinterpret_cast<uintptr_t>(byte_src) + sizeof(uintptr_t) - 1) &
+      ~(sizeof(uintptr_t) - 1));
+
+  // The stack copy bottom, which is offset from |stack_buffer_bottom| by the
+  // same alignment as in the original stack. This guarantees identical
+  // alignment between values in the original stack and the copy. This uses the
+  // platform stack alignment rather than pointer alignment so that the stack
+  // copy is aligned to platform expectations.
+  uint8_t* stack_copy_bottom =
+      reinterpret_cast<uint8_t*>(stack_buffer_bottom) +
+      (reinterpret_cast<uintptr_t>(byte_src) &
+       (StackSampler::StackBuffer::kPlatformStackAlignment - 1));
+  uint8_t* byte_dst = stack_copy_bottom;
+
+  // Copy bytes verbatim up to the first aligned address.
+  for (; byte_src < first_aligned_address; ++byte_src, ++byte_dst)
+    *byte_dst = *byte_src;
+
+  // Copy the remaining stack by pointer-sized values, rewriting anything that
+  // looks like a pointer into the stack.
+  const uintptr_t* src = reinterpret_cast<const uintptr_t*>(byte_src);
+  uintptr_t* dst = reinterpret_cast<uintptr_t*>(byte_dst);
   for (; src < original_stack_top; ++src, ++dst) {
     *dst = RewritePointerIfInOriginalStack(
         original_stack_bottom, original_stack_top, stack_copy_bottom, *src);
   }
+
+  return stack_copy_bottom;
 }
 
 }  // namespace base

base/profiler/stack_sampler_impl.h

@@ -64,15 +64,15 @@ class BASE_EXPORT StackSamplerImpl : public StackSampler {
 // These two functions are exposed for testing.
 
 BASE_EXPORT uintptr_t
-RewritePointerIfInOriginalStack(const uintptr_t* original_stack_bottom,
+RewritePointerIfInOriginalStack(const uint8_t* original_stack_bottom,
                                 const uintptr_t* original_stack_top,
-                                const uintptr_t* stack_copy_bottom,
+                                const uint8_t* stack_copy_bottom,
                                 uintptr_t pointer);
 
-BASE_EXPORT void CopyStackContentsAndRewritePointers(
-    const uintptr_t* original_stack_bottom,
+BASE_EXPORT const uint8_t* CopyStackContentsAndRewritePointers(
+    const uint8_t* original_stack_bottom,
     const uintptr_t* original_stack_top,
-    uintptr_t* stack_copy_bottom);
+    uintptr_t* stack_buffer_bottom);
 
 }  // namespace base
 

base/profiler/stack_sampler_impl_unittest.cc

@@ -2,7 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
+#include <cstring>
 #include <memory>
+#include <numeric>
 #include <utility>
 
 #include "base/profiler/profile_builder.h"
@@ -224,16 +227,26 @@ class FakeTestUnwinder : public Unwinder {
   std::vector<Result> results_;
 };
 
+static constexpr size_t kTestStackBufferSize = sizeof(uintptr_t) * 4;
+
+union alignas(StackSampler::StackBuffer::kPlatformStackAlignment)
+    TestStackBuffer {
+  uintptr_t as_uintptr[kTestStackBufferSize / sizeof(uintptr_t)];
+  uint16_t as_uint16[kTestStackBufferSize / sizeof(uint16_t)];
+  uint8_t as_uint8[kTestStackBufferSize / sizeof(uint8_t)];
+};
+
 }  // namespace
 
 TEST(StackSamplerImplTest, RewritePointerIfInOriginalStack_InStack) {
   uintptr_t original_stack[4];
   uintptr_t stack_copy[4];
-  EXPECT_EQ(
-      reinterpret_cast<uintptr_t>(&stack_copy[2]),
-      RewritePointerIfInOriginalStack(
-          &original_stack[0], &original_stack[0] + base::size(original_stack),
-          &stack_copy[0], reinterpret_cast<uintptr_t>(&original_stack[2])));
+  EXPECT_EQ(reinterpret_cast<uintptr_t>(&stack_copy[2]),
+            RewritePointerIfInOriginalStack(
+                reinterpret_cast<uint8_t*>(&original_stack[0]),
+                &original_stack[0] + base::size(original_stack),
+                reinterpret_cast<uint8_t*>(&stack_copy[0]),
+                reinterpret_cast<uintptr_t>(&original_stack[2])));
 }
 
 TEST(StackSamplerImplTest, RewritePointerIfInOriginalStack_NotInStack) {
@@ -243,26 +256,178 @@ TEST(StackSamplerImplTest, RewritePointerIfInOriginalStack_NotInStack) {
   uintptr_t original_stack[4];
   uintptr_t stack_copy[4];
 
-  EXPECT_EQ(
-      reinterpret_cast<uintptr_t>(&non_stack_location),
-      RewritePointerIfInOriginalStack(
-          &original_stack[0], &original_stack[0] + size(original_stack),
-          &stack_copy[0], reinterpret_cast<uintptr_t>(&non_stack_location)));
+  EXPECT_EQ(reinterpret_cast<uintptr_t>(&non_stack_location),
+            RewritePointerIfInOriginalStack(
+                reinterpret_cast<uint8_t*>(&original_stack[0]),
+                &original_stack[0] + size(original_stack),
+                reinterpret_cast<uint8_t*>(&stack_copy[0]),
+                reinterpret_cast<uintptr_t>(&non_stack_location)));
 }
 
-TEST(StackSamplerImplTest, CopyStackContentsAndRewritePointers) {
-  uintptr_t original_stack[4] = {1, 2, 0, 4};
-  original_stack[2] = reinterpret_cast<uintptr_t>(&original_stack[1]);
-  uintptr_t stack_copy[4];
+TEST(StackSamplerImplTest, StackCopy) {
+  TestStackBuffer original_stack;
+  // Fill the stack buffer with increasing uintptr_t values.
+  std::iota(&original_stack.as_uintptr[0],
+            &original_stack.as_uintptr[0] + size(original_stack.as_uintptr),
+            100);
+  // Replace the third value with an address within the buffer.
+  original_stack.as_uintptr[2] =
+      reinterpret_cast<uintptr_t>(&original_stack.as_uintptr[1]);
+  TestStackBuffer stack_copy;
+
+  CopyStackContentsAndRewritePointers(
+      &original_stack.as_uint8[0],
+      &original_stack.as_uintptr[0] + size(original_stack.as_uintptr),
+      &stack_copy.as_uintptr[0]);
+
+  EXPECT_EQ(original_stack.as_uintptr[0], stack_copy.as_uintptr[0]);
+  EXPECT_EQ(original_stack.as_uintptr[1], stack_copy.as_uintptr[1]);
+  EXPECT_EQ(reinterpret_cast<uintptr_t>(&stack_copy.as_uintptr[1]),
+            stack_copy.as_uintptr[2]);
+  EXPECT_EQ(original_stack.as_uintptr[3], stack_copy.as_uintptr[3]);
+}
+
+TEST(StackSamplerImplTest, StackCopy_NonAlignedStackPointerCopy) {
+  TestStackBuffer stack_buffer;
+
+  // Fill the stack buffer with increasing uint16_t values.
+  std::iota(&stack_buffer.as_uint16[0],
+            &stack_buffer.as_uint16[0] + size(stack_buffer.as_uint16), 100);
+
+  // Set the stack bottom to the unaligned location one uint16_t into the
+  // buffer.
+  uint8_t* unaligned_stack_bottom =
+      reinterpret_cast<uint8_t*>(&stack_buffer.as_uint16[1]);
+
+  // Leave extra space within the stack buffer beyond the end of the stack, but
+  // preserve the platform alignment.
+  const size_t extra_space = StackSampler::StackBuffer::kPlatformStackAlignment;
+  uintptr_t* stack_top =
+      &stack_buffer.as_uintptr[size(stack_buffer.as_uintptr) -
+                               extra_space / sizeof(uintptr_t)];
+
+  // Initialize the copy to all zeros.
+  TestStackBuffer stack_copy_buffer = {{0}};
+
+  const uint8_t* stack_copy_bottom = CopyStackContentsAndRewritePointers(
+      unaligned_stack_bottom, stack_top, &stack_copy_buffer.as_uintptr[0]);
+
+  // The stack copy bottom address is expected to be at the same offset into the
+  // stack copy buffer as the unaligned stack bottom is from the stack buffer.
+  // Since the buffers have the same platform stack alignment this also ensures
+  // the alignment of the bottom addresses is the same.
+  EXPECT_EQ(unaligned_stack_bottom - &stack_buffer.as_uint8[0],
+            stack_copy_bottom - &stack_copy_buffer.as_uint8[0]);
+
+  // The first value in the copy should not be overwritten since the stack
+  // starts at the second uint16_t.
+  EXPECT_EQ(0u, stack_copy_buffer.as_uint16[0]);
+
+  // The next values up to the extra space should have been copied.
+  const size_t max_index =
+      size(stack_copy_buffer.as_uint16) - extra_space / sizeof(uint16_t);
+  for (size_t i = 1; i < max_index; ++i)
+    EXPECT_EQ(i + 100, stack_copy_buffer.as_uint16[i]);
+
+  // None of the values in the empty space should have been copied.
+  for (size_t i = max_index; i < size(stack_copy_buffer.as_uint16); ++i)
+    EXPECT_EQ(0u, stack_copy_buffer.as_uint16[i]);
+}
+
+// Checks that an unaligned within-stack pointer value at the start of the stack
+// is not rewritten.
+TEST(StackSamplerImplTest,
+     StackCopy_NonAlignedStackPointerUnalignedRewriteAtStart) {
+  // Initially fill the buffer with 0s.
+  TestStackBuffer stack_buffer = {{0}};
+
+  // Set the stack bottom to the unaligned location one uint16_t into the
+  // buffer.
+  uint8_t* unaligned_stack_bottom =
+      reinterpret_cast<uint8_t*>(&stack_buffer.as_uint16[1]);
+
+  // Set the first unaligned pointer-sized value to an address within the stack.
+  uintptr_t within_stack_pointer =
+      reinterpret_cast<uintptr_t>(&stack_buffer.as_uintptr[2]);
+  std::memcpy(unaligned_stack_bottom, &within_stack_pointer,
+              sizeof(within_stack_pointer));
+
+  TestStackBuffer stack_copy_buffer = {{0}};
+
+  const uint8_t* stack_copy_bottom = CopyStackContentsAndRewritePointers(
+      unaligned_stack_bottom,
+      &stack_buffer.as_uintptr[0] + size(stack_buffer.as_uintptr),
+      &stack_copy_buffer.as_uintptr[0]);
+
+  uintptr_t copied_within_stack_pointer;
+  std::memcpy(&copied_within_stack_pointer, stack_copy_bottom,
+              sizeof(copied_within_stack_pointer));
+
+  // The rewriting should only operate on pointer-aligned values so the
+  // unaligned value should be copied verbatim.
+  EXPECT_EQ(within_stack_pointer, copied_within_stack_pointer);
+}
+
+// Checks that an unaligned within-stack pointer after the start of the stack is
+// not rewritten.
+TEST(StackSamplerImplTest,
+     StackCopy_NonAlignedStackPointerUnalignedRewriteAfterStart) {
+  // Initially fill the buffer with 0s.
+  TestStackBuffer stack_buffer = {{0}};
+
+  // Set the stack bottom to the unaligned location one uint16_t into the
+  // buffer.
+  uint8_t* unaligned_stack_bottom =
+      reinterpret_cast<uint8_t*>(&stack_buffer.as_uint16[1]);
+
+  // Set the second unaligned pointer-sized value to an address within the
+  // stack.
+  uintptr_t within_stack_pointer =
+      reinterpret_cast<uintptr_t>(&stack_buffer.as_uintptr[2]);
+  std::memcpy(unaligned_stack_bottom + sizeof(uintptr_t), &within_stack_pointer,
+              sizeof(within_stack_pointer));
+
+  TestStackBuffer stack_copy_buffer = {{0}};
+
+  const uint8_t* stack_copy_bottom = CopyStackContentsAndRewritePointers(
+      unaligned_stack_bottom,
+      &stack_buffer.as_uintptr[0] + size(stack_buffer.as_uintptr),
+      &stack_copy_buffer.as_uintptr[0]);
+
+  uintptr_t copied_within_stack_pointer;
+  std::memcpy(&copied_within_stack_pointer,
+              stack_copy_bottom + sizeof(uintptr_t),
+              sizeof(copied_within_stack_pointer));
+
+  // The rewriting should only operate on pointer-aligned values so the
+  // unaligned value should be copied verbatim.
+  EXPECT_EQ(within_stack_pointer, copied_within_stack_pointer);
+}
+
+TEST(StackSamplerImplTest, StackCopy_NonAlignedStackPointerAlignedRewrite) {
+  // Initially fill the buffer with 0s.
+  TestStackBuffer stack_buffer = {{0}};
+
+  // Set the stack bottom to the unaligned location one uint16_t into the
+  // buffer.
+  uint8_t* unaligned_stack_bottom =
+      reinterpret_cast<uint8_t*>(&stack_buffer.as_uint16[1]);
+
+  // Set the second aligned pointer-sized value to an address within the stack.
+  stack_buffer.as_uintptr[1] =
+      reinterpret_cast<uintptr_t>(&stack_buffer.as_uintptr[2]);
+
+  TestStackBuffer stack_copy_buffer = {{0}};
 
-  CopyStackContentsAndRewritePointers(&original_stack[0],
-                                      &original_stack[0] + size(original_stack),
-                                      &stack_copy[0]);
+  CopyStackContentsAndRewritePointers(
+      unaligned_stack_bottom,
+      &stack_buffer.as_uintptr[0] + size(stack_buffer.as_uintptr),
+      &stack_copy_buffer.as_uintptr[0]);
 
-  EXPECT_EQ(original_stack[0], stack_copy[0]);
-  EXPECT_EQ(original_stack[1], stack_copy[1]);
-  EXPECT_EQ(reinterpret_cast<uintptr_t>(&stack_copy[1]), stack_copy[2]);
-  EXPECT_EQ(original_stack[3], stack_copy[3]);
+  // The aligned pointer should have been rewritten to point within the stack
+  // copy.
+  EXPECT_EQ(reinterpret_cast<uintptr_t>(&stack_copy_buffer.as_uintptr[2]),
+            stack_copy_buffer.as_uintptr[1]);
 }
 
 TEST(StackSamplerImplTest, CopyStack) {