fix a few bugs in FBString

Summary:
* `push_back()` fails for medium strings of zero capacity if not linked
with jemalloc (see added test),
* we incorrectly initiliaze capacity when acquire mallocated string
(forgot about null terminator).

Test Plan: fbconfig --allocator=malloc folly/test:fbstring_test_using_jemalloc && fbmake runtests_opt

@override-unit-failures

Reviewed By: tudorb@fb.com

FB internal diff: D1012196

diff --git a/folly/FBString.h b/folly/FBString.h
index bde4e73501dbc681964269745e44d2f3f293fac3..f1da428dea23b602a09ea2e3a0a93435ec197d15 100644 (file)
--- a/folly/FBString.h
+++ b/folly/FBString.h
@@ -445,22 +445,24 @@ public:
   }
 
   // Snatches a previously mallocated string. The parameter "size"
-  // is the size of the string, and the parameter "capacity" is the size
-  // of the mallocated block.  The string must be \0-terminated, so
-  // data[size] == '\0' and capacity >= size + 1.
+  // is the size of the string, and the parameter "allocatedSize"
+  // is the size of the mallocated block.  The string must be
+  // \0-terminated, so allocatedSize >= size + 1 and data[size] == '\0'.
   //
-  // So if you want a 2-character string, pass malloc(3) as "data", pass 2 as
-  // "size", and pass 3 as "capacity".
-  fbstring_core(Char *const data, const size_t size,
-                const size_t capacity,
+  // So if you want a 2-character string, pass malloc(3) as "data",
+  // pass 2 as "size", and pass 3 as "allocatedSize".
+  fbstring_core(Char * const data,
+                const size_t size,
+                const size_t allocatedSize,
                 AcquireMallocatedString) {
     if (size > 0) {
-      assert(capacity > size);
+      assert(allocatedSize >= size + 1);
       assert(data[size] == '\0');
       // Use the medium string storage
       ml_.data_ = data;
       ml_.size_ = size;
-      ml_.capacity_ = capacity | isMedium;
+      // Don't forget about null terminator
+      ml_.capacity_ = (allocatedSize - 1) | isMedium;
     } else {
       // No need for the memory
       free(data);
@@ -604,7 +606,7 @@ public:
           smartRealloc(
             ml_.data_,
             ml_.size_ * sizeof(Char),
-            ml_.capacity() * sizeof(Char),
+            (ml_.capacity() + 1) * sizeof(Char),
             capacityBytes));
         writeTerminator();
         ml_.capacity_ = (capacityBytes / sizeof(Char) - 1) | isMedium;
@@ -696,7 +698,7 @@ public:
     } else {
       sz = ml_.size_;
       if (sz == capacity()) {  // always true for isShared()
-        reserve(sz * 3 / 2);  // ensures not shared
+        reserve(1 + sz * 3 / 2);  // ensures not shared
       }
     }
     assert(!isShared());

diff --git a/folly/test/FBStringTest.cpp b/folly/test/FBStringTest.cpp
index 6cd5d1fa559c3c394105c680ca1b2838bea94b20..3e294282449be887f643b9f25ed763bc6f1db6ad 100644 (file)
--- a/folly/test/FBStringTest.cpp
+++ b/folly/test/FBStringTest.cpp
@@ -1130,9 +1130,15 @@ TEST(FBString, testFixedBugs) {
     std::swap(str, str);
     EXPECT_EQ(1337, str.size());
   }
+  { // D1012196, --allocator=malloc
+    fbstring str(128, 'f');
+    str.clear();  // Empty medium string.
+    fbstring copy(str);  // Medium string of 0 capacity.
+    copy.push_back('b');
+    EXPECT_GE(copy.capacity(), 1);
+  }
 }
 
-
 TEST(FBString, testHash) {
   fbstring a;
   fbstring b;