Page MenuHomePhabricator

Don't require a null terminator when loading objects

Authored by glandium on Jan 8 2019, 9:37 PM.



Requiring one doesn't cause harm in most cases, but when things align perfectly, things can go really bad.

Imagine you have a browser, built with ASAN. It uses multiple processes. It has intentional leaks, with suppressions. You run it, exit it. Each child process exits. Each with leaks, that need symbolication to match against the suppressions. So each runs llvm-symbolizer. At the same time.

Some of the addresses to symbolicate are in a shared library. That shared library contains all DWARF info, so it's rather large (close to a 1GB). Oh, and because all stars were aligned, its size is exactly a multiple of the page size. So shouldUseMmap in MemoryBuffer returns false for it. So llvm-symbolizer pread() the file instead. All instances of it. Did I say there are multiple processes? So suddenly you have n processes simultaneously allocating and filling 1GB of memory each, on CI machines that have enough memory for the job they usually run, but not enough for a sudden rush of n GB.

And things go awry. When you're lucky and the OOM killer didn't take care of killing the CI entirely, symbolication couldn't happen and the suppressions are not matched, and leaks are reported.

Diff Detail


Event Timeline

glandium created this revision.Jan 8 2019, 9:37 PM
zturner accepted this revision.Jan 9 2019, 12:50 PM

Thanks. I fixed this exact bug in a different place a month ago. I wonder if we should audit all callers of getFileOrSTDIN and MemoryBuffer::getFile, because I'm guessing this can still occur in other places for the same reason.

This revision is now accepted and ready to land.Jan 9 2019, 12:50 PM
This revision was automatically updated to reflect the committed changes.