Don't require a null terminator when loading objects

[glandium]

Summary

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.

[zturner]

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.

lib/Object/Binary.cpp

 
 Expected<OwningBinary<Binary>> object::createBinary(StringRef Path) {
   ErrorOr<std::unique_ptr<MemoryBuffer>> FileOrErr =
-      MemoryBuffer::getFileOrSTDIN(Path);
+      MemoryBuffer::getFileOrSTDIN(Path, /*FileSize=*/-1,
+                                   /*RequiresNullTerminator=*/false);
   if (std::error_code EC = FileOrErr.getError())
     return errorCodeToError(EC);
   std::unique_ptr<MemoryBuffer> &Buffer = FileOrErr.get();