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

[Manuel Rigger via Phabricator via llvm-commits]

mrigger added inline comments.


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


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:485
+      // static buffer for subsequent invocations.
+      result = REAL(strtok)(str, delimiters);
+    } else {
----------------
alekseyshl wrote:
> 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?
Other interceptors also do not check the result of the libc functions. See for example `strchr` or `strpbrk`. I think all interceptors are based on the assumption that the libc functions do not contain any errors.


================
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,
----------------
alekseyshl wrote:
> What if your first call to strtok returns nullptr?
Good catch. I fixed the error and added a test case.


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


================
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;
----------------
alekseyshl wrote:
> 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.
> 
I implemented it like you suggested in my first revision. The current implementation is based on @ygribov's feedback and his explanations about `strict_strings`. Did I understand them wrong?


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


================
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
----------------
alekseyshl wrote:
> Why not have strtok-1.c, strtok-2.c etc. files? Tests would be quite a bit more readable.
I agree that the tests would be more readable. However, I observed that other larger `*.str` test cases follow this structure; for example, see `strtol_strict`, `strncasecmp_strict`, and `strncmp_strict`.


================
Comment at: test/asan/TestCases/strtok.c:12
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=false %run %t test4 2>&1
+// RUN: %clang_asan %s -o %t && %env_asan_opts=strict_string_checks=false not %run %t test5 2>&1 | FileCheck %s --check-prefix=CHECK5
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=false %run %t test5 2>&1
----------------
alekseyshl wrote:
> And, if we go with one file, please wrap these long lines like this:
> 
> // RUN: ... | \
> // RUN:     FileCheck ...
The other test cases, from which I copied this pattern, are all written in one line. Should I still break the existing "convention"?


================
Comment at: test/asan/TestCases/strtok.c:22
+
+// test overflow in delimiter
+void test1() {
----------------
alekseyshl wrote:
> kcc wrote:
> > alekseyshl wrote:
> > > Why other tests do not have comments? Please split them into separate files and add a top line comment explaining what exactly is tested.
> > Not worth it, IMHO
> > But what I did miss is that this test does "%clang_asan %s -o %t" 5 times, while 1 is enough. 
> Ok, fine, one file is acceptable too, but still, comments would be much welcomed!
I added a comment for each test case. Additionally, the test is now only compiled once.


https://reviews.llvm.org/D30384