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

[jkorous]

Summary

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

[arphaman]

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

clang/lib/Basic/SourceManager.cpp

1254–1257

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]

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

[jkorous]

calculate the size of the buffer upfront

[arphaman]

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.

[jkorous]

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

[jkorous]

test

[jkorous]

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

[jkorous]

(and a delete[])

[jkorous]

polish test

[arphaman]

LGTM, with a nit mentioned.

clang/unittests/Basic/SourceManagerTest.cpp

249

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

clang/lib/Basic/SourceManager.cpp

static void ComputeLineNumbers(DiagnosticsEngine &Diag, ContentCache *FI,
   SmallVector<unsigned, 256> LineOffsets;
 
   // Line #1 starts at char 0.
   LineOffsets.push_back(0);
 
   const unsigned char *Buf = (const unsigned char *)Buffer->getBufferStart();
   const unsigned char *End = (const unsigned char *)Buffer->getBufferEnd();
   unsigned I = 0;
-  while (true) {
-    // Skip over the contents of the line.
-    while (Buf[I] != '\n' && Buf[I] != '\r' && Buf[I] != '\0')
-      ++I;
-
-    if (Buf[I] == '\n' || Buf[I] == '\r') {
+  while (Buf + I < End) {
+    if (Buf[I] == '\n') {
+      LineOffsets.push_back(I + 1);
+    } else if (Buf[I] == '\r') {
       // If this is \r\n, skip both characters.
-      if (Buf[I] == '\r' && Buf[I+1] == '\n')
-        ++I;
-      ++I;
-      LineOffsets.push_back(I);
-    } else {
-      // Otherwise, this is a NUL. If end of file, exit.
-      if (Buf+I == End) break;
+      if (Buf + I + 1 < End && Buf[I + 1] == '\n')
         ++I;
+      LineOffsets.push_back(I + 1);
     }
+    ++I;
   }
 
   // Copy the offsets into the FileInfo structure.
   FI->NumLines = LineOffsets.size();
   FI->SourceLineCache = Alloc.Allocate<unsigned>(LineOffsets.size());
   std::copy(LineOffsets.begin(), LineOffsets.end(), FI->SourceLineCache);
 }
 

clang/unittests/Basic/SourceManagerTest.cpp

 #include "clang/Basic/TargetOptions.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/ModuleLoader.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/Process.h"
 #include "gtest/gtest.h"
+#include <cstddef>
 
 using namespace clang;
 
 namespace {
 
 // The test fixture.
 class SourceManagerTest : public ::testing::Test {
 protected:
ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
                     "\x00\x00\xFE\xFF#include <iostream>"))),
             "UTF-32 (BE)");
   ASSERT_EQ(StringRef(SrcMgr::ContentCache::getInvalidBOM(
                 llvm::StringLiteral::withInnerNUL(
                     "\xFF\xFE\x00\x00#include <iostream>"))),
             "UTF-32 (LE)");
 }
 
+
+TEST_F(SourceManagerTest, getLineNumber) {
+  const unsigned pageSize = llvm::sys::Process::getPageSizeEstimate();
+  char* source = new char[pageSize];
+  for(unsigned i = 0; i < pageSize; ++i) {
+    source[i] = 'a';
+  }
+
+  std::unique_ptr<llvm::MemoryBuffer> Buf =
+      llvm::MemoryBuffer::getMemBuffer(
+        llvm::MemoryBufferRef(
+          llvm::StringRef(source, 3), "whatever"
+        ),
+        false
+      );
+
+  FileID mainFileID = SourceMgr.createFileID(std::move(Buf));
+  SourceMgr.setMainFileID(mainFileID);
+
+  SourceMgr.getLineNumber(mainFileID, 1, nullptr);
+}
+
 #if defined(LLVM_ON_UNIX)
 
 TEST_F(SourceManagerTest, getMacroArgExpandedLocation) {
   const char *header =
     "#define FM(x,y) x\n";
 
   const char *main =
     "#include \"/test-header.h\"\n"