[PATCH] D30384: [asan] Add an interceptor for strtok
Manuel Rigger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 15 11:18:05 PDT 2017
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
More information about the llvm-commits
mailing list