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
diff --git a/libc/bionic/__strcat_chk.cpp b/libc/bionic/__strcat_chk.cpp
index fb46e0d..e0b3259 100644
--- a/libc/bionic/__strcat_chk.cpp
+++ b/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;
}
diff --git a/libc/bionic/__strncat_chk.cpp b/libc/bionic/__strncat_chk.cpp
index ab28541..f54d838 100644
--- a/libc/bionic/__strncat_chk.cpp
+++ b/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;
}
diff --git a/libc/include/string.h b/libc/include/string.h
index f8c573c..1691b16 100644
--- a/libc/include/string.h
+++ b/libc/include/string.h
@@ -49,6 +49,7 @@
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 @@
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);
diff --git a/libc/private/libc_logging.h b/libc/private/libc_logging.h
index c6e1765..e62ddf2 100644
--- a/libc/private/libc_logging.h
+++ b/libc/private/libc_logging.h
@@ -45,9 +45,6 @@
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,
diff --git a/tests/fortify1_test.cpp b/tests/fortify1_test.cpp
index 70d458a..a9d8afd 100644
--- a/tests/fortify1_test.cpp
+++ b/tests/fortify1_test.cpp
@@ -76,3 +76,166 @@
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]);
+}
diff --git a/tests/fortify2_test.cpp b/tests/fortify2_test.cpp
index c937e91..eee5770 100644
--- a/tests/fortify2_test.cpp
+++ b/tests/fortify2_test.cpp
@@ -80,6 +80,32 @@
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 @@
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), "");
+}
diff --git a/tests/string_test.cpp b/tests/string_test.cpp
index 63bfadb..a0924dc 100644
--- a/tests/string_test.cpp
+++ b/tests/string_test.cpp
@@ -209,6 +209,120 @@
}
}
+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";
