[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

#if SANITIZER_INTERCEPT_STRTOK and define it to 1 for now.

485

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

What if your first call to strtok returns nullptr?

491

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

494

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

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

test/asan/TestCases/strtok.c

2

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

23

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

23

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

13

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

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

23

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

Done.

485

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

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

491

Done.

494

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

Done.

test/asan/TestCases/strtok.c

2

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.

13

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

23

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

Ah, right, thanks.

494

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

13

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

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

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

494

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.

500

Bind star to ctx?

502

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

508

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

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

Same to line 508

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

?

516

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

Same issue with strlen here. Just do

COMMON_INTERCEPTOR_READ_STRING(ctx, delimeters, 1);

(1 is again suboptimal).

[ygribov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

494

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

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

test/asan/TestCases/strtok.c

13

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

Shouldn't this also use COMMON_INTERCEPTOR_READ_RANGE?

529

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

Good point! It is clearer to use COMMON_INTERCEPTOR_READ_RANGE here.

529

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!

lib/sanitizer_common/sanitizer_common_interceptors.inc

CALL_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_strstr, GET_CALLER_PC(), s1,
                              s2, r);
   return r;
 }
 
 #define INIT_STRSTR COMMON_INTERCEPT_FUNCTION(strstr);
 #else
 #define INIT_STRSTR
 #endif
 
 #if SANITIZER_INTERCEPT_STRCASESTR
 
 DECLARE_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_strcasestr, uptr called_pc,
                               const char *s1, const char *s2, char *result)
 
 INTERCEPTOR(char*, strcasestr, const char *s1, const char *s2) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strcasestr, s1, s2);
   char *r = REAL(strcasestr)(s1, s2);
   if (common_flags()->intercept_strstr)
     StrstrCheck(ctx, r, s1, s2);
   CALL_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_strcasestr, GET_CALLER_PC(),
                              s1, s2, r);
   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) {
+    char* result;
+    if (str == nullptr) {
+      // We can only check the first argument upon the first
+      // invocation of a token chain since strtok saves it in a
+      // static buffer for subsequent invocations.
+      result = REAL(strtok)(str, delimiters);
+    } else {
+      uptr str_length = REAL(strlen)(str);
+      result = REAL(strtok)(str, delimiters);
+      if (result != nullptr) {
+        uptr result_length = REAL(strlen)(result);
+        COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, str, str_length,
+                                              result_length + 1);
+      }
+    }
+    uptr del_length = REAL(strlen)(delimiters);
+    COMMON_INTERCEPTOR_READ_STRING_OF_LEN(ctx, delimiters, del_length, 1);
+    return result;
+  }
+  return REAL(strtok)(str, delimiters);
+}
+
+#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;

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 "

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

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 | FileCheck %s --check-prefix=CHECK1
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=false %run %t test1 2>&1
+// RUN: %env_asan_opts=strict_string_checks=true not %run %t test2 2>&1 | FileCheck %s --check-prefix=CHECK2
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=false %run %t test2 2>&1
+// RUN: %env_asan_opts=strict_string_checks=true not %run %t test3 2>&1 | FileCheck %s --check-prefix=CHECK3
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=false %run %t test3 2>&1
+// RUN: %env_asan_opts=strict_string_checks=true not %run %t test3 2>&1
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=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 | FileCheck %s --check-prefix=CHECK5
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=false %run %t test5 2>&1
+// RUN: %env_asan_opts=strict_string_checks=false not %run %t test6 2>&1 | FileCheck %s --check-prefix=CHECK6
+// RUN: %env_asan_opts=intercept_strtok=false:intercept_strlen=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;
+}