folly/GroupVarint: fix a clang-caught heap-buffer-overrun

Summary:
Avoid a clang-caught heap-buffer-overrun
and document that the ssse3 decoder requires at least 17 bytes
of readable memory starting at the source pointer.
* folly/GroupVarint.h (decode_simple): Comment out dead code.
(decode)[__SSSE3__]: Add a comment describing this limitation.
* folly/test/GroupVarintTest.cpp (testGroupVarint32):
Include <algorithm> for use of std::max.
Ensure that the buffer we use has length at least 17.

Test Plan:
Now, all tests succeed with clang/asan

Reviewed By: simpkins@fb.com

Subscribers: trunkagent, mathieubaudet, njormrod

FB internal diff: D1579109

Tasks: 5241437

diff --git a/folly/GroupVarint.h b/folly/GroupVarint.h
index 0bdf36975018fcf7351dd81f7746f91aa2ff4ddb..ff1c33c385da09eb4d5b61c9f617899a4cb4ea30 100644 (file)
--- a/folly/GroupVarint.h
+++ b/folly/GroupVarint.h
@@ -176,7 +176,7 @@ class GroupVarint<uint32_t> : public detail::GroupVarintBase<uint32_t> {
     p += k2+1;
     size_t k3 = b3key(k);
     *d = loadUnaligned<uint32_t>(p) & kMask[k3];
-    p += k3+1;
+    // p += k3+1;
     return end;
   }
 
@@ -189,6 +189,10 @@ class GroupVarint<uint32_t> : public detail::GroupVarintBase<uint32_t> {
   }
 
 #ifdef __SSSE3__
+  /**
+   * Just like the non-SSSE3 decode below, but with the additional constraint
+   * that we must be able to read at least 17 bytes from the input pointer, p.
+   */
   static const char* decode(const char* p, uint32_t* dest) {
     uint8_t key = p[0];
     __m128i val = _mm_loadu_si128((const __m128i*)(p+1));
@@ -198,6 +202,10 @@ class GroupVarint<uint32_t> : public detail::GroupVarintBase<uint32_t> {
     return p + detail::groupVarintLengths[key];
   }
 
+  /**
+   * Just like decode_simple, but with the additional constraint that
+   * we must be able to read at least 17 bytes from the input pointer, p.
+   */
   static const char* decode(const char* p, uint32_t* a, uint32_t* b,
                             uint32_t* c, uint32_t* d) {
     uint8_t key = p[0];

diff --git a/folly/test/GroupVarintTest.cpp b/folly/test/GroupVarintTest.cpp
index 9fb279b267eaa7c73eaa17f67a789b9a602ff0ac..8d9909da51c21c6fb198ceb473b1f0a9224f070a 100644 (file)
--- a/folly/test/GroupVarintTest.cpp
+++ b/folly/test/GroupVarintTest.cpp
@@ -15,6 +15,7 @@
  */
 
 #include <stdarg.h>
+#include <algorithm>
 #include <folly/GroupVarint.h>
 
 // On platforms where it's not supported, GroupVarint will be compiled out.
@@ -56,7 +57,10 @@ void testGroupVarint32(uint32_t a, uint32_t b, uint32_t c, uint32_t d, ...) {
   EXPECT_EQ(expectedBytes.size(), size);
 
   std::vector<char> foundBytes;
-  foundBytes.resize(size + 4);
+
+  // ssse3 decoding requires that the source buffer have length >= 17,
+  // so that it can read 128 bits from &start[1] via _mm_loadu_si128.
+  foundBytes.resize(std::max(size + 4, 17UL));
   char* start = &(foundBytes.front());
   char* p = GroupVarint32::encode(start, a, b, c, d);
   EXPECT_EQ((void*)(start + size), (void*)p);