[Profile] Handle invalid profile data

[aeubanks]

Summary

This mostly follows LLVM's InstrProfReader.cpp error handling.
Previously, attempting to merge corrupted profile data would result in
crashes. See https://crbug.com/1216811#c4.

[aeubanks]

formatting

[aeubanks]

trying to come up with a test case

[rnk]

I think it's worth the time to carefully refactor this code so we don't end up in a similar situation, maybe in the value profile data.

compiler-rt/lib/profile/InstrProfiling.h

110

This is a bit of an ABI backwards compatibility concern, but I think adding a return type should be safe enough.

compiler-rt/lib/profile/InstrProfilingFile.c

268

The joys of not having RAII...

compiler-rt/lib/profile/InstrProfilingMerge.c

100

I'm having a hard time reasoning about this code. My favorite code pattern for safely reading binary data in C++ is the "consume" pattern, illustrated in the consume* APIs in RecordSerialization.h:
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/DebugInfo/CodeView/RecordSerialization.h#L55

Unfortunately, I don't think it translate to C very well. I guess the main recommendation I have would be to keep everything in bytes for as long as possible, and delay casting until after bounds checking.

133

I think we should check SrcValueProfData + SrcValueProfData->TotalSize against ProfileSize while we're here. The NVK value seems like it's not really needed, it just checks for the absence of value data.

[aeubanks]

check SrcValueProfData

[aeubanks]

do more pointer comparisons
still working on a test

[rnk]

Sorry about all the nits, bounds checking is all about attention to detail.

compiler-rt/lib/profile/InstrProfilingMerge.c

101

size_t is unsigned, so this check is probably always false, and will probably trigger a warning in some build configurations.

114

size_t is unsigned, so underflow is possible here. The < 0 check below doesn't catch anything. I think that's fine: any value in the range [0, MaxNumCounters) is OK.

118

This is really nitty, but I think the overflow-safe way to do this addition and comparison is to use subtraction. We know both values are less than MaxNumCounters, so either subtraction is safe: NC >= MaxNumCounters - SrcCountersOffset

Also, should these be >=? I think the final condition should be:

if (SrcCountersOffset >= MaxNumCounters ||
    NC >= MaxNumCounters - SrcCountersOffset)
  return 1;

Hopefully I did that right.

120

nit: this variable is used once in the loop, maybe inline the addition into the loop, save a line (SrcCountersStart[SrcCountersOffset + I]). It makes it clearer that we are notionally indexing into the SrcCountersStart array, which is of length MaxNumCounters.

134

clang-tidy suggests a pointer cast here. Honestly, we'll have fewer casts if we change SrcValueProfData to be a const char * and cast when passing into VPMergeHook.

[aeubanks]

sorry, I think I updated while you were reviewing and some of your comments are no longer relevant

[aeubanks]

add test
make SrcValueProfData a char*

[aeubanks]

there are a couple ways to handle the invalid profile

1. remove it and bail
2. remove it and overwrite with the current profile
3. leave it and don't write anything more

1. seems the least useful, 3) seems the most useful for diagnosing issues, 2) is the most "keep going in the face of failures"

perhaps we should go with 3)?

[rnk]

lgtm

This fixes an urgent crash bug, so let's go ahead and land it.

+@davidxl @vsk @xur, feel free to provide post commit review.

compiler-rt/test/profile/Linux/corrupted-profile.c

47

I'm not sure this will reliably trigger the corruption check on big endian platforms. I'd recommend memsetting 8 bytes to 0xAB to reduce the likelihood of buildbot emails.

I also worry about 32-bit buildbots, but I think it's fine.

[aeubanks]

set all 8 bytes in test

[rnk]

I agree, it's a choice between 2 and 3. 3 would be consistent with what happens when the existing profraw file is from another binary, right? Maybe do that? It risks losing coverage data, and means the corrupt file is persisted. I guess that's best, since it will cause llvm-profdata merge to fail later.

[aeubanks]

keep old profile around, add test for that

[davidxl]

compiler-rt/lib/profile/InstrProfilingMerge.c

112

Is the second check redundant ? It is already checked outside the loop.

[aeubanks]

compiler-rt/lib/profile/InstrProfilingMerge.c

112

yes, fixed in b73742bc

[davidxl]

lgtm

compiler-rt/lib/profile/InstrProfiling.h

  * \brief Clear profile counters to zero.
  *
  */
 void __llvm_profile_reset_counters(void);
 
 /*!
  * \brief Merge profile data from buffer.
  *
- * Read profile data form buffer \p Profile  and merge with
- * in-process profile counters. The client is expected to
- * have checked or already knows the profile data in the
- * buffer matches the in-process counter structure before
- * calling it.
+ * Read profile data form buffer \p Profile  and merge with in-process profile
+ * counters. The client is expected to have checked or already knows the profile
+ * data in the buffer matches the in-process counter structure before calling
+ * it. Returns 0 (success) if the profile data is valid. Upon reading
+ * invalid/corrupted profile data, returns 1 (failure).
  */
-void __llvm_profile_merge_from_buffer(const char *Profile, uint64_t Size);
+int __llvm_profile_merge_from_buffer(const char *Profile, uint64_t Size);
 
 /*! \brief Check if profile in buffer matches the current binary.
  *
  *  Returns 0 (success) if the profile data in buffer \p Profile with size
  *  \p Size was generated by the same binary and therefore matches
  *  structurally the in-process counters. If the profile data in buffer is
  *  not compatible, the interface returns 1 (failure).
  */

compiler-rt/lib/profile/InstrProfilingFile.c

if (!ProfileFileSize)
     return 0;
 
   /* mmap() the profile and check that it is compatible with the data in
    * the current image. */
   if (mmapProfileForMerging(ProfileFile, ProfileFileSize, &ProfileBuffer) == -1)
     return -1;
 
   /* Now start merging */
-  __llvm_profile_merge_from_buffer(ProfileBuffer, ProfileFileSize);
+  if (__llvm_profile_merge_from_buffer(ProfileBuffer, ProfileFileSize)) {
+    PROF_ERR("%s\n", "Invalid profile data to merge");
+    (void)munmap(ProfileBuffer, ProfileFileSize);
+    return -1;
+  }
 
-  // Truncate the file in case merging of value profile did not happend to
+  // Truncate the file in case merging of value profile did not happen to
   // prevent from leaving garbage data at the end of the profile file.
   (void)COMPILER_RT_FTRUNCATE(ProfileFile,
                               __llvm_profile_get_size_for_buffer());
 
   (void)munmap(ProfileBuffer, ProfileFileSize);
   *MergeDone = 1;
 
   return 0;

compiler-rt/lib/profile/InstrProfilingMerge.c

if (SrcData->NameRef != DstData->NameRef ||
       return 1;
   }
 
   /* Matched! */
   return 0;
 }
 
 COMPILER_RT_VISIBILITY
-void __llvm_profile_merge_from_buffer(const char *ProfileData,
-                                      uint64_t ProfileSize) {
+int __llvm_profile_merge_from_buffer(const char *ProfileData,
   __llvm_profile_data *SrcDataStart, *SrcDataEnd, *SrcData, *DstData;
   __llvm_profile_header *Header = (__llvm_profile_header *)ProfileData;
   uint64_t *SrcCountersStart;
   const char *SrcNameStart;
-  ValueProfData *SrcValueProfDataStart, *SrcValueProfData;
+  const char *SrcValueProfDataStart, *SrcValueProfData;
 
   SrcDataStart =
       (__llvm_profile_data *)(ProfileData + sizeof(__llvm_profile_header));
   SrcDataEnd = SrcDataStart + Header->DataSize;
   SrcCountersStart = (uint64_t *)SrcDataEnd;
   SrcNameStart = (const char *)(SrcCountersStart + Header->CountersSize);
   SrcValueProfDataStart =
-      (ValueProfData *)(SrcNameStart + Header->NamesSize +
-                        __llvm_profile_get_num_padding_bytes(
-                            Header->NamesSize));
+      SrcNameStart + Header->NamesSize +
+      __llvm_profile_get_num_padding_bytes(Header->NamesSize);
+  if (SrcNameStart < (const char *)SrcCountersStart)
+    return 1;
 
   for (SrcData = SrcDataStart,
       DstData = (__llvm_profile_data *)__llvm_profile_begin_data(),
       SrcValueProfData = SrcValueProfDataStart;
        SrcData < SrcDataEnd; ++SrcData, ++DstData) {
-    uint64_t *SrcCounters;
     uint64_t *DstCounters = (uint64_t *)DstData->CounterPtr;
-    unsigned I, NC, NVK = 0;
+    unsigned NVK = 0;
 
-    NC = SrcData->NumCounters;
-    SrcCounters = SrcCountersStart +
-                  ((size_t)SrcData->CounterPtr - Header->CountersDelta) /
+    unsigned NC = SrcData->NumCounters;
+    if (NC == 0 || (const char *)SrcCountersStart >= SrcNameStart)
+      return 1;
+    uint64_t *SrcCounters = SrcCountersStart + ((size_t)SrcData->CounterPtr -
+                                                Header->CountersDelta) /
                                                    sizeof(uint64_t);
-    for (I = 0; I < NC; I++)
+    if (SrcCounters < SrcCountersStart ||
+        (const char *)SrcCounters >= SrcNameStart ||
+        (const char *)(SrcCounters + NC) > SrcNameStart)
+      return 1;
+    for (unsigned I = 0; I < NC; I++)
       DstCounters[I] += SrcCounters[I];
 
-    /* Now merge value profile data.  */
+    /* Now merge value profile data. */
     if (!VPMergeHook)
       continue;
 
-    for (I = 0; I <= IPVK_Last; I++)
+    for (unsigned I = 0; I <= IPVK_Last; I++)
       NVK += (SrcData->NumValueSites[I] != 0);
 
     if (!NVK)
       continue;
 
-    VPMergeHook(SrcValueProfData, DstData);
-    SrcValueProfData = (ValueProfData *)((char *)SrcValueProfData +
-                                         SrcValueProfData->TotalSize);
+    if (SrcValueProfData >= ProfileData + ProfileSize)
+      return 1;
+    VPMergeHook((ValueProfData *)SrcValueProfData, DstData);
+    SrcValueProfData =
+        SrcValueProfData + ((ValueProfData *)SrcValueProfData)->TotalSize;
   }
+
+  return 0;
 }

compiler-rt/test/profile/Linux/corrupted-profile.c

+// RUN: rm -f %t.profraw
+// RUN: touch %t.profraw
+// RUN: %clang_profgen -o %t %s
+// RUN: %t %t.profraw
+// RUN: %t %t.profraw modifyfile
+// RUN: cp %t.profraw %t.profraw.old
+// RUN: %t %t.profraw 2>&1 | FileCheck %s
+// RUN: diff %t.profraw %t.profraw.old
+// CHECK: Invalid profile data to merge
+
+#include <errno.h>
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <unistd.h>
+
+
+void __llvm_profile_set_file_object(FILE* File, int EnableMerge);
+
+void bail(const char* str) {
+  fprintf(stderr, "%s %s\n", str, strerror(errno));
+  exit(1);
+}
+
+int main(int argc, char** argv) {
+  if (argc == 3) {
+    int fd = open(argv[1], O_RDWR);
+    if (fd == -1)
+      bail("open");
+
+    struct stat st;
+    if (stat(argv[1], &st))
+      bail("stat");
+    uint64_t FileSize = st.st_size;
+
+    char* Buf = (char *) mmap(NULL, FileSize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+    if (Buf == MAP_FAILED)
+      bail("mmap");
+
+    // We're trying to make the first CounterPtr invalid.
+    // 10 64-bit words as header.
+    // CounterPtr is the third 64-bit word field.
+    memset(&Buf[10 * 8 + 2 * 8], 0xAB, 8);
+
+    if (munmap(Buf, FileSize))
+      bail("munmap");
+
+    if (close(fd))
+      bail("close");
+  } else {
+    FILE* f = fopen(argv[1], "r+b");
+    if (!f)
+      bail("fopen");
+    __llvm_profile_set_file_object(f, 1);
+  }
+}

compiler-rt/test/profile/Linux/instrprof-merge-vp.c

 // RUN: %clang_profgen -mllvm --enable-value-profiling=true -mllvm -vp-static-alloc=true -mllvm -vp-counters-per-site=3 -O2 -o %t %s
 // RUN: %run %t %t.profraw
 // RUN: llvm-profdata merge -o %t.profdata %t.profraw
 // RUN: llvm-profdata show --all-functions --counts --ic-targets %t.profdata > %t.profdump
 // RUN: FileCheck --input-file %t.profdump  %s --check-prefix=FOO
 // RUN: FileCheck --input-file %t.profdump  %s --check-prefix=BAR
 
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <sys/types.h>
 #include <unistd.h>
 
 int __llvm_profile_runtime = 0;
 int __llvm_profile_write_file();
 void __llvm_profile_reset_counters(void);
-void __llvm_profile_merge_from_buffer(const char *, uint64_t);
+int __llvm_profile_merge_from_buffer(const char *, uint64_t);
 void __llvm_profile_set_filename(const char *);
 struct __llvm_profile_data;
 struct ValueProfData;
 void lprofMergeValueProfData(struct ValueProfData *, struct __llvm_profile_data *);
 /* Force the vp merger module to be linked in.  */
 void *Dummy = &lprofMergeValueProfData;
 
 void callee1() {}

compiler-rt/test/profile/instrprof-merge.c

 // RUN: %clang_profgen -O2 -o %t %s
 // RUN: %run %t %t.profraw 1 1
 // RUN: llvm-profdata show --all-functions --counts %t.profraw  | FileCheck %s
 
 // FIXME: llvm-profdata exits with "Malformed instrumentation profile data"
 // XFAIL: msvc
 
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include "profile_test.h"
 
 int __llvm_profile_runtime = 0;
 uint64_t __llvm_profile_get_size_for_buffer(void);
 int __llvm_profile_write_buffer(char *);
 void __llvm_profile_reset_counters(void);
-void __llvm_profile_merge_from_buffer(const char *, uint64_t);
+int __llvm_profile_merge_from_buffer(const char *, uint64_t);
 
 int dumpBuffer(const char *FileN, const char *Buffer, uint64_t Size) {
   FILE *File = fopen(FileN, "w");
   if (!File)
     return 1;
   if (fwrite(Buffer, 1, Size, File) != Size)
     return 1;
   return fclose(File);

compiler-rt/test/profile/instrprof-without-libc.c

 
 #ifndef CHECK_SYMBOLS
 #include <stdio.h>
 #endif
 
 int __llvm_profile_runtime = 0;
 uint64_t __llvm_profile_get_size_for_buffer(void);
 int __llvm_profile_write_buffer(char *);
-void __llvm_profile_merge_from_buffer(const char *, uint64_t Size);
+int __llvm_profile_merge_from_buffer(const char *, uint64_t Size);
 
 int write_buffer(uint64_t, const char *);
 int main(int argc, const char *argv[]) {
   // CHECK-LABEL: define {{.*}} @main(
   // CHECK: br i1 %{{.*}}, label %{{.*}}, label %{{.*}}, !prof ![[PD1:[0-9]+]]
   if (argc < 2)
     return 1;