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

diff --git a/folly/io/IOBuf.cpp b/folly/io/IOBuf.cpp
index 7a7dd9756d21dc2bd342f3bb6f4e72f7de4bfa46..caccec139bcdb1564e0b6cabfc982cb3856d87d7 100644 (file)
--- 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 7f7e55c5346fffa9245ed6d61c68cb118c046243..85b9dcb36cad690ce84339f14a94607b5596bdf5 100644 (file)
--- 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);
+}