[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:494
+    uptr del_length = REAL(strlen)(delimiters);
+    COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, delimiters, del_length, 1);
+    return result;
----------------
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.




https://reviews.llvm.org/D30384