commit 9204c4eb5d0fa2cab106a7fade1da9ea35cb043f
author Mitch Phillips <31459023+hctim@users.noreply.github.com> Tue Mar 24 14:37:07 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Sep 30 11:42:59 2020 -0700
tree dd09b8de6a6bc3fa4f760ea00fbf62a79e557bb5
parent 9fdbe7d2ed60e4702f94ade0c50b4e950ea5519d

[GWP-ASan] Only pack frames that are stored.

Summary:
Backtrace() returns the number of frames that are *available*, rather
than the number of frames stored. When we pack, we supply the number of
frames we have stored. The number of available frames can exceed the
number of stored frames, leading to stack OOB read.

Fix up this problem.

Reviewers: eugenis

Reviewed By: eugenis

Subscribers: #sanitizers, morehouse, cferris, pcc

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D76722

GitOrigin-RevId: a4e8d89704d2584d5c56cb27745beab25c7b9b36

common.cpp

diff --git a/common.cpp b/common.cpp
index 4493581..3438c4b 100644
--- a/common.cpp
+++ b/common.cpp
@@ -59,6 +59,11 @@
   uintptr_t UncompressedBuffer[kMaxTraceLengthToCollect];
   size_t BacktraceLength =
       Backtrace(UncompressedBuffer, kMaxTraceLengthToCollect);
+  // Backtrace() returns the number of available frames, which may be greater
+  // than the number of frames in the buffer. In this case, we need to only pack
+  // the number of frames that are in the buffer.
+  if (BacktraceLength > kMaxTraceLengthToCollect)
+    BacktraceLength = kMaxTraceLengthToCollect;
   TraceSize =
       compression::pack(UncompressedBuffer, BacktraceLength, CompressedTrace,
                         AllocationMetadata::kStackFrameStorageBytes);

tests/backtrace.cpp

diff --git a/tests/backtrace.cpp b/tests/backtrace.cpp
index bc81f35..6c9a930 100644
--- a/tests/backtrace.cpp
+++ b/tests/backtrace.cpp
@@ -8,6 +8,7 @@
 
 #include <string>
 
+#include "gwp_asan/crash_handler.h"
 #include "gwp_asan/tests/harness.h"
 
 TEST_F(BacktraceGuardedPoolAllocator, DoubleFree) {
@@ -15,13 +16,13 @@
   GPA.deallocate(Ptr);
 
   std::string DeathRegex = "Double Free.*";
-  DeathRegex.append("backtrace\\.cpp:25.*");
+  DeathRegex.append("backtrace\\.cpp:26.*");
 
   DeathRegex.append("was deallocated.*");
-  DeathRegex.append("backtrace\\.cpp:15.*");
+  DeathRegex.append("backtrace\\.cpp:16.*");
 
   DeathRegex.append("was allocated.*");
-  DeathRegex.append("backtrace\\.cpp:14.*");
+  DeathRegex.append("backtrace\\.cpp:15.*");
   ASSERT_DEATH(GPA.deallocate(Ptr), DeathRegex);
 }
 
@@ -30,12 +31,36 @@
   GPA.deallocate(Ptr);
 
   std::string DeathRegex = "Use After Free.*";
-  DeathRegex.append("backtrace\\.cpp:40.*");
+  DeathRegex.append("backtrace\\.cpp:41.*");
 
   DeathRegex.append("was deallocated.*");
-  DeathRegex.append("backtrace\\.cpp:30.*");
+  DeathRegex.append("backtrace\\.cpp:31.*");
 
   DeathRegex.append("was allocated.*");
-  DeathRegex.append("backtrace\\.cpp:29.*");
+  DeathRegex.append("backtrace\\.cpp:30.*");
   ASSERT_DEATH({ *Ptr = 7; }, DeathRegex);
 }
+
+TEST(Backtrace, Short) {
+  gwp_asan::AllocationMetadata Meta;
+  Meta.AllocationTrace.RecordBacktrace(
+      [](uintptr_t *TraceBuffer, size_t /* Size */) -> size_t {
+        TraceBuffer[0] = 123u;
+        TraceBuffer[1] = 321u;
+        return 2u;
+      });
+  uintptr_t TraceOutput[2] = {};
+  EXPECT_EQ(2u, __gwp_asan_get_allocation_trace(&Meta, TraceOutput, 2));
+  EXPECT_EQ(TraceOutput[0], 123u);
+  EXPECT_EQ(TraceOutput[1], 321u);
+}
+
+TEST(Backtrace, ExceedsStorableLength) {
+  gwp_asan::AllocationMetadata Meta;
+  Meta.AllocationTrace.RecordBacktrace(
+      [](uintptr_t * /* TraceBuffer */, size_t /* Size */) -> size_t {
+        return SIZE_MAX; // Wow, that's big!
+      });
+  uintptr_t TraceOutput;
+  EXPECT_EQ(1u, __gwp_asan_get_allocation_trace(&Meta, &TraceOutput, 1));
+}