From 7c2ef80cad4449b84a0eb2ee147c818b76256f73 Mon Sep 17 00:00:00 2001 From: Nicholas Ormrod Date: Mon, 10 Oct 2016 15:45:25 -0700 Subject: [PATCH] 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 --- folly/FBString.h | 11 +++++++++-- folly/test/FBStringTest.cpp | 7 +++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/folly/FBString.h b/folly/FBString.h index 64249c28..00d1a959 100644 --- a/folly/FBString.h +++ b/folly/FBString.h @@ -166,6 +166,9 @@ inline void podFill(Pod* b, Pod* e, T c) { */ template 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::initMedium( // allocate one extra Char for the terminating null. auto const allocSize = goodMallocSize((1 + size) * sizeof(Char)); ml_.data_ = static_cast(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 a31038f7..fb4a7991 100644 --- 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); +} -- 2.34.1