From: Adam Simpkins <simpkins@fb.com>
Date: Tue, 27 Jun 2017 02:15:05 +0000 (-0700)
Subject: make io::Cursor::push() safe to call with an empty ByteRange
X-Git-Tag: v2017.07.03.00~28
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=46c5dbce6c04234ae0185d3fadb62556c7e7625b;hp=2734e379f411f7d9757f9b8d13c020249a6c2dd6

make io::Cursor::push() safe to call with an empty ByteRange

Summary:
Clang's UndefinedBehaviorSanitizer flagged an issue that pushAtMost() could
call `memcpy(dest, nullptr, 0)` if the input was an empty `ByteRange`.  A
default constructed `ByteRange` (or `StringPiece`) will be empty and both its
begin and end pointers will be null.  Unfortunately it is undefined behavior to
call `memcpy()` with a null source pointer even if the length is 0.

This updates the `Cursor` and `Appender` code to avoid calling `memcpy()` at
all when the input length is 0.

Reviewed By: yfeldblum

Differential Revision: D5322917

fbshipit-source-id: 67fce9579f97e7e93a5767b11cc5e43ff7be5876
---

diff --git a/folly/io/Cursor.h b/folly/io/Cursor.h
index ff9082b0..23186764 100644
--- a/folly/io/Cursor.h
+++ b/folly/io/Cursor.h
@@ -756,6 +756,14 @@ class RWCursor
 
   using detail::Writable<RWCursor<access>>::pushAtMost;
   size_t pushAtMost(const uint8_t* buf, size_t len) {
+    // We have to explicitly check for an input length of 0.
+    // We support buf being nullptr in this case, but we need to avoid calling
+    // memcpy() with a null source pointer, since that is undefined behavior
+    // even if the length is 0.
+    if (len == 0) {
+      return 0;
+    }
+
     size_t copied = 0;
     for (;;) {
       // Fast path: the current buffer is big enough.
@@ -884,6 +892,14 @@ class Appender : public detail::Writable<Appender> {
 
   using detail::Writable<Appender>::pushAtMost;
   size_t pushAtMost(const uint8_t* buf, size_t len) {
+    // We have to explicitly check for an input length of 0.
+    // We support buf being nullptr in this case, but we need to avoid calling
+    // memcpy() with a null source pointer, since that is undefined behavior
+    // even if the length is 0.
+    if (len == 0) {
+      return 0;
+    }
+
     size_t copied = 0;
     for (;;) {
       // Fast path: it all fits in one buffer.
diff --git a/folly/io/test/IOBufCursorTest.cpp b/folly/io/test/IOBufCursorTest.cpp
index cfc5109d..68aa7784 100644
--- a/folly/io/test/IOBufCursorTest.cpp
+++ b/folly/io/test/IOBufCursorTest.cpp
@@ -286,7 +286,6 @@ TEST(IOBuf, pushCursorData) {
   EXPECT_EQ(1, rcursor2.readBE<uint64_t>());
   EXPECT_EQ(10, rcursor2.readBE<uint64_t>());
   EXPECT_EQ(20, rcursor2.readBE<uint32_t>());
-
 }
 
 TEST(IOBuf, Gather) {
@@ -1115,3 +1114,21 @@ TEST(IOBuf, readConsumesAllInputOnFailure) {
   EXPECT_THROW(rcursor.read<uint32_t>(), std::out_of_range);
   EXPECT_EQ(0, rcursor.totalLength());
 }
+
+TEST(IOBuf, pushEmptyByteRange) {
+  // Test pushing an empty ByteRange.  This mainly tests that we do not
+  // trigger UBSAN warnings by calling memcpy() with an null source pointer,
+  // which is undefined behavior even if the length is 0.
+  IOBuf buf{IOBuf::CREATE, 2};
+  ByteRange emptyBytes;
+
+  // Test calling Cursor::push()
+  RWPrivateCursor wcursor(&buf);
+  wcursor.push(emptyBytes);
+  EXPECT_EQ(0, buf.computeChainDataLength());
+
+  // Test calling Appender::push()
+  Appender app(&buf, 16);
+  app.push(emptyBytes);
+  EXPECT_EQ(0, buf.computeChainDataLength());
+}