[llvm-commits] [compiler-rt] r161167 - in /compiler-rt/trunk/lib/asan: asan_interceptors.cc tests/asan_test.cc

[Alexander Potapenko]

Author: glider
Date: Thu Aug  2 05:25:46 2012
New Revision: 161167

URL: http://llvm.org/viewvc/llvm-project?rev=161167&view=rev
Log:
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.

Modified:
    compiler-rt/trunk/lib/asan/asan_interceptors.cc
    compiler-rt/trunk/lib/asan/tests/asan_test.cc

Modified: compiler-rt/trunk/lib/asan/asan_interceptors.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_interceptors.cc?rev=161167&r1=161166&r2=161167&view=diff
==============================================================================
--- compiler-rt/trunk/lib/asan/asan_interceptors.cc (original)
+++ compiler-rt/trunk/lib/asan/asan_interceptors.cc Thu Aug  2 05:25:46 2012
@@ -408,16 +408,22 @@
 # 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*, 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);

Modified: compiler-rt/trunk/lib/asan/tests/asan_test.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/tests/asan_test.cc?rev=161167&r1=161166&r2=161167&view=diff
==============================================================================
--- compiler-rt/trunk/lib/asan/tests/asan_test.cc (original)
+++ compiler-rt/trunk/lib/asan/tests/asan_test.cc Thu Aug  2 05:25:46 2012
@@ -1226,8 +1226,9 @@
   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 @@
   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 @@
   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 @@
   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);