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 ff9082b056020bab4a756d6dd652e6213e8bda16..23186764da40f631ae59b7f60b5bb9ca1a14cfd7 100644 (file)
--- 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) {
 
   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.
     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) {
 
   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.
     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 cfc5109db81dc0efe4b13419b141d45539abc81f..68aa7784b1cb5d6e2231d152f60a7724bb57b959 100644 (file)
--- 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>());
   EXPECT_EQ(1, rcursor2.readBE<uint64_t>());
   EXPECT_EQ(10, rcursor2.readBE<uint64_t>());
   EXPECT_EQ(20, rcursor2.readBE<uint32_t>());

-

 }
 
 TEST(IOBuf, Gather) {
 }
 
 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());
 }
   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());
+}