r261449 - Lex: Check whether the header map buffer has space for the buckets

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

Author: dexonsmith
Date: Sat Feb 20 15:24:31 2016
New Revision: 261449

URL: http://llvm.org/viewvc/llvm-project?rev=261449&view=rev
Log:
Lex: Check whether the header map buffer has space for the buckets

Check up front whether the header map buffer has space for all of its
declared buckets.

There was already a check in `getBucket()`, but it had UB (comparing
pointers that were outside of objects in the error path) and was
insufficient (only checking for a single byte of the relevant bucket).
I fixed the check, moved it to `checkHeader()`, and left a fixed version
behind as an assertion.

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=261449&r1=261448&r2=261449&view=diff
==============================================================================
--- cfe/trunk/lib/Lex/HeaderMap.cpp (original)
+++ cfe/trunk/lib/Lex/HeaderMap.cpp Sat Feb 20 15:24:31 2016
@@ -83,14 +83,16 @@ bool HeaderMapImpl::checkHeader(const ll
   if (Header->Reserved != 0)
     return false;
 
-  // Check the number of buckets.
+  // Check the number of buckets.  It should be a power of two, and there
+  // should be enough space in the file for all of them.
   auto NumBuckets = NeedsByteSwap
                         ? llvm::sys::getSwappedBytes(Header->NumBuckets)
                         : Header->NumBuckets;
-
-  // If the number of buckets is not a power of two, the headermap is corrupt.
   if (NumBuckets & (NumBuckets - 1))
     return false;
+  if (File.getBufferSize() <
+      sizeof(HMapHeader) + sizeof(HMapBucket) * NumBuckets)
+    return false;
 
   // Okay, everything looks good.
   return true;
@@ -122,21 +124,19 @@ const HMapHeader &HeaderMapImpl::getHead
 /// bswap'ing its fields as appropriate.  If the bucket number is not valid,
 /// this return a bucket with an empty key (0).
 HMapBucket HeaderMapImpl::getBucket(unsigned BucketNo) const {
+  assert(FileBuffer->getBufferSize() >=
+             sizeof(HMapHeader) + sizeof(HMapBucket) * BucketNo &&
+         "Expected bucket to be in range");
+
   HMapBucket Result;
   Result.Key = HMAP_EmptyBucketKey;
 
   const HMapBucket *BucketArray =
     reinterpret_cast<const HMapBucket*>(FileBuffer->getBufferStart() +
                                         sizeof(HMapHeader));
-
   const HMapBucket *BucketPtr = BucketArray+BucketNo;
-  if ((const char*)(BucketPtr+1) > FileBuffer->getBufferEnd()) {
-    Result.Prefix = 0;
-    Result.Suffix = 0;
-    return Result;  // Invalid buffer, corrupt hmap.
-  }
 
-  // Otherwise, the bucket is valid.  Load the values, bswapping as needed.
+  // Load the values, bswapping as needed.
   Result.Key    = getEndianAdjustedWord(BucketPtr->Key);
   Result.Prefix = getEndianAdjustedWord(BucketPtr->Prefix);
   Result.Suffix = getEndianAdjustedWord(BucketPtr->Suffix);

Modified: cfe/trunk/unittests/Lex/HeaderMapTest.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Lex/HeaderMapTest.cpp?rev=261449&r1=261448&r2=261449&view=diff
==============================================================================
--- cfe/trunk/unittests/Lex/HeaderMapTest.cpp (original)
+++ cfe/trunk/unittests/Lex/HeaderMapTest.cpp Sat Feb 20 15:24:31 2016
@@ -100,4 +100,12 @@ TEST(HeaderMapTest, checkHeader3Buckets)
   ASSERT_FALSE(HeaderMapImpl::checkHeader(*File.getBuffer(), NeedsSwap));
 }
 
+TEST(HeaderMapTest, checkHeaderNotEnoughBuckets) {
+  MapFile<1, 1> File;
+  File.init();
+  File.Header.NumBuckets = 8;
+  bool NeedsSwap;
+  ASSERT_FALSE(HeaderMapImpl::checkHeader(*File.getBuffer(), NeedsSwap));
+}
+
 } // end namespace