[sanitizer][asan][win][msvc] Correct interception of strdup on windows, additionally correclty intercept memmove/memcpy on i386 windows.

[barcharcraz]

Summary

We (Microsoft) have a bunch of changes on top of LLVM-14 to enable asan to work on windows with msvc. We'd like to start working in the open, on LLVM main, but our branch has diverged somewhat. We've thrown some changesets over the wall previously and I'm going to be resuming the effort.

In any event this is a very self-contained set of fixes to interception. It intercepts _strdup on windows instead of strdup and changes the way memcpy is intercepted on I386 windows. For strdup windows does not define an actual strdup function, only _strdup (because strdup is in POSIX, not C, until C23). Note that _strdup_dbg ends up calling _strdup. For memcpy the behavior is changed on I386 windows to intercept with asan's version of memmove. On I386 windows memcpy and memmove are actually separate functions, even though they have identical implementations, thus memcpy needs to be intercepted with asan's memmove to avoid having asan check for overlapping ranges, which could break programs relying on this behavior (this is a bit of a judgement call though, we think preserving the memmoving behavior of memcpy makes asan more useful for catching real bugs, but our memcpy isn't actually documented as being memmove on all architectures.)

[asan][win][msvc] Intercept memcpy in addition to memmove on i386 windows.

[asan][win][test] Add tests ensuring memcpy and memmove are properly intercepted on windows

[barcharcraz]

• clang-format

[vitalybuka]

Please split patches, changes are unrelated.

compiler-rt/lib/asan/asan_interceptors.cpp

466 ↗

(On Diff #519198)

please use:
#if WIndows
#define strdup _strdup
#endif

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

907

isn't this constant for the platform?

[barcharcraz]

remove the changes to strdup from this diff.

[barcharcraz]

compiler-rt/lib/asan/asan_interceptors.cpp

466 ↗

(On Diff #519198)

This is a little dangerous if <string.h> ends up getting included in this file, since we define strdup to _strdup_dbg or _strdup there (and in asan we want to intercept _strdup even in debug mode).

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

907

I'm not 100% sure for armv7. I think it's not the case for IA64, but .... it's IA64, we don't really support it anymore.

[barcharcraz]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

907

are you saying that you'd prefer to have the preprocessor logic on lines 202-210 inlined here (and at other usages)

[vitalybuka]

memmove and strdup should be two different patches

compiler-rt/lib/asan/asan_interceptors.cpp

466 ↗

(On Diff #519198)

Is this even would compile if you include string.h here?

We are doing this for other platforms, and I would like consistency.
and i don't like repeating this code 3 times

alternative approach:
in a separate patch extract common code from 2 functions
in this patch use the code for the third one

470–481 ↗

(On Diff #519198)

Move GET_STACK_TRACE_MALLOC and extract this
You need REAL(memcpy) as parameters

485 ↗

(On Diff #519198)

Please introduce ASAN_INTERCEPT_STRDUP and ASAN_INTERCEPT__STRDUP and use instead of SANITIZER_WINDOWS

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

906–907

I am asking to remove selected condition condition

[barcharcraz]

• revert changes to INIT_MEMCPY as they are redundant
• clang format

[barcharcraz]

compiler-rt/lib/asan/asan_interceptors.cpp

466 ↗

(On Diff #519198)

I ended up using push/pop macro, and those changes appear in https://reviews.llvm.org/D149876 (which contains only the strdup changes, as requested).

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

906–907

Ah I see, it's redundant with the code already in the memcpy interceptor. I've reverted the changes to INIT_MEMCPY.

[alvinhochun]

Thanks for adding me in the loop. The change itself looks fine, though I haven't tried it on MinGW-w64 target yet. Some comments on the tests:

• It seems the two tests memcpy_sanity.cpp and memmove_sanity.cpp does the same thing for the most part. Can they be merged into one single test file and switch between testing memmove and memcpy by checking argc/argv?
• These tests will not work on MinGW-w64 as is because of the use of clang-cl command line flags. I have been fixing the asan tests to work on MinGW-w64 targets (see for example D150269) and there are now nightly builds that run asan tests. Please add // UNSUPPORTED: target={{.*-windows-gnu}} to disable the new tests for now. (It would be nice if you can make the test compatible with MinGW-w64, but that might be asking for too much.)

[barcharcraz]

In D149769#4337197, @alvinhochun wrote:

Thanks for adding me in the loop. The change itself looks fine, though I haven't tried it on MinGW-w64 target yet. Some comments on the tests:

• It seems the two tests memcpy_sanity.cpp and memmove_sanity.cpp does the same thing for the most part. Can they be merged into one single test file and switch between testing memmove and memcpy by checking argc/argv?
• These tests will not work on MinGW-w64 as is because of the use of clang-cl command line flags. I have been fixing the asan tests to work on MinGW-w64 targets (see for example D150269) and there are now nightly builds that run asan tests. Please add // UNSUPPORTED: target={{.*-windows-gnu}} to disable the new tests for now. (It would be nice if you can make the test compatible with MinGW-w64, but that might be asking for too much.)

Honestly I do want to make them work with mingw, but there's an additional complication ....

Our internal test harness treats "%clang_cl_asan" as "real" cl.exe, so clang style flags won't actually work at all (although in this case I had to add the no-builtin flags anyway). In the end I'll probably add some kind of conditional macro in lit to allow specifying separate cl and clang style flags. For now, I agree that just making them unsupported on -gnu would be easiest.

Also, on mingw could lit make clang_cl_asan work by just telling it to build for the gnu target instead of the msvc one? I'm not sure how well this is supported.

[barcharcraz]

• Make memcpy and memmove tests unsupported on mingw

[barcharcraz]

• merge the memcpy and memmove tests
• clang-format

[alvinhochun]

compiler-rt/test/asan/TestCases/Windows/memcpy_memmove_sanity.cpp

1–13

This will allow the test to run on mingw-w64. @vitalybuka how do you feel about this?

[strega-nil]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

204

In line with our conversation yesterday, we'd prefer to not lose the memcpy/memmove distinction when it comes to diagnostics; we still want to diagnose:

cxx
int main() {
  char buf[] = "Hello!";
  memcpy(&buf[1], &buf[0], (sizeof buf) - 1);
}

[strega-nil]

Probably good to rename the title as well.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 #define utimensat __utimensat_time64
 #define utimes __utimes_time64
 #define utime __utime64
 #define wait3 __wait3_time64
 #define wait4 __wait4_time64
 #endif
 
 // Platform-specific options.
-#if SANITIZER_APPLE
-#define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE 0
-#elif SANITIZER_WINDOWS64
-#define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE 0
+#if SANITIZER_APPLE || (SANITIZER_WINDOWS && SANITIZER_X64)
+#  define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE 0
+#  define PLATFORM_MEMCPY_MEMMOVE_CAN_BE_FOLDED 0
+#elif SANITIZER_WINDOWS && SANITIZER_I386
+#  define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE 1
+#  define PLATFORM_MEMCPY_MEMMOVE_CAN_BE_FOLDED 1
 #else
 #define PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE 1
+#  define PLATFORM_MEMCPY_MEMMOVE_CAN_BE_FOLDED 0
 #endif  // SANITIZER_APPLE
 
 #ifndef COMMON_INTERCEPTOR_INITIALIZE_RANGE
 #define COMMON_INTERCEPTOR_INITIALIZE_RANGE(p, size) {}
 #endif
 
 #ifndef COMMON_INTERCEPTOR_UNPOISON_PARAM
 #define COMMON_INTERCEPTOR_UNPOISON_PARAM(count) {}

 #if SANITIZER_INTERCEPT_MEMCPY
 INTERCEPTOR(void *, memcpy, void *dst, const void *src, uptr size) {
   // On OS X, calling internal_memcpy here will cause memory corruptions,
   // because memcpy and memmove are actually aliases of the same
   // implementation.  We need to use internal_memmove here.
   // N.B.: If we switch this to internal_ we'll have to use internal_memmove
   // due to memcpy being an alias of memmove on OS X.
   void *ctx;
-#if PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE
-    COMMON_INTERCEPTOR_MEMCPY_IMPL(ctx, dst, src, size);
-#else
-    COMMON_INTERCEPTOR_MEMMOVE_IMPL(ctx, dst, src, size);
-#endif
+#  ifndef PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE
+#    error "Undefined"
+#  endif
+
+#  ifndef PLATFORM_MEMCPY_MEMMOVE_CAN_BE_FOLDED
+#    error "Undefined"
+#  endif
+#  if PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE && \
+      !PLATFORM_MEMCPY_MEMMOVE_CAN_BE_FOLDED
+#  else
+#  endif
 }
 
 #define INIT_MEMCPY                                  \
   do {                                               \
     if (PLATFORM_HAS_DIFFERENT_MEMCPY_AND_MEMMOVE) { \
       COMMON_INTERCEPT_FUNCTION(memcpy);             \
     } else {                                         \
       ASSIGN_REAL(memcpy, memmove);                  \
     }                                                \
     CHECK(REAL(memcpy));                             \
   } while (false)
 
 #else
 #define INIT_MEMCPY
 #endif
 
 #if SANITIZER_INTERCEPT_MEMCMP
 DECLARE_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_memcmp, uptr called_pc,
                               const void *s1, const void *s2, uptr n,
                               int result)
 
 // Common code for `memcmp` and `bcmp`.
 int MemcmpInterceptorCommon(void *ctx,
                             int (*real_fn)(const void *, const void *, uptr),
                             const void *a1, const void *a2, uptr size) {
   if (common_flags()->intercept_memcmp) {

compiler-rt/test/asan/TestCases/Windows/memcpy_memmove_sanity.cpp

+// RUN: %clang_cl_asan %s -Fe%t.icf.ref /clang:-fno-builtin-memcpy /clang:-fno-builtin-memmove /link /OPT:ICF /OPT:REF
+// RUN: %clang_cl_asan %s -Fe%t.noicf.ref /clang:-fno-builtin-memcpy /clang:-fno-builtin-memmove /link /OPT:NOICF /OPT:REF
+// RUN: %clang_cl_asan %s -Fe%t.icf.noref /clang:-fno-builtin-memcpy /clang:-fno-builtin-memmove /link /OPT:ICF /OPT:NOREF
+// RUN: %clang_cl_asan %s -Fe%t.noicf.noref /clang:-fno-builtin-memcpy /clang:-fno-builtin-memmove /link /OPT:NOICF /OPT:NOREF
+// RUN: not %run %t.icf.ref memcpy 2>&1 | FileCheck %s
+// RUN: not %run %t.noicf.ref memcpy 2>&1 | FileCheck %s
+// RUN: not %run %t.icf.noref memcpy 2>&1 | FileCheck %s
+// RUN: not %run %t.noicf.noref memcpy 2>&1 | FileCheck %s
+// RUN: not %run %t.icf.ref memmove 2>&1 | FileCheck %s
+// RUN: not %run %t.noicf.ref memmove 2>&1 | FileCheck %s
+// RUN: not %run %t.icf.noref memmove 2>&1 | FileCheck %s
+// RUN: not %run %t.noicf.noref memmove 2>&1 | FileCheck %s
+// UNSUPPORTED: target={{.*-windows-gnu}}
+
+#include <stdlib.h>
+#include <string.h>
+
+int main(int argc, char **argv) {
+  int *buf = new int[4];
+
+  memmove(buf + 1, buf, 3 * sizeof(int));
+  memcpy(buf + 1, buf, 3 * sizeof(int));
+
+  delete[] buf;
+  if (argc != 2) {
+    abort();
+  }
+  if (strcmp(argv[1], "memcpy") == 0) {
+    memcpy(buf + 1, buf, 3 * sizeof(int));
+  } else if (strcmp(argv[1], "memmove") == 0) {
+    memmove(buf + 1, buf, 3 * sizeof(int));
+  }
+  // CHECK: AddressSanitizer: heap-use-after-free
+
+  return 0;
+}