[PATCH] Moar string interceptors: strstr, strcasestr, strcspn, strpbrk

[Alexander Potapenko]

================
Comment at: lib/msan/msan_interceptors.cc:96
@@ -95,1 +95,3 @@
 
+#define CHECK_UNPOISONED_STRING(x, n)                           \
+  CHECK_UNPOISONED((x),                                         \
----------------
Am I right that you're only using this macro with n=0?

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:327
@@ +326,3 @@
+    COMMON_INTERCEPTOR_READ_STRING(ctx, s1,
+        r ? r - s1 + 1: REAL(strlen)(s1) + 1);
+  }
----------------
Please put a space before the colon.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:2401
@@ -2310,1 +2400,3 @@
 
+UNUSED static inline bool IsValidStrtolBase(int base) {
+  return (base == 0) || (2 <= base && base <= 36);
----------------
I suggest putting the UNUSED function under the #ifdef below and remove the UNUSED attribute.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:2427
@@ +2426,3 @@
+  // if base is valid.
+  if (IsValidStrtolBase(base)) {
+    FixRealStrtolEndptr(nptr, &real_endptr);
----------------
You don't need IsValidStrtolBase anymore:
```
  bool is_valid_base = (base == 0) || (2 <= base && base <= 36);
  if (is_valid_base) {
    ...
  }
  COMMON_INTERCEPTOR_READ_STRING(ctx, nptr, is_valid_base ? ...)
```

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:4759
@@ -4631,2 +4758,3 @@
   COMMON_INTERCEPTOR_ENTER_NOIGNORE(ctx, dlopen, filename, flag);
+  COMMON_INTERCEPTOR_READ_STRING(ctx, filename, 0);
   void *res = REAL(dlopen)(filename, flag);
----------------
Chrome's net_unittests crash for me with the following stacktrace:

```
Program received signal SIGSEGV, Segmentation fault.
__sanitizer::internal_strlen (s=s at entry=0x0)
    at /usr/local/google/ssd/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_libc.cc:155
155	  while (s[i]) i++;
(gdb) bt
#0  __sanitizer::internal_strlen (s=s at entry=0x0)
    at /usr/local/google/ssd/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_libc.cc:155
#1  0x000000000059f080 in __interceptor_dlopen (filename=filename at entry=0x0, 
    flag=flag at entry=1)
    at /usr/local/google/ssd/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:4759
#2  0x00007ffff5167ec2 in pr_FindSymbolInProg (
    name=0x7ffff517f29f "nspr_use_zone_allocator") at prmem.c:98
#3  0x00007ffff5167fad in _PR_InitZones () at prmem.c:154
...
```

We'll need to add yet another dlopen() test once we figure out what's wrong.

================
Comment at: lib/sanitizer_common/sanitizer_flags.cc:167
@@ -162,1 +166,3 @@
       "default and for sanitizers that don't reserve lots of virtual memory.");
+  ParseFlag(str, &f->replace_strstr, "replace_strstr", "If set, uses custom "
+       "wrappers for strstr and strcasestr functions to find more errors.");
----------------
I think we need to be consistent in naming the flags that disable interceptors.
There's already "intercept_tls_get_addr", so we either need to change it (no need to do that in this CL) or rename your flags into "intercept_something".
(Note that "intrin" in "replace_intrin" isn't a function name, so you don't need to be consistent with that).

================
Comment at: lib/sanitizer_common/sanitizer_flags.cc:173
@@ +172,3 @@
+       "wrappers for strpbrk function to find more errors.");
+  ParseFlag(str, &f->strict_str, "strict_str", "If set, check that "
+       "string arguments are properly null-terminated");
----------------
I think "strict_string_checks" is more informative, but that's bikeshedding.

================
Comment at: lib/tsan/rtl/tsan_interceptors.cc:243
@@ +242,3 @@
+  MemoryAccessRange(thr, pc, (uptr)s,                               \
+    common_flags()->strict_str ? internal_strlen(s) + 1 : n, false )
+
----------------
nit: spare whitespace before ')'

================
Comment at: test/asan/TestCases/atoi_strict.c:14
@@ +13,3 @@
+  // CHECK: READ of size
+  // CHECK:'{{[p|z]}}' <== Memory access at offset {{[0-9]+ .*}}flows this variable
+  assert(r == 1);
----------------
Can you please add a space after the 'CHECK:' label here and below?

================
Comment at: test/asan/TestCases/strstr-1.c:13
@@ +12,3 @@
+  char s2[] = "c";
+  char s1[] = {'a', 'b'};
+  char s3 = 0;
----------------
Note that you can use #ifdef directives and different check prefixes to combine related test cases into one file.

================
Comment at: test/asan/Unit/lit.site.cfg.in:27
@@ +26,2 @@
+# Disable strict str checks in unit tests
+config.environment['ASAN_OPTIONS'] = 'strict_str=false'
----------------
Why do you need this line?

================
Comment at: test/sanitizer_common/TestCases/strcasestr.c:3
@@ +2,3 @@
+
+// There's no interceptor for strcasestr on Windows
+// XFAIL: win32
----------------
Why? Is that because strcasestr() is unavailable on Windows?

http://reviews.llvm.org/D6056

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/