[PATCH] D18020: [sanitizer] Add strlen to the common interceptors

[Alexey Samsonov via llvm-commits]

samsonov added a comment.

Looks mostly good, thank you for working on this.


================
Comment at: lib/asan/asan_flags.cc:162
@@ -161,1 +161,3 @@
   }
+  // Preserve the old meanings of flags from prior to moving interceptors
+  // from asan into the common set of interceptors.
----------------
Hm... I think I would prefer to break the old (non-default) use case rather than have this ugly workaround... As I understand it, we will gradually phase out ASAN_OPTIONS=replace_str flag anyway, so maybe we can get away with a warning instead:

  if (!f->replace_str && common_flags()->intercept_strlen) {
    Report("WARNING: strlen interceptor is enabled even though replace_str=0. Use intercept_strlen=0 to disable it.");
  }

================
Comment at: lib/asan/asan_interceptors.cc:151
@@ -150,1 +150,3 @@
   ASAN_READ_RANGE(ctx, ptr, size)
+// We check asan_init_is_running to handle situations that include:
+// + strlen is called from malloc_default_purgeable_zone()
----------------
Probably you can omit this comment: I believe there are more cases when libc function may be called while __asan_init is still running. E.g. it may be required for FreeBSD: see https://llvm.org/svn/llvm-project/compiler-rt/trunk@222885

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:205
@@ +204,3 @@
+INTERCEPTOR(SIZE_T, strlen, const char *s) {
+  if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
+    return internal_strlen(s);
----------------
I don't think you need it here: looks like this will be handled by COMMON_INTERCEPTOR_ENTER macro.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:211
@@ +210,3 @@
+  if (common_flags()->intercept_strlen)
+    COMMON_INTERCEPTOR_READ_STRING(ctx, s, result + 1);
+  return result;
----------------
I think it should be just COMMON_INTERCEPTOR_READ_RANGE - COMMON_INTERCEPTOR_READ_STRING is only used for the special-handling of cases where we are accessing only some prefix of a null-terminated string.


http://reviews.llvm.org/D18020