Page MenuHomePhabricator

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

Authored by vsk on Feb 22 2021, 4:39 PM.

Details

Summary

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

rdar://74571261

Diff Detail

Event Timeline

vsk requested review of this revision.Feb 22 2021, 4:39 PM
vsk created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2021, 4:39 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vsk updated this revision to Diff 325612.Feb 22 2021, 4:46 PM

Clean up the test by removing the call to set_dumped.

MaskRay added inline comments.Feb 22 2021, 11:15 PM
compiler-rt/lib/profile/InstrProfilingFile.c
775–776

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

vsk added inline comments.Feb 23 2021, 8:32 AM
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 added inline comments.
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 updated this revision to Diff 326209.Feb 24 2021, 2:39 PM

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 accepted this revision.Feb 24 2021, 2:45 PM
This revision is now accepted and ready to land.Feb 24 2021, 2:45 PM
kastiglione added inline comments.Feb 24 2021, 2:46 PM
compiler-rt/lib/profile/InstrProfilingFile.c
707

oh shouldn't these be Idx < Strlen?

vsk added inline comments.Feb 24 2021, 2:49 PM
compiler-rt/lib/profile/InstrProfilingFile.c
707

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

This revision was landed with ongoing or failed builds.Feb 24 2021, 2:49 PM
This revision was automatically updated to reflect the committed changes.
shafik added a subscriber: shafik.Feb 24 2021, 3:04 PM
shafik added inline comments.
compiler-rt/lib/profile/InstrProfilingFile.c
776

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