Fix some memory issues with ctl::string (#1201)

There were a few errors in how capacity and memory was being handled for
small strings. The capacity errors meant that small strings would become
big strings too soon, and the memory error introduced undefined behavior
that was caught by CheckMemoryLeaks in our test file but only sometimes.

The crucial change is in reserve: we only copy n bytes into p2, and then
we manually set the null terminator instead of expecting it to have been
there already. (E.g. it might not be there for an empty small string.)

We also fix one other doozy in append when we were exactly at the small-
to-big string boundary: we set the last byte (i.e., the remainder field)
to 0, then decremented it, giving us size_t max. Whoops. We boneheadedly
fix this by setting the 0 byte after we've fixed up the remainder, so it
is at worst a no-op.

Otherwise, capacity now works the same for small strings as it does with
big strings: it's the amount of space available including the null byte.

We test all of this with a new test that only gets included if our class
under test is not std::string (presumably meaning it's ctl::string.) The
test manually verifies that the small string optimization behaves how we
expect.

Since this test checks against std::string, we go ahead and include that
other header from the STL.

Also modifies the new test we introduced to also run on std::string, but
it just does the append without expecting anything about how its data is
stored. We also check that the string has the right value afterwards.

Author: mrdomino

ctl/string.cc

@@ -99,7 +99,8 @@ string::reserve(size_t c2) noexcept
     if (!isbig()) {
         if (!(p2 = (char*)malloc(c2)))
             __builtin_trap();
-        memcpy(p2, data(), size() + 1);
+        memcpy(p2, data(), size());
+        p2[size()] = 0;
     } else {
         if (!(p2 = (char*)realloc(big()->p, c2)))
             __builtin_trap();
@@ -134,18 +135,18 @@ string::append(char ch) noexcept
     if (ckd_add(&n2, size(), 2))
         __builtin_trap();
     if (n2 > capacity()) {
-        size_t c2 = capacity() + 2;
+        size_t c2 = capacity();
         if (ckd_add(&c2, c2, c2 >> 1))
             __builtin_trap();
         reserve(c2);
     }
     data()[size()] = ch;
-    data()[size() + 1] = 0;
     if (isbig()) {
         ++big()->n;
     } else {
         --small()->rem;
     }
+    data()[size()] = 0;
 }
 
 void

ctl/string.h

@@ -151,7 +151,7 @@ class string
         if (isbig() && big()->c <= __::sso_max)
             __builtin_trap();
 #endif
-        return isbig() ? __::big_mask & big()->c : __::sso_max;
+        return isbig() ? __::big_mask & big()->c : __::string_size;
     }
 
     iterator begin() noexcept

test/ctl/string_test.cc

@@ -18,6 +18,7 @@
 
 #include "ctl/string.h"
 
+#include <__type_traits/is_same.h>
 #include <__utility/move.h>
 
 #include "libc/runtime/runtime.h"
@@ -312,7 +313,7 @@ main()
             s.append(" world");
         }
         if (s != "hello world world world world world world world world world "
-            "world world") {
+                 "world world") {
             return 64;
         }
     }
@@ -353,6 +354,33 @@ main()
             return 78;
     }
 
+    {
+        ctl::string s;
+#undef ctl
+        if constexpr (std::is_same_v<ctl::string, decltype(s)>) {
+            // tests the small-string optimization on ctl::string
+            char* d = s.data();
+            for (int i = 0; i < 23; ++i) {
+                s.append("a");
+                if (s.data() != d) {
+                    return 79 + i;
+                }
+            }
+            s.append("a");
+            if (s.data() == d) {
+                return 103;
+            }
+        } else {
+            // just check that append in a loop works
+            for (int i = 0; i < 24; ++i) {
+                s.append("a");
+            }
+        }
+        if (s != "aaaaaaaaaaaaaaaaaaaaaaaa") {
+            return 104;
+        }
+    }
+
     CheckForMemoryLeaks();
     return 0;
 }