r261461 - Lex: Never overflow the file in HeaderMap::lookupFilename()

[Duncan P. N. Exon Smith via cfe-commits]

Author: dexonsmith
Date: Sat Feb 20 18:14:36 2016
New Revision: 261461

URL: http://llvm.org/viewvc/llvm-project?rev=261461&view=rev
Log:
Lex: Never overflow the file in HeaderMap::lookupFilename()

If a header map file is corrupt, the strings in the string table may not
be null-terminated.  The logic here previously relied on `MemoryBuffer`
always being null-terminated, but this isn't actually guaranteed by the
class AFAICT.  Moreover, we're seeing a lot of crash traces at calls to
`strlen()` inside of `lookupFilename()`, so something is going wrong
there.

Instead, use `strnlen()` to get the length, and check for corruption.

Also remove code paths that could call `StringRef(nullptr)`.  r261459
made these rather obvious (although they'd been there all along).

Modified:
    cfe/trunk/lib/Lex/HeaderMap.cpp
    cfe/trunk/unittests/Lex/HeaderMapTest.cpp

Modified: cfe/trunk/lib/Lex/HeaderMap.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/HeaderMap.cpp?rev=261461&r1=261460&r2=261461&view=diff
==============================================================================
--- cfe/trunk/lib/Lex/HeaderMap.cpp (original)
+++ cfe/trunk/lib/Lex/HeaderMap.cpp Sat Feb 20 18:14:36 2016
@@ -21,6 +21,7 @@
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/SwapByteOrder.h"
 #include "llvm/Support/Debug.h"
+#include <cstring>
 #include <memory>
 using namespace clang;
 
@@ -151,12 +152,17 @@ StringRef HeaderMapImpl::getString(unsig
 
   // Check for invalid index.
   if (StrTabIdx >= FileBuffer->getBufferSize())
-    return nullptr;
+    return "";
+
+  const char *Data = FileBuffer->getBufferStart() + StrTabIdx;
+  unsigned MaxLen = FileBuffer->getBufferSize() - StrTabIdx;
+  unsigned Len = strnlen(Data, MaxLen);
+
+  // Check whether the buffer is null-terminated.
+  if (Len == MaxLen && Data[Len - 1])
+    return "";
 
-  // Otherwise, we have a valid pointer into the file.  Just return it.  We know
-  // that the "string" can not overrun the end of the file, because the buffer
-  // is nul terminated by virtue of being a MemoryBuffer.
-  return FileBuffer->getBufferStart()+StrTabIdx;
+  return StringRef(Data, Len);
 }
 
 //===----------------------------------------------------------------------===//

Modified: cfe/trunk/unittests/Lex/HeaderMapTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/HeaderMapTest.cpp?rev=261461&r1=261460&r2=261461&view=diff
==============================================================================
--- cfe/trunk/unittests/Lex/HeaderMapTest.cpp (original)
+++ cfe/trunk/unittests/Lex/HeaderMapTest.cpp Sat Feb 20 18:14:36 2016
@@ -14,6 +14,7 @@
 #include "llvm/Support/SwapByteOrder.h"
 #include "gtest/gtest.h"
 #include <cassert>
+#include <type_traits>
 
 using namespace clang;
 using namespace llvm;
@@ -170,4 +171,45 @@ TEST(HeaderMapTest, lookupFilename) {
   ASSERT_EQ("bc", Map.lookupFilename("a", DestPath));
 }
 
+template <class FileTy, class PaddingTy> struct PaddedFile {
+  FileTy File;
+  PaddingTy Padding;
+};
+
+TEST(HeaderMapTest, lookupFilenameTruncated) {
+  typedef MapFile<2, 64 - sizeof(HMapHeader) - 2 * sizeof(HMapBucket)> FileTy;
+  static_assert(std::is_standard_layout<FileTy>::value,
+                "Expected standard layout");
+  static_assert(sizeof(FileTy) == 64, "check the math");
+  PaddedFile<FileTy, uint64_t> P;
+  auto &File = P.File;
+  auto &Padding = P.Padding;
+  File.init();
+
+  FileMaker<FileTy> Maker(File);
+  auto a = Maker.addString("a");
+  auto b = Maker.addString("b");
+  auto c = Maker.addString("c");
+  Maker.addBucket(getHash("a"), a, b, c);
+
+  // Add 'x' characters to cause an overflow into Padding.
+  ASSERT_EQ('c', File.Bytes[5]);
+  for (unsigned I = 6; I < sizeof(File.Bytes); ++I) {
+    ASSERT_EQ(0, File.Bytes[I]);
+    File.Bytes[I] = 'x';
+  }
+  Padding = 0xffffffff; // Padding won't stop it either.
+
+  bool NeedsSwap;
+  ASSERT_TRUE(HeaderMapImpl::checkHeader(*File.getBuffer(), NeedsSwap));
+  ASSERT_FALSE(NeedsSwap);
+  HeaderMapImpl Map(File.getBuffer(), NeedsSwap);
+
+  // The string for "c" runs to the end of File.  Check that the suffix
+  // ("cxxxx...") is ignored.  Another option would be to return an empty
+  // filename altogether.
+  SmallString<24> DestPath;
+  ASSERT_EQ("b", Map.lookupFilename("a", DestPath));
+}
+
 } // end namespace