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

[Dmitry Vyukov]

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:269
@@ -268,1 +268,3 @@
 
+
+#if SANITIZER_INTERCEPT_STRSTR
----------------
delete empty line (two consequent empty lines)

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:271
@@ +270,3 @@
+#if SANITIZER_INTERCEPT_STRSTR
+
+static inline void StrstrCheck(void *ctx, char *r, const char *s1,
----------------
delete empty line (that's how it is written everywhere in this file)

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:277
@@ +276,3 @@
+    COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, s1, len1,
+                                          r ? r - s1 + len2 + 1 : len1 + 1);
+    COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, s2, len2, r ? len2 + 1 :
----------------
Is "r - s1 + len2 + 1" correct?
I think it should be "r - s1 + len2", because strstr can read only len2 bytes to find a match.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:279
@@ +278,3 @@
+    COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, s2, len2, r ? len2 + 1 :
+                                   (len2 <= len1 ? len2 : len1 + 1));
+}
----------------
I am not sure such complex computations are necessary. People should not rely on such things.
If anything, I think, the fifth argument should be 1, because if I call strstr("aaaaaaaaaaaa", "bcdef") I can "expect" that the function will read only the first symbol "b" from the second string.

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:283
@@ +282,3 @@
+INTERCEPTOR(char*, strstr, const char *s1, const char *s2) {
+  if COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED
+    return internal_strstr(s1, s2);
----------------
add parens around COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:288
@@ +287,3 @@
+  char *r = REAL(strstr)(s1, s2);
+  if (common_flags()->intercept_strstr) {
+    StrstrCheck(ctx, r, s1, s2);
----------------
drop {}

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:294
@@ +293,3 @@
+
+#define INIT_STRSTR COMMON_INTERCEPT_FUNCTION(strstr);
+
----------------
#define INIT_STRSTR \
    COMMON_INTERCEPT_FUNCTION(strstr); \
    COMMON_INTERCEPT_FUNCTION(strcasestr);

and move below

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:306
@@ +305,3 @@
+
+#define INIT_STRCASESTR COMMON_INTERCEPT_FUNCTION(strcasestr);
+
----------------
remove

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:310
@@ +309,3 @@
+#define INIT_STRSTR
+#define INIT_STRCASESTR
+#endif
----------------
remove

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:312
@@ +311,3 @@
+#endif
+
+
----------------
delete empty line

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:315
@@ +314,3 @@
+#if SANITIZER_INTERCEPT_STRCSPN
+
+INTERCEPTOR(SIZE_T, strcspn, const char *s1, const char *s2) {
----------------
delete empty line

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:322
@@ +321,3 @@
+    COMMON_INTERCEPTOR_READ_STRING(ctx, s2,
+        s1 && *s1 ? internal_strchr(s2, s1[r]) - s2 + 1 : 0);
+    COMMON_INTERCEPTOR_READ_STRING(ctx, s1, r + 1);
----------------
I don't believe this is necessary. Make it just:
READ_RANGE(s2, REAL(strlen)(s2) + 1);

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:328
@@ +327,3 @@
+
+#define INIT_STRCSPN COMMON_INTERCEPT_FUNCTION(strcspn);
+
----------------
move below (see how it is formatted in other case in this file)

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:337
@@ +336,3 @@
+        r ? internal_strchr(s2, *r) - s2 + 1 :
+        *s1 ? REAL(strlen)(s2) + 1 : 0);
+    COMMON_INTERCEPTOR_READ_STRING(ctx, s1,
----------------
I don't believe this complex code is necessary. Make it just:
READ_RANGE(s2, REAL(strlen)(s2) + 1);

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:4945
@@ +4944,3 @@
+  INIT_STRSTR;
+  INIT_STRCASESTR;
+  INIT_STRCSPN;
----------------
delete

================
Comment at: lib/sanitizer_common/sanitizer_flags.inc:156
@@ -155,1 +155,3 @@
             "If set check that string arguments are properly null-terminated")
+COMMON_FLAG(bool, intercept_strstr, true,
+            "If set, uses custom wrappers for strstr and strcasestr functions "
----------------
do we need all these additional flags? why?

================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:58
@@ +57,3 @@
+#define SANITIZER_INTERCEPT_STRSTR 1
+#define SANITIZER_INTERCEPT_STRCASESTR SI_NOT_WINDOWS
+#define SANITIZER_INTERCEPT_STRCSPN 1
----------------
this is unused, delete

================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:60
@@ -57,1 +59,3 @@
+#define SANITIZER_INTERCEPT_STRCSPN 1
+#define SANITIZER_INTERCEPT_STRPBRK 1
 #define SANITIZER_INTERCEPT_TEXTDOMAIN SI_LINUX_NOT_ANDROID
----------------
delete

http://reviews.llvm.org/D9017

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