Page MenuHomePhabricator

[clang] Fix out-of-bounds memory access in ComputeLineNumbers

Authored by jkorous on Jan 8 2020, 12:38 PM.



There was an access past the end of buffer for buffers not terminated by zero.

Diff Detail

Event Timeline

jkorous created this revision.Jan 8 2020, 12:38 PM

Did you check if you can reproduce it with a unittest by passing in a Buffer that's not nil-terminated?


It might be better to check if I is <. than the size of the buffer here and down below to avoid extra pointer arithmetic.

jkorous marked an inline comment as done.Jan 8 2020, 4:11 PM

I tried but couldn't reproduce a segfault. Do you have any suggestion on how to reasonably reliably (TM) reproduce it?

jkorous updated this revision to Diff 236930.Jan 8 2020, 4:12 PM

calculate the size of the buffer upfront

It might be hard to test in a regular build: you probably would need to construct a test-case where the buffer is precisely a multiple of the page size and not nil-terminated, and then hopefully the OS will trigger a fault once you access the out of bounds character. Have you tried ASANified build? I think it could trigger a crash even if you don't get the pages right and just have a not-nil terminated buffer.

Ok, seems such test fails reliably (on macOS).

Oh, I will just add some ASSERT to the test and a comment on what it actually tests.

(and a delete[])

jkorous updated this revision to Diff 237219.Jan 9 2020, 3:18 PM
jkorous retitled this revision from [clang] Fix segfault in ComputeLineNumbers to [clang] Fix out-of-bounds memory access in ComputeLineNumbers.
jkorous edited the summary of this revision. (Show Details)

polish test

arphaman accepted this revision.Jan 10 2020, 10:42 AM

LGTM, with a nit mentioned.

249 ↗(On Diff #237219)

Nit: you can use std::unique_ptr<char[]> to avoid the manual delete[] below.

This revision is now accepted and ready to land.Jan 10 2020, 10:42 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 10 2020, 11:27 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript