[PATCH] D42289: [XRay] [compiler-rt] fix heap overflow by computing record pointers correctly

[Martin Pelikán via Phabricator via llvm-commits]

pelikan created this revision.
pelikan added reviewers: eizan, kpw, dberris, dblaikie.
Herald added a subscriber: Sanitizers.

While there, unify InMemoryRawLog and InMemoryRawLogWithArg's coding style:

- swap libc's memcpy(3) for sanitizer's internal memcpy
- use basic pointer arithmetics to compute offsets from the first record entry in the pre-allocated buffer, which is always the appropriate type for the given function
- lose the local variable references as the TLD.* names fit just as well


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D42289

Files:
  lib/xray/xray_inmemory_log.cc


Index: lib/xray/xray_inmemory_log.cc
===================================================================
--- lib/xray/xray_inmemory_log.cc
+++ lib/xray/xray_inmemory_log.cc
@@ -157,7 +157,6 @@
 void InMemoryRawLog(int32_t FuncId, XRayEntryType Type,
                     RDTSC ReadTSC) XRAY_NEVER_INSTRUMENT {
   auto &TLD = getThreadLocalData();
-  auto &InMemoryBuffer = TLD.InMemoryBuffer;
   int Fd = getGlobalFd();
   if (Fd == -1)
     return;
@@ -240,14 +239,12 @@
   R.TId = TLD.TID;
   R.Type = Type;
   R.FuncId = FuncId;
-  auto EntryPtr = static_cast<char *>(InMemoryBuffer) +
-                  (sizeof(__xray::XRayRecord) * TLD.BufferOffset);
-  __sanitizer::internal_memcpy(EntryPtr, &R, sizeof(R));
+  auto FirstEntry = reinterpret_cast<__xray::XRayRecord *>(TLD.InMemoryBuffer);
+  __sanitizer::internal_memcpy(FirstEntry + TLD.BufferOffset, &R, sizeof(R));
   if (++TLD.BufferOffset == TLD.BufferSize) {
     __sanitizer::SpinMutexLock L(&LogMutex);
-    auto RecordBuffer = reinterpret_cast<__xray::XRayRecord *>(InMemoryBuffer);
-    retryingWriteAll(Fd, reinterpret_cast<char *>(RecordBuffer),
-                     reinterpret_cast<char *>(RecordBuffer + TLD.BufferOffset));
+    retryingWriteAll(Fd, reinterpret_cast<char *>(FirstEntry),
+                     reinterpret_cast<char *>(FirstEntry + TLD.BufferOffset));
     TLD.BufferOffset = 0;
     TLD.StackEntries = 0;
   }
@@ -257,22 +254,21 @@
 void InMemoryRawLogWithArg(int32_t FuncId, XRayEntryType Type, uint64_t Arg1,
                            RDTSC ReadTSC) XRAY_NEVER_INSTRUMENT {
   auto &TLD = getThreadLocalData();
-  auto &InMemoryBuffer = TLD.InMemoryBuffer;
-  auto &Offset = TLD.BufferOffset;
+  auto FirstEntry =
+      reinterpret_cast<__xray::XRayArgPayload *>(TLD.InMemoryBuffer);
   const auto &BuffLen = TLD.BufferSize;
   int Fd = getGlobalFd();
   if (Fd == -1)
     return;
 
   // First we check whether there's enough space to write the data consecutively
   // in the thread-local buffer. If not, we first flush the buffer before
   // attempting to write the two records that must be consecutive.
-  if (Offset + 2 > BuffLen) {
+  if (TLD.BufferOffset + 2 > BuffLen) {
     __sanitizer::SpinMutexLock L(&LogMutex);
-    auto RecordBuffer = reinterpret_cast<__xray::XRayRecord *>(InMemoryBuffer);
-    retryingWriteAll(Fd, reinterpret_cast<char *>(RecordBuffer),
-                     reinterpret_cast<char *>(RecordBuffer + Offset));
-    Offset = 0;
+    retryingWriteAll(Fd, reinterpret_cast<char *>(FirstEntry),
+                     reinterpret_cast<char *>(FirstEntry + TLD.BufferOffset));
+    TLD.BufferOffset = 0;
     TLD.StackEntries = 0;
   }
 
@@ -290,15 +286,12 @@
   R.FuncId = FuncId;
   R.TId = TLD.TID;
   R.Arg = Arg1;
-  auto EntryPtr =
-      &reinterpret_cast<__xray::XRayArgPayload *>(&InMemoryBuffer)[Offset];
-  std::memcpy(EntryPtr, &R, sizeof(R));
-  if (++Offset == BuffLen) {
+  __sanitizer::internal_memcpy(FirstEntry + TLD.BufferOffset, &R, sizeof(R));
+  if (++TLD.BufferOffset == BuffLen) {
     __sanitizer::SpinMutexLock L(&LogMutex);
-    auto RecordBuffer = reinterpret_cast<__xray::XRayRecord *>(InMemoryBuffer);
-    retryingWriteAll(Fd, reinterpret_cast<char *>(RecordBuffer),
-                     reinterpret_cast<char *>(RecordBuffer + Offset));
-    Offset = 0;
+    retryingWriteAll(Fd, reinterpret_cast<char *>(FirstEntry),
+                     reinterpret_cast<char *>(FirstEntry + TLD.BufferOffset));
+    TLD.BufferOffset = 0;
     TLD.StackEntries = 0;
   }
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42289.130580.patch
Type: text/x-patch
Size: 3518 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180119/1932e5dd/attachment.bin>