[PATCH] D30384: [asan] Add an interceptor for strtok

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl requested changes to this revision.
alekseyshl added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:475
 #endif
 
+INTERCEPTOR(char*, strtok, char *str, const char *delimiters) {
----------------
#if SANITIZER_INTERCEPT_STRTOK and define it to 1 for now.


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:485
+      // static buffer for subsequent invocations.
+      result = REAL(strtok)(str, delimiters);
+    } else {
----------------
Why are we not checking result string here? When non-null, it's expected to be a part of the original str and to be readable and valid, right?


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:489
+      result = REAL(strtok)(str, delimiters);
+      uptr result_length = REAL(strlen)(result);
+      COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, str, str_length,
----------------
What if your first call to strtok returns nullptr?


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:491
+      COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, str, str_length,
+          result_length+1);
+    }
----------------
result_length + 1); and indent it to align with other arguments


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:494
+    uptr del_length = REAL(strlen)(delimiters);
+    COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, delimiters, del_length, 1);
+    return result;
----------------
First, it should be ..., del_length, del_length + 1); and second, it does not make sense to use COMMON_INTERCEPTOR_READ_STRING_OF_LEN here, 

   COMMON_INTERCEPTOR_READ_RANGE(ctx, delimiters, REAL(strlen)(delimiters) + 1);

would be enough.



================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:499
+}
+#define INIT_STRTOK COMMON_INTERCEPT_FUNCTION(strtok)
+
----------------
I'd move it below strcasestr interceptor (strstr and strcasestr are related).


================
Comment at: test/asan/TestCases/strtok.c:1
+// Test overflows with strict_string_checks
+// RUN: %clang_asan %s -o %t && %env_asan_opts=strict_string_checks=true not %run %t test1 2>&1 | FileCheck %s --check-prefix=CHECK1
----------------
Why not have strtok-1.c, strtok-2.c etc. files? Tests would be quite a bit more readable.


================
Comment at: test/asan/TestCases/strtok.c:22
+
+// test overflow in delimiter
+void test1() {
----------------
Why other tests do not have comments? Please split them into separate files and add a top line comment explaining what exactly is tested.


https://reviews.llvm.org/D30384