commit ed9d4b9ed96fbfb4e626f3f4c185dfa76c3cce02
author Mitch Phillips <31459023+hctim@users.noreply.github.com> Mon Jul 27 10:51:53 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Sep 30 11:43:10 2020 -0700
tree 78b2f69162a751f9028dee971ece345ba3ac7f31
parent df841da2f78ef5dca876a2fde67e1e39f4e1ca24

[GWP-ASan] Crash handler API returns sizeof(collected trace)

Summary:
Fix up a slight bug with the crash handler API, where we say that we
return the size of the collected trace (instead of the size of the trace
that's returned) when the return buffer is too small, and the result is
truncated.

Also, as a result, patch up a small uninitialized memory bug.

Reviewers: morehouse, eugenis

Reviewed By: eugenis

Subscribers: #sanitizers

Tags: #sanitizers

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

GitOrigin-RevId: d19af2f2476b5e13a65d5283cce9859e2c1ef763

common.cpp

diff --git a/common.cpp b/common.cpp
index 3438c4b..483694d 100644
--- a/common.cpp
+++ b/common.cpp
@@ -34,6 +34,9 @@
   __builtin_trap();
 }
 
+constexpr size_t AllocationMetadata::kStackFrameStorageBytes;
+constexpr size_t AllocationMetadata::kMaxTraceLengthToCollect;
+
 void AllocationMetadata::RecordAllocation(uintptr_t AllocAddr,
                                           size_t AllocSize) {
   Addr = AllocAddr;

crash_handler.cpp

diff --git a/crash_handler.cpp b/crash_handler.cpp
index c3b9e14..3c64025 100644
--- a/crash_handler.cpp
+++ b/crash_handler.cpp
@@ -10,6 +10,7 @@
 #include "gwp_asan/stack_trace_compressor.h"
 
 #include <assert.h>
+#include <string.h>
 
 using AllocationMetadata = gwp_asan::AllocationMetadata;
 using Error = gwp_asan::Error;
@@ -112,9 +113,15 @@
 size_t __gwp_asan_get_allocation_trace(
     const gwp_asan::AllocationMetadata *AllocationMeta, uintptr_t *Buffer,
     size_t BufferLen) {
-  return gwp_asan::compression::unpack(
+  uintptr_t UncompressedBuffer[AllocationMetadata::kMaxTraceLengthToCollect];
+  size_t UnpackedLength = gwp_asan::compression::unpack(
       AllocationMeta->AllocationTrace.CompressedTrace,
-      AllocationMeta->AllocationTrace.TraceSize, Buffer, BufferLen);
+      AllocationMeta->AllocationTrace.TraceSize, UncompressedBuffer,
+      AllocationMetadata::kMaxTraceLengthToCollect);
+  if (UnpackedLength < BufferLen)
+    BufferLen = UnpackedLength;
+  memcpy(Buffer, UncompressedBuffer, BufferLen * sizeof(*Buffer));
+  return UnpackedLength;
 }
 
 bool __gwp_asan_is_deallocated(
@@ -130,9 +137,15 @@
 size_t __gwp_asan_get_deallocation_trace(
     const gwp_asan::AllocationMetadata *AllocationMeta, uintptr_t *Buffer,
     size_t BufferLen) {
-  return gwp_asan::compression::unpack(
+  uintptr_t UncompressedBuffer[AllocationMetadata::kMaxTraceLengthToCollect];
+  size_t UnpackedLength = gwp_asan::compression::unpack(
       AllocationMeta->DeallocationTrace.CompressedTrace,
-      AllocationMeta->DeallocationTrace.TraceSize, Buffer, BufferLen);
+      AllocationMeta->DeallocationTrace.TraceSize, UncompressedBuffer,
+      AllocationMetadata::kMaxTraceLengthToCollect);
+  if (UnpackedLength < BufferLen)
+    BufferLen = UnpackedLength;
+  memcpy(Buffer, UncompressedBuffer, BufferLen * sizeof(*Buffer));
+  return UnpackedLength;
 }
 
 #ifdef __cplusplus

tests/backtrace.cpp

diff --git a/tests/backtrace.cpp b/tests/backtrace.cpp
index b3d4427..9515065 100644
--- a/tests/backtrace.cpp
+++ b/tests/backtrace.cpp
@@ -8,6 +8,7 @@
 
 #include <string>
 
+#include "gwp_asan/common.h"
 #include "gwp_asan/crash_handler.h"
 #include "gwp_asan/tests/harness.h"
 
@@ -76,9 +77,46 @@
 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 *TraceBuffer, size_t Size) -> size_t {
+        // Need to inintialise the elements that will be packed.
+        memset(TraceBuffer, 0u, Size * sizeof(*TraceBuffer));
+
+        // Indicate that there were more frames, and we just didn't have enough
+        // room to store them.
+        return Size * 2;
+      });
+  // Retrieve a frame from the collected backtrace, make sure it works E2E.
+  uintptr_t TraceOutput;
+  EXPECT_EQ(gwp_asan::AllocationMetadata::kMaxTraceLengthToCollect,
+            __gwp_asan_get_allocation_trace(&Meta, &TraceOutput, 1));
+}
+
+TEST(Backtrace, ExceedsRetrievableAllocLength) {
+  gwp_asan::AllocationMetadata Meta;
+  constexpr size_t kNumFramesToStore = 3u;
+  Meta.AllocationTrace.RecordBacktrace(
+      [](uintptr_t *TraceBuffer, size_t /* Size */) -> size_t {
+        memset(TraceBuffer, kNumFramesToStore,
+               kNumFramesToStore * sizeof(*TraceBuffer));
+        return kNumFramesToStore;
       });
   uintptr_t TraceOutput;
-  EXPECT_EQ(1u, __gwp_asan_get_allocation_trace(&Meta, &TraceOutput, 1));
+  // Ask for one element, get told that there's `kNumFramesToStore` available.
+  EXPECT_EQ(kNumFramesToStore,
+            __gwp_asan_get_allocation_trace(&Meta, &TraceOutput, 1));
+}
+
+TEST(Backtrace, ExceedsRetrievableDeallocLength) {
+  gwp_asan::AllocationMetadata Meta;
+  constexpr size_t kNumFramesToStore = 3u;
+  Meta.DeallocationTrace.RecordBacktrace(
+      [](uintptr_t *TraceBuffer, size_t /* Size */) -> size_t {
+        memset(TraceBuffer, kNumFramesToStore,
+               kNumFramesToStore * sizeof(*TraceBuffer));
+        return kNumFramesToStore;
+      });
+  uintptr_t TraceOutput;
+  // Ask for one element, get told that there's `kNumFramesToStore` available.
+  EXPECT_EQ(kNumFramesToStore,
+            __gwp_asan_get_deallocation_trace(&Meta, &TraceOutput, 1));
 }