Commit 7c2ef80c authored by Nicholas Ormrod's avatar Nicholas Ormrod Committed by Facebook Github Bot

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
parent 328c1a56
......@@ -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';
......
......@@ -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);
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment