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

Aleksey Shlyapnikov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 15 11:43:31 PDT 2017


alekseyshl requested changes to this revision.
alekseyshl added a comment.
This revision now requires changes to proceed.

Awesome, thank you for doing it!



================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:485
+      // static buffer for subsequent invocations.
+      result = REAL(strtok)(str, delimiters);
+    } else {
----------------
mrigger wrote:
> 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.
Ah, right, thanks.


================
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;
----------------
mrigger wrote:
> 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?
You're explicitly calling strlen on that string, it will look for the \0 anyway. Check what COMMON_INTERCEPTOR_READ_STRING_OF_LEN is doing, substitute your arguments.

Your last argument being just 1 should definitely be fixed, now you're checking just the first char, right?


================
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
----------------
mrigger wrote:
> 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"?
Style evolves. This is not critical, especially with your last change, lines are shorter and easier to parse, but still, I prefer to keep all lines under 80 chars. If it is not too big of a problem, I'd appreciate breaking those exceeding 80 chars at FileCheck, as I suggested, and please indent FileCheck:

`RUN:    FileCheck ...`


https://reviews.llvm.org/D30384





More information about the llvm-commits mailing list