SIGSEGV in Sanitizer INTERCEPTOR of strstr function.

[RitanyaB]

Summary

This is a segmentation fault in INTERCEPTOR function on a special edge
case of strstr libc call. When 'Haystack'(main string to be examined) is
NULL and 'needle'(sub-string to be searched in 'Haystack') is an empty
string then it hits a SEGV while using sanitizers and as a 'string not
found' case otherwise.

[vitalybuka]

Nice find!

Can you please extend llvm-project/compiler-rt/test/sanitizer_common/TestCases/strstr.c

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

594

Could you fix instead internal_strstr and StrstrCheck?

[RitanyaB]

In D115919#3199280, @vitalybuka wrote:

Nice find!

Can you please extend llvm-project/compiler-rt/test/sanitizer_common/TestCases/strstr.c

I have added the fix to internal_strstr and extended the test case well. In libc's implementation of strstr, the empty needle string check is done at the very beginning and returns haystack if true. In accordance to that, I have added the fix in INTERCEPTOR for an early check rather than StrstrCheck where string length is being calculated. Fixing StrstrCheck would require changing the behavior of internal_strlen and that would not be consistent with the behavior of libc's implementation of strlen.

[vitalybuka]

update

[vitalybuka]

hm, according to this it's Undefined Behavior
Sanitizer should probably crash in this case, at least with strict_string_checks
https://en.cppreference.com/w/c/string/byte/strstr

If this version is OK, I will land it?

compiler-rt/test/sanitizer_common/TestCases/strstr.c

2

why this one is here?

[soumitra]

In D115919#3207280, @vitalybuka wrote:

hm, according to this it's Undefined Behavior
Sanitizer should probably crash in this case, at least with strict_string_checks
https://en.cppreference.com/w/c/string/byte/strstr

Instead, why should the sanitizer not diagnose the use of undefined behavior?
It would be good for the sanitizer to emit a diagnostic about "undefined behavior" (probably a warning). Code that leads to undefined behavior is generally not portable and is a bad programing practice that should be avoided.

[soumitra]

compiler-rt/lib/sanitizer_common/sanitizer_libc.cpp

221–223

Isn't checking for needle[0] efficient than calling a function?
It also matches the corresponding implementation in glibc.

[vitalybuka]

In D115919#3207475, @soumitra wrote:

In D115919#3207280, @vitalybuka wrote:

hm, according to this it's Undefined Behavior
Sanitizer should probably crash in this case, at least with strict_string_checks
https://en.cppreference.com/w/c/string/byte/strstr

Instead, why should the sanitizer not diagnose the use of undefined behavior?
It would be good for the sanitizer to emit a diagnostic about "undefined behavior" (probably a warning). Code that leads to undefined behavior is generally not portable and is a bad programing practice that should be avoided.

SEGV in this case should be straightforward
but if we check here but not in the rest of COMMON_INTERCEPTOR_READ_STRING usage, it will be inconsistent

compiler-rt/lib/sanitizer_common/sanitizer_libc.cpp

221–223

most likely case is != 0 and internal_strlen is going to be inlined anyway producing same code

[RitanyaB]

In D115919#3207280, @vitalybuka wrote:

hm, according to this it's Undefined Behavior
Sanitizer should probably crash in this case, at least with strict_string_checks
https://en.cppreference.com/w/c/string/byte/strstr

If this version is OK, I will land it?

I verified the changes and it looks good to me. Please go ahead with the commit. Thanks.

[paulkirth]

Hi, we've noticed a breakage in Fuchsia's canaries for LLVM. This commit seems to be responsible, as it fails on the strstr.c tests or aarch64-linux.

It seems to affect all sanitizers in our build. Sorry this took so long to report, but other breakages hid this for most of the day.

If this is not easy to address, can you revert until a fix is ready?

Script:
--
: 'RUN: at line 1';      /b/s/w/ir/x/w/staging/llvm_build/./bin/clang  -gline-tables-only -fsanitize=address  --unwindlib=libunwind -static-libgcc -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta  -funwind-tables --sysroot=/b/s/w/ir/x/w/cipd/linux  -ldl /b/s/w/ir/x/w/llvm-project/compiler-rt/test/sanitizer_common/TestCases/strstr.c -o /b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-aarch64-unknown-linux-gnu-bins/compiler-rt/test/sanitizer_common/asan-aarch64-Linux/Output/strstr.c.tmp &&  /b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-aarch64-unknown-linux-gnu-bins/compiler-rt/test/sanitizer_common/asan-aarch64-Linux/Output/strstr.c.tmp 2>&1
--
Exit Code: 1

Command Output (stdout):
--
AddressSanitizer:DEADLYSIGNAL
=================================================================
==30792==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0xffff9ad9c328 bp 0xfffff746b950 sp 0xfffff746b950 T0)
==30792==The signal is caused by a READ memory access.
==30792==Hint: address points to the zero page.
libunwind: Unsupported .eh_frame_hdr version
    #0 0xffff9ad9c328 in strstr (/lib/aarch64-linux-gnu/libc.so.6+0x7c328) (BuildId: 53f40c1d2f3739ae017dcdcef1a17314786e3709)
    #1 0x26f8c8 in strstr ../staging/llvm_build/runtimes/runtimes-aarch64-unknown-linux-gnu-bins/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc
    #2 0xffff9ad406dc in __libc_start_main (/lib/aarch64-linux-gnu/libc.so.6+0x206dc) (BuildId: 53f40c1d2f3739ae017dcdcef1a17314786e3709)
    #3 0x25a554 in _start (/b/s/w/ir/x/w/staging/llvm_build/runtimes/runtimes-aarch64-unknown-linux-gnu-bins/compiler-rt/test/sanitizer_common/asan-aarch64-Linux/Output/strstr.c.tmp+0x25a554) (BuildId: 5196c7df40b5e4c2)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/lib/aarch64-linux-gnu/libc.so.6+0x7c328) (BuildId: 53f40c1d2f3739ae017dcdcef1a17314786e3709) in strstr
==30792==ABORTING

--

[vitalybuka]

I am going to revert and investigate tomorrow.

[paulkirth]

filed https://github.com/llvm/llvm-project/issues/53031 to track

[RitanyaB]

In D115919#3224323, @paulkirth wrote:

filed https://github.com/llvm/llvm-project/issues/53031 to track

Hi, any updates on the fix so far? Is there anything I can do to get this patch committed?

[paulkirth]

Hi, I closed the issue on github, since we hadn't seen movement here or the issue show up at for a month. I was just reporting the breakage, and didn't investigate further after the revert. Maybe check with @vitalybuka if you need more support/information?

I think this should show up in any config when compiling w/ asan. So, AFICT just building for a Linux x64 target should reproduce the SEGV for you. The most important bit of Fuchsia's toolchain build is that we use a build a stage 2 compiler, so building clang, and using that just built clang + runtimes should give you something very close to our config if you're having trouble reproducing.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 #else
 #define INIT_STRCASECMP
 #define INIT_STRNCASECMP
 #endif
 
 #if SANITIZER_INTERCEPT_STRSTR || SANITIZER_INTERCEPT_STRCASESTR
 static inline void StrstrCheck(void *ctx, char *r, const char *s1,
                                const char *s2) {
-    uptr len1 = internal_strlen(s1);
-    uptr len2 = internal_strlen(s2);
-    COMMON_INTERCEPTOR_READ_STRING(ctx, s1, r ? r - s1 + len2 : len1 + 1);
-    COMMON_INTERCEPTOR_READ_RANGE(ctx, s2, len2 + 1);
+  if (len2 == 0 && !common_flags()->strict_string_checks)
+    return;
+  uptr len1 = internal_strlen(s1);
+  COMMON_INTERCEPTOR_READ_STRING(ctx, s1, r ? r - s1 + len2 : len1 + 1);
 }
 #endif
 
 #if SANITIZER_INTERCEPT_STRSTR
 
 DECLARE_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_strstr, uptr called_pc,
                               const char *s1, const char *s2, char *result)
 
 INTERCEPTOR(char*, strstr, const char *s1, const char *s2) {
   if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
     return internal_strstr(s1, s2);
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strstr, s1, s2);
   char *r = REAL(strstr)(s1, s2);
   if (common_flags()->intercept_strstr)
     StrstrCheck(ctx, r, s1, s2);
   CALL_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_strstr, GET_CALLER_PC(), s1,
                              s2, r);
   return r;

compiler-rt/lib/sanitizer_common/sanitizer_libc.cpp

 uptr internal_strnlen(const char *s, uptr maxlen) {
   uptr i = 0;
   while (i < maxlen && s[i]) i++;
   return i;
 }
 
 char *internal_strstr(const char *haystack, const char *needle) {
   // This is O(N^2), but we are not using it in hot places.
-  uptr len1 = internal_strlen(haystack);
   uptr len2 = internal_strlen(needle);
+  if (len2 == 0)
+    return const_cast<char *>(haystack);
+  uptr len1 = internal_strlen(haystack);
   if (len1 < len2) return nullptr;
   for (uptr pos = 0; pos <= len1 - len2; pos++) {
     if (internal_memcmp(haystack + pos, needle, len2) == 0)
       return const_cast<char *>(haystack) + pos;
   }
   return nullptr;
 }
 

compiler-rt/test/sanitizer_common/TestCases/strstr.c

 // RUN: %clang %s -o %t && %run %t 2>&1
 
 #include <assert.h>
 #include <string.h>
 int main(int argc, char **argv) {
   char *r = 0;
   char s1[] = "ab";
   char s2[] = "b";
   r = strstr(s1, s2);
   assert(r == s1 + 1);
+  char *s3 = NULL;
+  char *s4 = "";
+  char *p = strstr(s3, s4);
+  assert(p == NULL);
   return 0;
 }