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

[Aleksey Shlyapnikov via Phabricator via llvm-commits]

alekseyshl added inline comments.


================
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:
> > 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?
> Note, that we use the not-intercepted `strlen`. Following, an out-of-bounds access might not result in an error here.
> 
> Yes, we only check the first delimiter (for `strict_string_checks=0`), as @ygribov suggested, if I understood him right. @ygribov, can you please comment to clear things up?
> 
> Here is a demonstration of the behavior on an example:
> ```
> #include <stdio.h>
> #include <string.h>
> 
> int main() {
>     char buf[] = "abcb";
>     char del[] = {'b'}; // missing NULL terminator
>     char* result = strtok(buf, del);
>     printf("%s\n", result);
> }
> ```
> Executing the program with `ASAN_OPTIONS=strict_string_checks=1 ./a.out` lets ASan output an error. However, executing with `ASAN_OPTIONS=strict_string_checks=0 ./a.out` does not result in an error.
> 
> Here is also the documentation for `strict_string_checks`:
> ```
> 	strict_string_checks
> 		- If set check that string arguments are properly null-terminated
> ```
> 
> I'm happy to change it, but I want to be sure that we implement the expected behavior.
> 
> 
The code I proposed uses non-intercepted strlen as well:

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

The problem I'm trying to point out is not about the first delimiter (that's fine, but it's about the other COMMON_INTERCEPTOR_READ_STRING_OF_LEN use, the one above), it's that you pass n=1 to your second macro, this one:

  COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, delimiters, del_length, 1);

which means "1 char". I believe, it was not your intention.


https://reviews.llvm.org/D30384