From b0c9ca0f0ebd6dd13d0e4a4525c862358c92fee1 Mon Sep 17 00:00:00 2001
From: Marcus Holland-Moritz <mhx@fb.com>
Date: Fri, 13 Jan 2017 13:47:19 -0800
Subject: [PATCH] Protect memcpy calls against undefined behaviour

Summary:
While running a UBSan enabled binary, I got:

  folly/io/IOBuf.cpp:671:15: runtime error: null pointer passed as argument 2, which is declared to never be null

This change protects all calls to memcpy from passing `nullptr`.

Reviewed By: pixelb

Differential Revision: D4415355

fbshipit-source-id: a27ba74244abcca8cd4e106967222890a67f5b6d
---
 folly/io/IOBuf.cpp          | 28 ++++++++++++++++++++--------
 folly/io/test/IOBufTest.cpp | 13 +++++++++++++
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/folly/io/IOBuf.cpp b/folly/io/IOBuf.cpp
index 7a7dd975..caccec13 100644
--- a/folly/io/IOBuf.cpp
+++ b/folly/io/IOBuf.cpp
@@ -210,8 +210,11 @@ IOBuf::IOBuf(CopyBufferOp /* op */,
              uint64_t minTailroom)
     : IOBuf(CREATE, headroom + size + minTailroom) {
   advance(headroom);
-  memcpy(writableData(), buf, size);
-  append(size);
+  if (size > 0) {
+    assert(buf != nullptr);
+    memcpy(writableData(), buf, size);
+    append(size);
+  }
 }
 
 IOBuf::IOBuf(CopyBufferOp op, ByteRange br,
@@ -545,7 +548,10 @@ void IOBuf::unshareOneSlow() {
   // Maintain the same amount of headroom.  Since we maintained the same
   // minimum capacity we also maintain at least the same amount of tailroom.
   uint64_t headlen = headroom();
-  memcpy(buf + headlen, data_, length_);
+  if (length_ > 0) {
+    assert(data_ != nullptr);
+    memcpy(buf + headlen, data_, length_);
+  }
 
   // Release our reference on the old buffer
   decrementRefcount();
@@ -666,10 +672,13 @@ void IOBuf::coalesceAndReallocate(size_t newHeadroom,
   IOBuf* current = this;
   size_t remaining = newLength;
   do {
-    assert(current->length_ <= remaining);
-    remaining -= current->length_;
-    memcpy(p, current->data_, current->length_);
-    p += current->length_;
+    if (current->length_ > 0) {
+      assert(current->length_ <= remaining);
+      assert(current->data_ != nullptr);
+      remaining -= current->length_;
+      memcpy(p, current->data_, current->length_);
+      p += current->length_;
+    }
     current = current->next_;
   } while (current != end);
   assert(remaining == 0);
@@ -810,7 +819,10 @@ void IOBuf::reserveSlow(uint64_t minHeadroom, uint64_t minTailroom) {
       throw std::bad_alloc();
     }
     newBuffer = static_cast<uint8_t*>(p);
-    memcpy(newBuffer + minHeadroom, data_, length_);
+    if (length_ > 0) {
+      assert(data_ != nullptr);
+      memcpy(newBuffer + minHeadroom, data_, length_);
+    }
     if (sharedInfo()) {
       freeExtBuffer();
     }
diff --git a/folly/io/test/IOBufTest.cpp b/folly/io/test/IOBufTest.cpp
index 7f7e55c5..85b9dcb3 100644
--- a/folly/io/test/IOBufTest.cpp
+++ b/folly/io/test/IOBufTest.cpp
@@ -1325,3 +1325,16 @@ TEST(IOBuf, Managed) {
   writableStr(*buf2)[0] = 'x';
   EXPECT_EQ("jelloxorldhelloxorld", toString(*buf1));
 }
+
+TEST(IOBuf, CoalesceEmptyBuffers) {
+  auto b1 = IOBuf::takeOwnership(nullptr, 0);
+  auto b2 = fromStr("hello");
+  auto b3 = IOBuf::takeOwnership(nullptr, 0);
+
+  b2->appendChain(std::move(b3));
+  b1->appendChain(std::move(b2));
+
+  auto br = b1->coalesce();
+
+  EXPECT_TRUE(ByteRange(StringPiece("hello")) == br);
+}
-- 
2.34.1