FORTIFY_SOURCE: strcat / strncat optimize

__strcat_chk and __strncat_chk are slightly inefficient,
because they end up traversing over the same memory region
two times.

This change optimizes __strcat_chk / __strncat_chk so they
only access the memory once. Although I haven't benchmarked these
changes, it should improve the performance of these functions.

__strlen_chk - expose this function, even if -D_FORTIFY_SOURCE
isn't defined. This is needed to compile libc itself without
-D_FORTIFY_SOURCE.

Change-Id: Id2c70dff55a276b47c59db27a03734d659f84b74

libc/bionic/__strcat_chk.cpp

@@ -29,7 +29,6 @@
 #include <string.h>
 #include <stdlib.h>
 #include "libc_logging.h"
-#include <safe_iop.h>
 
 /*
  * Runtime implementation of __builtin____strcat_chk.
@@ -42,22 +41,24 @@
  * This strcat check is called if _FORTIFY_SOURCE is defined and
  * greater than 0.
  */
-extern "C" char *__strcat_chk (char *dest, const char *src, size_t dest_buf_size) {
-    // TODO: optimize so we don't scan src/dest twice.
-    size_t src_len  = strlen(src);
-    size_t dest_len = strlen(dest);
-    size_t sum;
+extern "C" char* __strcat_chk(
+        char* __restrict dest,
+        const char* __restrict src,
+        size_t dest_buf_size)
+{
+    char* save = dest;
+    size_t dest_len = __strlen_chk(dest, dest_buf_size);
 
-    // sum = src_len + dest_len + 1 (with overflow protection)
-    if (!safe_add3(&sum, src_len, dest_len, 1U)) {
-        __fortify_chk_fail("strcat integer overflow",
-                             BIONIC_EVENT_STRCAT_INTEGER_OVERFLOW);
+    dest += dest_len;
+    dest_buf_size -= dest_len;
+
+    while ((*dest++ = *src++) != '\0') {
+        dest_buf_size--;
+        if (__predict_false(dest_buf_size == 0)) {
+            __fortify_chk_fail("strcat buffer overflow",
+                               BIONIC_EVENT_STRCAT_BUFFER_OVERFLOW);
+        }
     }
 
-    if (sum > dest_buf_size) {
-        __fortify_chk_fail("strcat buffer overflow",
-                             BIONIC_EVENT_STRCAT_BUFFER_OVERFLOW);
-    }
-
-    return strcat(dest, src);
+    return save;
 }

libc/bionic/__strncat_chk.cpp

@@ -29,7 +29,6 @@
 #include <string.h>
 #include <stdlib.h>
 #include "libc_logging.h"
-#include <safe_iop.h>
 
 /*
  * Runtime implementation of __builtin____strncat_chk.
@@ -42,27 +41,33 @@
  * This strncat check is called if _FORTIFY_SOURCE is defined and
  * greater than 0.
  */
-extern "C" char *__strncat_chk (char *dest, const char *src,
-              size_t len, size_t dest_buf_size)
+extern "C" char *__strncat_chk(
+        char* __restrict dest,
+        const char* __restrict src,
+        size_t len, size_t dest_buf_size)
 {
-    // TODO: optimize so we don't scan src/dest twice.
-    size_t dest_len = strlen(dest);
-    size_t src_len = strlen(src);
-    if (src_len > len) {
-        src_len = len;
+    if (len == 0) {
+        return dest;
     }
 
-    size_t sum;
-    // sum = src_len + dest_len + 1 (with overflow protection)
-    if (!safe_add3(&sum, src_len, dest_len, 1U)) {
-        __fortify_chk_fail("strncat integer overflow",
-                             BIONIC_EVENT_STRNCAT_INTEGER_OVERFLOW);
+    size_t dest_len = __strlen_chk(dest, dest_buf_size);
+    char *d = dest + dest_len;
+    dest_buf_size -= dest_len;
+
+    while (*src != '\0') {
+        *d++ = *src++;
+        len--; dest_buf_size--;
+
+        if (__predict_false(dest_buf_size == 0)) {
+            __fortify_chk_fail("strncat buffer overflow",
+                               BIONIC_EVENT_STRNCAT_BUFFER_OVERFLOW);
+        }
+
+        if (len == 0) {
+            break;
+        }
     }
 
-    if (sum > dest_buf_size) {
-        __fortify_chk_fail("strncat buffer overflow",
-                             BIONIC_EVENT_STRNCAT_BUFFER_OVERFLOW);
-    }
-
-    return strncat(dest, src, len);
+    *d = '\0';
+    return dest;
 }

libc/include/string.h

@@ -49,6 +49,7 @@ extern char*  strchr(const char *, int) __purefunc;
 extern char*  strrchr(const char *, int) __purefunc;
 
 extern size_t strlen(const char *) __purefunc;
+extern size_t __strlen_chk(const char *, size_t);
 extern int    strcmp(const char *, const char *) __purefunc;
 extern char*  strcpy(char* __restrict, const char* __restrict);
 extern char*  strcat(char* __restrict, const char* __restrict);
@@ -207,8 +208,6 @@ size_t strlcat(char* __restrict dest, const char* __restrict src, size_t size) {
     return __strlcat_chk(dest, src, size, bos);
 }
 
-extern size_t __strlen_chk(const char *, size_t);
-
 __BIONIC_FORTIFY_INLINE
 size_t strlen(const char *s) {
     size_t bos = __bos(s);

libc/private/libc_logging.h

@@ -45,9 +45,6 @@ enum {
   BIONIC_EVENT_MEMSET_BUFFER_OVERFLOW = 80125,
   BIONIC_EVENT_STRCPY_BUFFER_OVERFLOW = 80130,
 
-  BIONIC_EVENT_STRCAT_INTEGER_OVERFLOW = 80200,
-  BIONIC_EVENT_STRNCAT_INTEGER_OVERFLOW = 80205,
-
   BIONIC_EVENT_RESOLVER_OLD_RESPONSE = 80300,
   BIONIC_EVENT_RESOLVER_WRONG_SERVER = 80305,
   BIONIC_EVENT_RESOLVER_WRONG_QUERY = 80310,

tests/fortify1_test.cpp

@@ -76,3 +76,166 @@ TEST(Fortify1_DeathTest, strncat2_fortified) {
   size_t n = atoi("10"); // avoid compiler optimizations
   ASSERT_EXIT(strncat(buf, "0123456789", n), testing::KilledBySignal(SIGSEGV), "");
 }
+
+TEST(Fortify1_DeathTest, strcat_fortified) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  char src[11];
+  strcpy(src, "0123456789");
+  char buf[10];
+  buf[0] = '\0';
+  ASSERT_EXIT(strcat(buf, src), testing::KilledBySignal(SIGSEGV), "");
+}
+
+extern "C" char* __strncat_chk(char*, const char*, size_t, size_t);
+extern "C" char* __strcat_chk(char*, const char*, size_t);
+
+TEST(Fortify1, strncat) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = __strncat_chk(buf, "01234", sizeof(buf) - strlen(buf) - 1, sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('\0', buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(Fortify1, strncat2) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = __strncat_chk(buf, "0123456789", 5, sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('\0', buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(Fortify1, strncat3) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = '\0';
+  char* res = __strncat_chk(buf, "0123456789", 5, sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('0',  buf[0]);
+  ASSERT_EQ('1',  buf[1]);
+  ASSERT_EQ('2',  buf[2]);
+  ASSERT_EQ('3',  buf[3]);
+  ASSERT_EQ('4',  buf[4]);
+  ASSERT_EQ('\0', buf[5]);
+  ASSERT_EQ('A',  buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(Fortify1, strncat4) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[9] = '\0';
+  char* res = __strncat_chk(buf, "", 5, sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('A',  buf[0]);
+  ASSERT_EQ('A',  buf[1]);
+  ASSERT_EQ('A',  buf[2]);
+  ASSERT_EQ('A',  buf[3]);
+  ASSERT_EQ('A',  buf[4]);
+  ASSERT_EQ('A',  buf[5]);
+  ASSERT_EQ('A',  buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('\0', buf[9]);
+}
+
+TEST(Fortify1, strncat5) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = __strncat_chk(buf, "01234567", 8, sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('5', buf[6]);
+  ASSERT_EQ('6',  buf[7]);
+  ASSERT_EQ('7',  buf[8]);
+  ASSERT_EQ('\0',  buf[9]);
+}
+
+TEST(Fortify1, strncat6) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = __strncat_chk(buf, "01234567", 9, sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('5', buf[6]);
+  ASSERT_EQ('6',  buf[7]);
+  ASSERT_EQ('7',  buf[8]);
+  ASSERT_EQ('\0',  buf[9]);
+}
+
+
+TEST(Fortify1, strcat) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = __strcat_chk(buf, "01234", sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('\0', buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(Fortify1, strcat2) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = __strcat_chk(buf, "01234567", sizeof(buf));
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('5', buf[6]);
+  ASSERT_EQ('6',  buf[7]);
+  ASSERT_EQ('7',  buf[8]);
+  ASSERT_EQ('\0',  buf[9]);
+}

tests/fortify2_test.cpp

@@ -80,6 +80,32 @@ TEST(Fortify2_DeathTest, strncat2_fortified2) {
   ASSERT_EXIT(strncat(myfoo.a, "0123456789", n), testing::KilledBySignal(SIGSEGV), "");
 }
 
+TEST(Fortify2_DeathTest, strncat3_fortified2) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  foo myfoo;
+  memcpy(myfoo.a, "0123456789", sizeof(myfoo.a)); // unterminated string
+  myfoo.b[0] = '\0';
+  size_t n = atoi("10"); // avoid compiler optimizations
+  ASSERT_EXIT(strncat(myfoo.b, myfoo.a, n), testing::KilledBySignal(SIGSEGV), "");
+}
+
+TEST(Fortify2_DeathTest, strcat_fortified2) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  char src[11];
+  strcpy(src, "0123456789");
+  foo myfoo;
+  myfoo.a[0] = '\0';
+  ASSERT_EXIT(strcat(myfoo.a, src), testing::KilledBySignal(SIGSEGV), "");
+}
+
+TEST(Fortify2_DeathTest, strcat2_fortified2) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  foo myfoo;
+  memcpy(myfoo.a, "0123456789", sizeof(myfoo.a)); // unterminated string
+  myfoo.b[0] = '\0';
+  ASSERT_EXIT(strcat(myfoo.b, myfoo.a), testing::KilledBySignal(SIGSEGV), "");
+}
+
 /***********************************************************/
 /* TESTS BELOW HERE DUPLICATE TESTS FROM fortify1_test.cpp */
 /***********************************************************/
@@ -138,3 +164,12 @@ TEST(Fortify2_DeathTest, strncat2_fortified) {
   size_t n = atoi("10"); // avoid compiler optimizations
   ASSERT_EXIT(strncat(buf, "0123456789", n), testing::KilledBySignal(SIGSEGV), "");
 }
+
+TEST(Fortify2_DeathTest, strcat_fortified) {
+  ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+  char src[11];
+  strcpy(src, "0123456789");
+  char buf[10];
+  buf[0] = '\0';
+  ASSERT_EXIT(strcat(buf, src), testing::KilledBySignal(SIGSEGV), "");
+}

tests/string_test.cpp

@@ -209,6 +209,120 @@ TEST(string, strcat) {
   }
 }
 
+TEST(string, strcat2) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = strcat(buf, "01234");
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('\0', buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(string, strcat3) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = strcat(buf, "01234567");
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('5', buf[6]);
+  ASSERT_EQ('6',  buf[7]);
+  ASSERT_EQ('7',  buf[8]);
+  ASSERT_EQ('\0',  buf[9]);
+}
+
+TEST(string, strncat2) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = strncat(buf, "01234", sizeof(buf) - strlen(buf) - 1);
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('\0', buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(string, strncat3) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = strncat(buf, "0123456789", 5);
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('\0', buf[6]);
+  ASSERT_EQ('A',  buf[7]);
+  ASSERT_EQ('A',  buf[8]);
+  ASSERT_EQ('A',  buf[9]);
+}
+
+TEST(string, strncat4) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = strncat(buf, "01234567", 8);
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('5', buf[6]);
+  ASSERT_EQ('6',  buf[7]);
+  ASSERT_EQ('7',  buf[8]);
+  ASSERT_EQ('\0',  buf[9]);
+}
+
+TEST(string, strncat5) {
+  char buf[10];
+  memset(buf, 'A', sizeof(buf));
+  buf[0] = 'a';
+  buf[1] = '\0';
+  char* res = strncat(buf, "01234567", 9);
+  ASSERT_EQ(buf, res);
+  ASSERT_EQ('a',  buf[0]);
+  ASSERT_EQ('0',  buf[1]);
+  ASSERT_EQ('1',  buf[2]);
+  ASSERT_EQ('2',  buf[3]);
+  ASSERT_EQ('3',  buf[4]);
+  ASSERT_EQ('4',  buf[5]);
+  ASSERT_EQ('5', buf[6]);
+  ASSERT_EQ('6',  buf[7]);
+  ASSERT_EQ('7',  buf[8]);
+  ASSERT_EQ('\0',  buf[9]);
+}
+
 TEST(string, strchr_with_0) {
   char buf[10];
   const char* s = "01234";