From 33824c54f6160450b505a96aff834166cef179ab Mon Sep 17 00:00:00 2001
From: Alexander Potapenko <glider@google.com>
Date: Thu, 2 Aug 2012 10:25:46 +0000
Subject: [PATCH] Make strcat() and strncat() more standard-compliant (check
 for invalid parameters even if zero bytes is copied, more accurate overlap
 check) Fix the tests that were relying on the incorrect behavior.

llvm-svn: 161167
---
 compiler-rt/lib/asan/asan_interceptors.cc | 23 +++++++++++++++--------
 compiler-rt/lib/asan/tests/asan_test.cc   | 13 +++++++------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/compiler-rt/lib/asan/asan_interceptors.cc b/compiler-rt/lib/asan/asan_interceptors.cc
index b0571f87bfcf..ec03fff02169 100644
--- a/compiler-rt/lib/asan/asan_interceptors.cc
+++ b/compiler-rt/lib/asan/asan_interceptors.cc
@@ -408,16 +408,22 @@ DEFINE_REAL(char*, index, const char *string, int c)
 # endif
 #endif  // ASAN_INTERCEPT_INDEX
 
+// For both strcat() and strncat() we need to check the validity of |to|
+// argument irrespective of the |from| length.
 INTERCEPTOR(char*, strcat, char *to, const char *from) {  // NOLINT
   ENSURE_ASAN_INITED();
   if (flags()->replace_str) {
     uptr from_length = REAL(strlen)(from);
     ASAN_READ_RANGE(from, from_length + 1);
+    uptr to_length = REAL(strlen)(to);
+    ASAN_READ_RANGE(to, to_length);
+    ASAN_WRITE_RANGE(to + to_length, from_length + 1);
+    // If the copying actually happens, the |from| string should not overlap
+    // with the resulting string starting at |to|, which has a length of
+    // to_length + from_length + 1.
     if (from_length > 0) {
-      uptr to_length = REAL(strlen)(to);
-      ASAN_READ_RANGE(to, to_length);
-      ASAN_WRITE_RANGE(to + to_length, from_length + 1);
-      CHECK_RANGES_OVERLAP("strcat", to, to_length + 1, from, from_length + 1);
+      CHECK_RANGES_OVERLAP("strcat", to, from_length + to_length + 1,
+                           from, from_length + 1);
     }
   }
   return REAL(strcat)(to, from);  // NOLINT
@@ -425,15 +431,16 @@ INTERCEPTOR(char*, strcat, char *to, const char *from) {  // NOLINT
 
 INTERCEPTOR(char*, strncat, char *to, const char *from, uptr size) {
   ENSURE_ASAN_INITED();
-  if (flags()->replace_str && size > 0) {
+  if (flags()->replace_str) {
     uptr from_length = MaybeRealStrnlen(from, size);
-    ASAN_READ_RANGE(from, Min(size, from_length + 1));
+    uptr copy_length = Min(size, from_length + 1);
+    ASAN_READ_RANGE(from, copy_length);
     uptr to_length = REAL(strlen)(to);
     ASAN_READ_RANGE(to, to_length);
     ASAN_WRITE_RANGE(to + to_length, from_length + 1);
     if (from_length > 0) {
-      CHECK_RANGES_OVERLAP("strncat", to, to_length + 1,
-                           from, Min(size, from_length + 1));
+      CHECK_RANGES_OVERLAP("strncat", to, to_length + copy_length + 1,
+                           from, copy_length);
     }
   }
   return REAL(strncat)(to, from, size);
diff --git a/compiler-rt/lib/asan/tests/asan_test.cc b/compiler-rt/lib/asan/tests/asan_test.cc
index 8e967e929899..994e78aa3bcc 100644
--- a/compiler-rt/lib/asan/tests/asan_test.cc
+++ b/compiler-rt/lib/asan/tests/asan_test.cc
@@ -1226,8 +1226,9 @@ TEST(AddressSanitizer, StrCatOOBTest) {
   strcat(to, from);
   strcat(to, from);
   strcat(to + from_size, from + from_size - 2);
-  // Catenate empty string is not always an error.
-  strcat(to - 1, from + from_size - 1);
+  // Passing an invalid pointer is an error even when concatenating an empty
+  // string.
+  EXPECT_DEATH(strcat(to - 1, from + from_size - 1), LeftOOBErrorMessage(1));
   // One of arguments points to not allocated memory.
   EXPECT_DEATH(strcat(to - 1, from), LeftOOBErrorMessage(1));
   EXPECT_DEATH(strcat(to, from - 1), LeftOOBErrorMessage(1));
@@ -1262,8 +1263,8 @@ TEST(AddressSanitizer, StrNCatOOBTest) {
   strncat(to, from, from_size);
   from[from_size - 1] = '\0';
   strncat(to, from, 2 * from_size);
-  // Catenating empty string is not an error.
-  strncat(to - 1, from, 0);
+  // Catenating empty string with an invalid string is still an error.
+  EXPECT_DEATH(strncat(to - 1, from, 0), LeftOOBErrorMessage(1));
   strncat(to, from + from_size - 1, 10);
   // One of arguments points to not allocated memory.
   EXPECT_DEATH(strncat(to - 1, from, 2), LeftOOBErrorMessage(1));
@@ -1336,7 +1337,7 @@ TEST(AddressSanitizer, StrArgsOverlapTest) {
   str[10] = '\0';
   str[20] = '\0';
   strcat(str, str + 10);
-  strcat(str, str + 11);
+  EXPECT_DEATH(strcat(str, str + 11), OverlapErrorMessage("strcat"));
   str[10] = '\0';
   strcat(str + 11, str);
   EXPECT_DEATH(strcat(str, str + 9), OverlapErrorMessage("strcat"));
@@ -1347,7 +1348,7 @@ TEST(AddressSanitizer, StrArgsOverlapTest) {
   memset(str, 'z', size);
   str[10] = '\0';
   strncat(str, str + 10, 10);  // from is empty
-  strncat(str, str + 11, 10);
+  EXPECT_DEATH(strncat(str, str + 11, 10), OverlapErrorMessage("strncat"));
   str[10] = '\0';
   str[20] = '\0';
   strncat(str + 5, str, 5);
-- 
GitLab