[profile] Fix buffer overrun when parsing %c in filename string

[vsk]

Summary

Fix a buffer overrun that can occur when parsing '%c' at the end of a
filename pattern string.

rdar://74571261

[vsk]

Clean up the test by removing the call to set_dumped.

[MaskRay]

compiler-rt/lib/profile/InstrProfilingFile.c

775–776

Adding getChar seems excessive. Does simply dropping I++ here fix the bug?

[vsk]

compiler-rt/lib/profile/InstrProfilingFile.c

775–776

Yes. getChar adds a little complexity, but lets us write a test that reliably fails pre-patch. I think that's worth it.

[kastiglione]

compiler-rt/lib/profile/InstrProfilingFile.c

740

It might be nice to have reading a char be consistent across the cases. This first one uses getChar(…), but the others use FilenamePat[I]. One option is a function like bool checkBounds(…), then the for loop could have:

for (I = 0; checkBounds(I, FilenamePatLen) && FilenamePat[I]; ++I)

and then here:

++I; /* Advance to the next character. */
if (!checkBounds(I, FilenamePatLen))
  break;
if (FilenamePat[I] == 'p') {

Another option is to keep getChar and save its result to a variable, which each of the cases reuse rather than directly reading from the array. Given that these are case-like, this could be structured as single read by doing switch (getChar(…)) and then having each read be a case 'c': etc.

775–776

So adding getChar, but not removing the I++ causes test failure. And then leaving getChar also protects against future changes to the loop body?

[vsk]

Address review feedback. I opted to add the checkBounds helper, as this seemed like the least invasive option. The convention in the rest of this library appears to be to not use stdbool.h, so I decided not to pull it in here.

compiler-rt/lib/profile/InstrProfilingFile.c

775–776

Yes, that's right. The checkBounds helper should serve the same role.

[kastiglione]

compiler-rt/lib/profile/InstrProfilingFile.c

707

oh shouldn't these be Idx < Strlen?

[vsk]

compiler-rt/lib/profile/InstrProfilingFile.c

707

Followed up offline but to recap: no, we dereference the null byte at the end.

[shafik]

compiler-rt/lib/profile/InstrProfilingFile.c

776

Obviously not for this PR but it seems unwise we don't actually verify FilenamePat[I] == 'm'

compiler-rt/lib/profile/InstrProfilingFile.c

 /*===- InstrProfilingFile.c - Write instrumentation to a file -------------===*\
 |*
 |* Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 |* See https://llvm.org/LICENSE.txt for license information.
 |* SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 |*
 \*===----------------------------------------------------------------------===*/
 
 #if !defined(__Fuchsia__)
 
+#include <assert.h>
 #include <errno.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #ifdef _MSC_VER
 /* For _alloca. */
 #include <malloc.h>
 #endif
for (;; ++J) {
     Num = Num * 10 + C - '0';
 
     /* If FilenamePat[*I+J] is between '0' and '9', the next byte is guaranteed
      * to be in-bound as the string is null terminated. */
   }
   return 0;
 }
 
+/* Assert that Idx does index past a string null terminator. Return the
+ * result of the check. */
+static int checkBounds(int Idx, int Strlen) {
+  assert(Idx <= Strlen && "Indexing past string null terminator");
+  return Idx <= Strlen;
+}
+
 /* Parses the pattern string \p FilenamePat and stores the result to
  * lprofcurFilename structure. */
 static int parseFilenamePattern(const char *FilenamePat,
                                 unsigned CopyFilenamePat) {
   int NumPids = 0, NumHosts = 0, I;
   char *PidChars = &lprofCurFilename.PidChars[0];
   char *Hostname = &lprofCurFilename.Hostname[0];
   int MergingEnabled = 0;
+  int FilenamePatLen = strlen(FilenamePat);
 
   /* Clean up cached prefix and filename.  */
   if (lprofCurFilename.ProfilePathPrefix)
     free((void *)lprofCurFilename.ProfilePathPrefix);
 
   if (lprofCurFilename.FilenamePat && lprofCurFilename.OwnsFilenamePat) {
     free((void *)lprofCurFilename.FilenamePat);
   }
 
   memset(&lprofCurFilename, 0, sizeof(lprofCurFilename));
 
   if (!CopyFilenamePat)
     lprofCurFilename.FilenamePat = FilenamePat;
   else {
     lprofCurFilename.FilenamePat = strdup(FilenamePat);
     lprofCurFilename.OwnsFilenamePat = 1;
   }
   /* Check the filename for "%p", which indicates a pid-substitution. */
-  for (I = 0; FilenamePat[I]; ++I)
+  for (I = 0; checkBounds(I, FilenamePatLen) && FilenamePat[I]; ++I) {
     if (FilenamePat[I] == '%') {
-      if (FilenamePat[++I] == 'p') {
+      ++I; /* Advance to the next character. */
+      if (!checkBounds(I, FilenamePatLen))
+        break;
+      if (FilenamePat[I] == 'p') {
         if (!NumPids++) {
           if (snprintf(PidChars, MAX_PID_SIZE, "%ld", (long)getpid()) <= 0) {
             PROF_WARN("Unable to get pid for filename pattern %s. Using the "
                       "default name.",
                       FilenamePat);
             return -1;
           }
         }
if (FilenamePat[I] == '%') {
       } else if (FilenamePat[I] == 'c') {
         if (__llvm_profile_is_continuous_mode_enabled()) {
           PROF_WARN("%%c specifier can only be specified once in %s.\n",
                     FilenamePat);
           return -1;
         }
 
         __llvm_profile_set_page_size(getpagesize());
         __llvm_profile_enable_continuous_mode();
-        I++; /* advance to 'c' */
       } else {
         unsigned MergePoolSize = getMergePoolSize(FilenamePat, &I);
         if (!MergePoolSize)
           continue;
         if (MergingEnabled) {
           PROF_WARN("%%m specifier can only be specified once in %s.\n",
                     FilenamePat);
           return -1;
         }
         MergingEnabled = 1;
         lprofCurFilename.MergePoolSize = MergePoolSize;
       }
     }
+  }
 
   lprofCurFilename.NumPids = NumPids;
   lprofCurFilename.NumHosts = NumHosts;
   return 0;
 }
 
 static void parseAndSetFilename(const char *FilenamePat,
                                 ProfileNameSpecifier PNS,

compiler-rt/test/profile/ContinuousSyncMode/get-filename.c

+// REQUIRES: darwin
+
+// RUN: %clang_pgogen -o %t.exe %s
+// RUN: env LLVM_PROFILE_FILE="%c%t.profraw" %run %t.exe %t.profraw
+// RUN: env LLVM_PROFILE_FILE="%t%c.profraw" %run %t.exe %t.profraw
+// RUN: env LLVM_PROFILE_FILE="%t.profraw%c" %run %t.exe %t.profraw
+
+#include <string.h>
+#include <stdio.h>
+
+extern int __llvm_profile_is_continuous_mode_enabled(void);
+extern const char *__llvm_profile_get_filename();
+extern void __llvm_profile_set_dumped(void);
+
+int main(int argc, char **argv) {
+  if (!__llvm_profile_is_continuous_mode_enabled())
+    return 1;
+
+  // Check that the filename is "%t.profraw", followed by a null terminator.
+  size_t n = strlen(argv[1]) + 1;
+  const char *Filename = __llvm_profile_get_filename();
+
+  for (int i = 0; i < n; ++i) {
+    if (Filename[i] != argv[1][i]) {
+      printf("Difference at: %d, Got: %c, Expected: %c\n", i, Filename[i], argv[1][i]);
+      printf("Got: %s\n", Filename);
+      printf("Expected: %s\n", argv[1]);
+      return 1;
+    }
+  }
+  return 0;
+}