[asan] Add an interceptor for strtok

[mrigger]

Summary

This change addresses https://github.com/google/sanitizers/issues/766. I tested the change with make check-asan and the newly added test case.

[ygribov]

I suggest to regenerate patch with -U99999, to get more context.

asan_interceptors.cc

549 ↗

(On Diff #89814)

Please use ASAN_READ_STRING instead, similar to other string interceptors. str_size should be checked only if strict_strings is enabled, otherwise you can only check range up to first delimiter (inclusively).

tests/asan_str_test.cc

118 ↗

(On Diff #89814)

Better avoid aliasing, strtok arguments are restricted (more below).

121 ↗

(On Diff #89814)

delim is somewhat misleading name cause it's also used in other context here.

123 ↗

(On Diff #89814)

What does this add to previous tests?

[mrigger]

Thanks a lot for your feedback @ygribov! I improved the test case based on it. Additionally, I changed the interceptor to use ASAN_READ_STRING_OF_LEN instead of directly calling ASAN_READ_RANGE. However, I think I have not yet understood why using strict_strings should only verify the string until the first delimiter. If I select the passed n to be smaller than strlen(str) + 1 then the test case fails, since it does not detect the overflow. What did I understand wrong? Should I instead implement the test case in projects/compiler-rt/test/asan/TestCases/ and set RUN: %env_asan_opts=strict_string_checks=true? If yes, should I select n to be strlen(str) (like the other interceptors) or 1?

[kcc]

lib/asan/asan_interceptors.cc

541 ↗

(On Diff #89941)

This should go to lib/sanitizer_common/sanitizer_common_interceptors.inc so that other sanitizers can benefit

[ygribov]

However, I think I have not yet understood why using strict_strings should only verify the string until the first delimiter.

I actually learned this lesson the hard way. C/C++ standards require arguments of standard string functions (strcpy, strchr`, etc.) to be strings i.e. zero-terminated arrays. But unfortunately a lot of existing code seems to ignore this rule and also call string functions on arbitrary C arrays. For example

char a[10];
memcpy(a, "11112", 5);  // Non-zero terminated
strchr(a, '2');  // Works in practice, even though a isn't a string, because strchr only checks first 5 chars

By default we'd like to not warn about uninteresting errors so we only check string prefix (unless user explicitly asks us with strict_strings runtime flag).

If I select the passed n to be smaller than strlen(str) + 1 then the test case fails, since it does not detect the overflow. What did I understand wrong?

Yes, I guess the test needs to be updated. You can take a look at how other string tests are done (e.g. test/asan/TestCases//strstr*.c, you'll probly need __asan_poison_memory_region hack).

[mrigger]

Thanks for feedback @ygribov and @kcc and sorry for the delay in addressing the issues in the change! I moved the implementation of the interceptor to lib/sanitizer_common/sanitizer_common_interceptors.inc, fixed the issues that existed when having strict_string_checks disabled, and improved the test case.

[kcc]

LGTM, but I'd like Aleksey to take a closer look.
and thanks!

[alekseyshl]

lib/sanitizer_common/sanitizer_common_interceptors.inc

475 ↗

(On Diff #91474)

#if SANITIZER_INTERCEPT_STRTOK and define it to 1 for now.

485 ↗

(On Diff #91474)

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?

489 ↗

(On Diff #91474)

What if your first call to strtok returns nullptr?

491 ↗

(On Diff #91474)

result_length + 1); and indent it to align with other arguments

494 ↗

(On Diff #91474)

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.

499 ↗

(On Diff #91474)

I'd move it below strcasestr interceptor (strstr and strcasestr are related).

test/asan/TestCases/strtok.c

1 ↗

(On Diff #91474)

Why not have strtok-1.c, strtok-2.c etc. files? Tests would be quite a bit more readable.

22 ↗

(On Diff #91474)

Why other tests do not have comments? Please split them into separate files and add a top line comment explaining what exactly is tested.

[kcc]

test/asan/TestCases/strtok.c

22 ↗

(On Diff #91474)

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.

[alekseyshl]

test/asan/TestCases/strtok.c

12 ↗

(On Diff #91474)

And, if we go with one file, please wrap these long lines like this:

RUN: ... | \
RUN: FileCheck ...

22 ↗

(On Diff #91474)

Ok, fine, one file is acceptable too, but still, comments would be much welcomed!

[mrigger]

Thanks for the feedback @alekseyshl! Check the in-line comments to see which issues I already fixed, and to which ones I still have questions.

lib/sanitizer_common/sanitizer_common_interceptors.inc

475 ↗

(On Diff #91474)

Done.

485 ↗

(On Diff #91474)

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.

489 ↗

(On Diff #91474)

Good catch. I fixed the error and added a test case.

491 ↗

(On Diff #91474)

Done.

494 ↗

(On Diff #91474)

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?

499 ↗

(On Diff #91474)

Done.

test/asan/TestCases/strtok.c

1 ↗

(On Diff #91474)

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.

12 ↗

(On Diff #91474)

The other test cases, from which I copied this pattern, are all written in one line. Should I still break the existing "convention"?

22 ↗

(On Diff #91474)

I added a comment for each test case. Additionally, the test is now only compiled once.

[alekseyshl]

Awesome, thank you for doing it!

lib/sanitizer_common/sanitizer_common_interceptors.inc

485 ↗

(On Diff #91474)

Ah, right, thanks.

494 ↗

(On Diff #91474)

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?

test/asan/TestCases/strtok.c

12 ↗

(On Diff #91474)

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 ...

[mrigger]

lib/sanitizer_common/sanitizer_common_interceptors.inc

494 ↗

(On Diff #91474)

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.

[alekseyshl]

lib/sanitizer_common/sanitizer_common_interceptors.inc

494 ↗

(On Diff #91474)

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.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

500 ↗

(On Diff #91904)

Bind star to ctx?

502 ↗

(On Diff #91904)

I'd suggest to handle !intercept_strtok case here as this will allow to reduce function nesting. It's minor though.

508 ↗

(On Diff #91904)

Strtok will return NULL or zero-terminated string so you should be able to do plain COMMON_INTERCEPTOR_READ_RANGE on str (probably only when strict_strings is off as otherwise str has been completely checked on the first call).

510 ↗

(On Diff #91904)

I agree with Alex - we shouldn't call strlen here, because if str isn't zero-terminated we risk to trigger runtime error. The best we can do is

COMMON_INTERCEPTOR_READ_STRING(ctx, str, 1);

Note that 1 is suboptimal here - we could have used strspn to find the first occurence of delimeters and use that to better estimate the length.

515 ↗

(On Diff #91904)

Same to line 508

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

?

516 ↗

(On Diff #91904)

I think else-branch shouldn't be neglected - you can safely do

COMMON_INTERCEPTOR_READ_RANGE(ctx, str, REAL(strlen) + 1)

(we know that none of delimeters has been found so str must have been scanned to the terminating 0).

519 ↗

(On Diff #91904)

Same issue with strlen here. Just do

COMMON_INTERCEPTOR_READ_STRING(ctx, delimeters, 1);

(1 is again suboptimal).

494 ↗

(On Diff #91474)

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.

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

494 ↗

(On Diff #91474)

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).

[mrigger]

Thanks again for the review @ygribov and @alekseyshl! I believe that I've addressed all the issues that you raised. @ygribov, I followed your suggestions and also split the strict_string_checks and !strict_string_checks paths since I think that the code is more readable like this.

lib/sanitizer_common/sanitizer_common_interceptors.inc

500 ↗

(On Diff #91904)

Sorry, I didn't get this comment. Should I change something in the code?

test/asan/TestCases/strtok.c

12 ↗

(On Diff #91474)

Done. Note that I also removed intercept_strtok=false since it is not necessary in the current implementation.

[alekseyshl]

lib/sanitizer_common/sanitizer_common_interceptors.inc

523 ↗

(On Diff #92059)

Shouldn't this also use COMMON_INTERCEPTOR_READ_RANGE?

529 ↗

(On Diff #92059)

If I get the earlier @ygribov's suggestion correctly, we should check the else branch too:

if (result != nullptr) {
  COMMON_INTERCEPTOR_READ_RANGE(ctx, result, REAL(strlen)(result) + 1);
} else if (str != nullptr) {
  // No delimiter were found, it's safe to assume that the entire str was scanned.
  COMMON_INTERCEPTOR_READ_RANGE(ctx, str, REAL(strlen)(str) + 1);
}

[mrigger]

Thanks for all the help! Is there still something that we should improve before merging the change?

lib/sanitizer_common/sanitizer_common_interceptors.inc

523 ↗

(On Diff #92059)

Good point! It is clearer to use COMMON_INTERCEPTOR_READ_RANGE here.

529 ↗

(On Diff #92059)

Right, I forgot about that case.

[ygribov]

Looks good, thanks!

[mrigger]

Can someone with commit access commit the change?

[alekseyshl]

I'll do it.

[mrigger]

Okay, thanks!

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc

INTERCEPTOR(char*, strcasestr, const char *s1, const char *s2) {
   return r;
 }
 
 #define INIT_STRCASESTR COMMON_INTERCEPT_FUNCTION(strcasestr);
 #else
 #define INIT_STRCASESTR
 #endif
 
+#if SANITIZER_INTERCEPT_STRTOK
+
+INTERCEPTOR(char*, strtok, char *str, const char *delimiters) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, strtok, str, delimiters);
+  if (!common_flags()->intercept_strtok) {
+    return REAL(strtok)(str, delimiters);
+  }
+  if (common_flags()->strict_string_checks) {
+    // If strict_string_checks is enabled, we check the whole first argument
+    // string on the first call (strtok saves this string in a static buffer
+    // for subsequent calls). We do not need to check strtok's result.
+    // As the delimiters can change, we check them every call.
+    if (str != nullptr) {
+      COMMON_INTERCEPTOR_READ_RANGE(ctx, str, REAL(strlen)(str) + 1);
+    }
+    COMMON_INTERCEPTOR_READ_RANGE(ctx, delimiters,
+                                  REAL(strlen)(delimiters) + 1);
+    return REAL(strtok)(str, delimiters);
+  } else {
+    // However, when strict_string_checks is disabled we cannot check the
+    // whole string on the first call. Instead, we check the result string
+    // which is guaranteed to be a NULL-terminated substring of the first
+    // argument. We also conservatively check one character of str and the
+    // delimiters.
+    if (str != nullptr) {
+      COMMON_INTERCEPTOR_READ_STRING(ctx, str, 1);
+    }
+    COMMON_INTERCEPTOR_READ_RANGE(ctx, delimiters, 1);
+    char *result = REAL(strtok)(str, delimiters);
+    if (result != nullptr) {
+      COMMON_INTERCEPTOR_READ_RANGE(ctx, result, REAL(strlen)(result) + 1);
+    } else if (str != nullptr) {
+      // No delimiter were found, it's safe to assume that the entire str was
+      // scanned.
+      COMMON_INTERCEPTOR_READ_RANGE(ctx, str, REAL(strlen)(str) + 1);
+    }
+    return result;
+  }
+}
+
+#define INIT_STRTOK COMMON_INTERCEPT_FUNCTION(strtok)
+#else
+#define INIT_STRTOK
+#endif
+
 #if SANITIZER_INTERCEPT_MEMMEM
 DECLARE_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_memmem, uptr called_pc,
                               const void *s1, SIZE_T len1, const void *s2,
                               SIZE_T len2, void *result)
 
 INTERCEPTOR(void*, memmem, const void *s1, SIZE_T len1, const void *s2,
             SIZE_T len2) {
   void *ctx;
static void InitializeCommonInterceptors() {
   INIT_STRCASECMP;
   INIT_STRNCASECMP;
   INIT_STRSTR;
   INIT_STRCASESTR;
   INIT_STRCHR;
   INIT_STRCHRNUL;
   INIT_STRRCHR;
   INIT_STRSPN;
+  INIT_STRTOK;
   INIT_STRPBRK;
   INIT_MEMSET;
   INIT_MEMMOVE;
   INIT_MEMCPY;
   INIT_MEMCHR;
   INIT_MEMCMP;
   INIT_MEMRCHR;
   INIT_MEMMEM;

compiler-rt/trunk/lib/sanitizer_common/sanitizer_flags.inc

 COMMON_FLAG(bool, strict_string_checks, false,
             "If set check that string arguments are properly null-terminated")
 COMMON_FLAG(bool, intercept_strstr, true,
             "If set, uses custom wrappers for strstr and strcasestr functions "
             "to find more errors.")
 COMMON_FLAG(bool, intercept_strspn, true,
             "If set, uses custom wrappers for strspn and strcspn function "
             "to find more errors.")
+COMMON_FLAG(bool, intercept_strtok, true,
+            "If set, uses a custom wrapper for the strtok function "
+            "to find more errors.")
 COMMON_FLAG(bool, intercept_strpbrk, true,
             "If set, uses custom wrappers for strpbrk function "
             "to find more errors.")
 COMMON_FLAG(bool, intercept_strlen, true,
             "If set, uses custom wrappers for strlen and strnlen functions "
             "to find more errors.")
 COMMON_FLAG(bool, intercept_strchr, true,
             "If set, uses custom wrappers for strchr, strchrnul, and strrchr "

compiler-rt/trunk/lib/sanitizer_common/sanitizer_platform_interceptors.h

 # define SI_UNIX_NOT_MAC 0
 #endif
 
 #define SANITIZER_INTERCEPT_STRLEN 1
 #define SANITIZER_INTERCEPT_STRNLEN SI_NOT_MAC
 #define SANITIZER_INTERCEPT_STRCMP 1
 #define SANITIZER_INTERCEPT_STRSTR 1
 #define SANITIZER_INTERCEPT_STRCASESTR SI_NOT_WINDOWS
+#define SANITIZER_INTERCEPT_STRTOK 1
 #define SANITIZER_INTERCEPT_STRCHR 1
 #define SANITIZER_INTERCEPT_STRCHRNUL SI_UNIX_NOT_MAC
 #define SANITIZER_INTERCEPT_STRRCHR 1
 #define SANITIZER_INTERCEPT_STRSPN 1
 #define SANITIZER_INTERCEPT_STRPBRK 1
 #define SANITIZER_INTERCEPT_TEXTDOMAIN SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_STRCASECMP SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_MEMSET 1

compiler-rt/trunk/test/asan/TestCases/strtok.c

+// RUN: %clang_asan %s -o %t
+
+// Test overflows with strict_string_checks
+
+// RUN: %env_asan_opts=strict_string_checks=true not %run %t test1 2>&1 | \
+// RUN:    FileCheck %s --check-prefix=CHECK1
+// RUN: %env_asan_opts=intercept_strtok=false%run %t test1 2>&1
+// RUN: %env_asan_opts=strict_string_checks=true not %run %t test2 2>&1 | \
+// RUN:    FileCheck %s --check-prefix=CHECK2
+// RUN: %env_asan_opts=intercept_strtok=false %run %t test2 2>&1
+// RUN: %env_asan_opts=strict_string_checks=true not %run %t test3 2>&1 | \
+// RUN:    FileCheck %s --check-prefix=CHECK3
+// RUN: %env_asan_opts=intercept_strtok=false %run %t test3 2>&1
+// RUN: %env_asan_opts=strict_string_checks=true %run %t test4 2>&1
+// RUN: %env_asan_opts=intercept_strtok=false %run %t test4 2>&1
+
+// Test overflows with !strict_string_checks
+// RUN: %env_asan_opts=strict_string_checks=false not %run %t test5 2>&1 | \
+// RUN:    FileCheck %s --check-prefix=CHECK5
+// RUN: %env_asan_opts=intercept_strtok=false %run %t test5 2>&1
+// RUN: %env_asan_opts=strict_string_checks=false not %run %t test6 2>&1 | \
+// RUN:    FileCheck %s --check-prefix=CHECK6
+// RUN: %env_asan_opts=intercept_strtok=false %run %t test6 2>&1
+
+
+#include <assert.h>
+#include <string.h>
+#include <sanitizer/asan_interface.h>
+
+// Check that we find overflows in the delimiters on the first call 
+// with strict_string_checks.
+void test1() {
+  char *token;
+  char s[4] = "abc";
+  char token_delimiter[2] = "b";
+  __asan_poison_memory_region ((char *)&token_delimiter[1], 2);
+  token = strtok(s, token_delimiter);
+  // CHECK1:'token_delimiter' <== Memory access at offset {{[0-9]+}} partially overflows this variable
+  assert(strcmp(token, "a") == 0);
+}
+
+// Check that we find overflows in the delimiters on the second call (str == NULL)
+// with strict_string_checks.
+void test2() {
+  char *token;
+  char s[4] = "abc";
+  char token_delimiter[2] = "b";
+  token = strtok(s, token_delimiter);
+  assert(strcmp(token, "a") == 0);
+  __asan_poison_memory_region ((char *)&token_delimiter[1], 2);
+  token = strtok(NULL, token_delimiter);
+  // CHECK2:'token_delimiter' <== Memory access at offset {{[0-9]+}} partially overflows this variable
+  assert(strcmp(token, "c") == 0);
+}
+
+// Check that we find overflows in the string (only on the first call) with strict_string_checks.
+void test3() {
+  char *token;
+  char s[4] = "abc";
+  char token_delimiter[2] = "b";
+  __asan_poison_memory_region ((char *)&s[3], 2);
+  token = strtok(s, token_delimiter);
+  // CHECK3:'s' <== Memory access at offset {{[0-9]+}} partially overflows this variable
+  assert(token == s);
+}
+
+// Check that we do not crash when strtok returns NULL with strict_string_checks.
+void test4() {
+  char *token;
+  char s[] = "";
+  char token_delimiter[] = "a";
+  token = strtok(s, token_delimiter);
+  assert(token == NULL);
+}
+
+// Check that we find overflows in the string (only on the first call) with !strict_string_checks.
+void test5() {
+  char *token;
+  char s[4] = "abc";
+  char token_delimiter[2] = "d";
+  __asan_poison_memory_region ((char *)&s[2], 2);
+  __asan_poison_memory_region ((char *)&token_delimiter[1], 2);
+  token = strtok(s, token_delimiter);
+  // CHECK5:'s' <== Memory access at offset {{[0-9]+}} partially overflows this variable
+  assert(token == s);
+}
+
+// Check that we find overflows in the delimiters (only on the first call) with !strict_string_checks.
+void test6() {
+  char *token;
+  char s[4] = "abc";
+  char token_delimiter[1] = {'d'};
+  __asan_poison_memory_region ((char *)&token_delimiter[1], 2);
+  token = strtok(s, &token_delimiter[1]);
+  // CHECK6:'token_delimiter' <== Memory access at offset {{[0-9]+}} overflows this variable
+  assert(strcmp(token, "abc") == 0);
+}
+
+int main(int argc, char **argv) {
+  if (argc != 2) return 1;
+  if (!strcmp(argv[1], "test1")) test1();
+  if (!strcmp(argv[1], "test2")) test2();
+  if (!strcmp(argv[1], "test3")) test3();
+  if (!strcmp(argv[1], "test4")) test4();
+  if (!strcmp(argv[1], "test5")) test5();
+  if (!strcmp(argv[1], "test6")) test6();
+  return 0;
+}