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

[Dave Lee via Phabricator via llvm-commits]

kastiglione added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:740
+      ++I; /* Advance to the next character. */
+      if (getChar(FilenamePat, I, FilenamePatLen) == 'p') {
         if (!NumPids++) {
----------------
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.


================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:773
         __llvm_profile_set_page_size(getpagesize());
         __llvm_profile_enable_continuous_mode();
       } else {
----------------
vsk wrote:
> MaskRay wrote:
> > Adding `getChar` seems excessive. Does simply dropping `I++` here fix the bug?
> Yes. `getChar` adds a little complexity, but lets us write a test that reliably fails pre-patch. I think that's worth it.
So adding `getChar`, but not removing the `I++` causes test failure. And then leaving `getChar` also protects against future changes to the loop body?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97239/new/

https://reviews.llvm.org/D97239