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

[Yury Gribov via Phabricator via llvm-commits]

ygribov 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;
----------------
ygribov wrote:
> alekseyshl wrote:
> > 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.
> Sorry guys, I was out. Frankly I agree with @alekseyshl in that we shouldn't unconditionally call `strlen` on `str` and `delimeters`. They are not necessarily null-terminated so by calling `REAL(strlen)` when strict_strings is off, you risk triggering a runtime error.
> 
> See my suggestions below for how to change the code to avoid strlens.
> Your last argument being just 1 should definitely be fixed,
> now you're checking just the first char, right?

We can't easily check for more when `strict_strings` is off (unless we reimplement most of `strtok` in interceptor).


https://reviews.llvm.org/D30384