Remove memcpy-UB in fbstring

Summary:
The bug report in t13764686 shows that `folly::StringPiece().str()` causes undefined behavior by passing nullptr into memcpy: the default-initialized StringPiece has two null pointers, which will call `string(char*, size)` with nullptr and 0. This gets forwarded to `podCopy` in fbstring, which calls memcpy and has undefined behavior.

The call pattern `string(nullptr, 0)` is not illegal (note: that syntax is ambiguous, as the literal `0` can be converted to a pointer). Prevent this legal call pattern from causing undefined behavior in fbstring.

This diff puts asserts in podCopy to catch errors, and ensures that current podCopy callsites ensure that the pointer is non-null.

Most podCopy callsites do not need to guarded, since the size is guaranteed to be greater than 0 (which will hard crash instead). The interesting callsite is in `initMedium`, because there is a mode (disableSSO) which will allocate empty strings on the heap, so we need to add a guard there.

Reviewed By: luciang

Differential Revision: D3996440

fbshipit-source-id: b311a311973d1d969542245c72035c5b38da58e3

diff --git a/folly/FBString.h b/folly/FBString.h
index 64249c2845408f390935280ade400a49405cbb43..00d1a959978cc6df28ed2d74ba75ab7a4585f568 100644 (file)
--- a/folly/FBString.h
+++ b/folly/FBString.h
@@ -166,6 +166,9 @@ inline void podFill(Pod* b, Pod* e, T c) {
  */
 template <class Pod>
 inline void podCopy(const Pod* b, const Pod* e, Pod* d) {
+  FBSTRING_ASSERT(b != nullptr);
+  FBSTRING_ASSERT(e != nullptr);
+  FBSTRING_ASSERT(d != nullptr);
   FBSTRING_ASSERT(e >= b);
   FBSTRING_ASSERT(d >= e || d + (e - b) <= b);
   memcpy(d, b, (e - b) * sizeof(Pod));
@@ -554,7 +557,9 @@ private:
     static RefCounted * create(const Char * data, size_t * size) {
       const size_t effectiveSize = *size;
       auto result = create(size);
-      fbstring_detail::podCopy(data, data + effectiveSize, result->data_);
+      if (FBSTRING_LIKELY(effectiveSize > 0)) {
+        fbstring_detail::podCopy(data, data + effectiveSize, result->data_);
+      }
       return result;
     }
 
@@ -761,7 +766,9 @@ FOLLY_MALLOC_NOINLINE inline void fbstring_core<Char>::initMedium(
   // allocate one extra Char for the terminating null.
   auto const allocSize = goodMallocSize((1 + size) * sizeof(Char));
   ml_.data_ = static_cast<Char*>(checkedMalloc(allocSize));
-  fbstring_detail::podCopy(data, data + size, ml_.data_);
+  if (FBSTRING_LIKELY(size > 0)) {
+    fbstring_detail::podCopy(data, data + size, ml_.data_);
+  }
   ml_.size_ = size;
   ml_.setCapacity(allocSize / sizeof(Char) - 1, Category::isMedium);
   ml_.data_[size] = '\0';

diff --git a/folly/test/FBStringTest.cpp b/folly/test/FBStringTest.cpp
index a31038f7c5d3710735d584ca788a3cba70568b46..fb4a79912f044edeb303d2abbd653f07bc7cc224 100644 (file)
--- a/folly/test/FBStringTest.cpp
+++ b/folly/test/FBStringTest.cpp
@@ -1419,3 +1419,10 @@ TEST(FBStringCtorTest, DefaultInitStructAlloc) {
   EXPECT_TRUE(t2.stringMember.empty());
   EXPECT_EQ(allocatorConstructedCount.load(), 1);
 }
+
+TEST(FBStringCtorTest, NullZeroConstruction) {
+  char* p = nullptr;
+  int n = 0;
+  folly::fbstring f(p, n);
+  EXPECT_EQ(f.size(), 0);
+}