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

[Maria Guseva]

================
Comment at: lib/msan/msan_interceptors.cc:96
@@ -95,1 +95,3 @@
 
+#define CHECK_UNPOISONED_STRING(x, n)                           \
+  CHECK_UNPOISONED((x),                                         \
----------------
ygribov wrote:
> glider wrote:
> > Am I right that you're only using this macro with n=0?
> I think Maria wanted added n to anticipate future work on conservative length checking.
>ygribov
>I think Maria wanted added n to anticipate future work on conservative length checking.
Exactly.

================
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);
----------------
glider wrote:
> I suggest putting the UNUSED function under the #ifdef below and remove the UNUSED attribute.
These functions were moved from asan_interceptors.cc source where they are used in strtol-related interceptors. Some of them(strtol\atoi\atol) are not under any if directive. Is there any better place where to put this helpers to be shared by asan and common interceptors?

================
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);
----------------
ygribov wrote:
> glider wrote:
> > 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.
> Looks like COMMON_INTERCEPTOR_READ_STRING needs a check for NULL.
Here is my bug. It needs check for NULL **before** COMMON_INTERCEPTOR_READ_STRING. Cause dlopen  accepts null filename pointer as a normal argument and then there is nothing to check here. In other interceptors (e.g strsmthing) NULL pointer arg is an error case. So the program should fail with segfault. And it will in internal_strlen. I'll recheck other modified interceptors for such cases.

================
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.");
----------------
glider wrote:
> 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).
The "replace_" flags were suggested in original issue. I agree it should be aligned with existing one. 

================
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'
----------------
glider wrote:
> ygribov wrote:
> > glider wrote:
> > > Why do you need this line?
> > AFAIR unit tests are not strict_str-safe.
> Is that something we may want to fix?
> If so, please file a bug and add a TODO
>ygribov
>AFAIR unit tests are not strict_str-safe.
Right. I saw fails caused by checks relied on standard optimizations in string functions. And I didn't want to remove these tests at all cause they seems meaningful.

http://reviews.llvm.org/D6056

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