[sanitizer] intercept bstring functions

[kcwu]

Summary

Make all sanitizers intercept bzero, bcopy, and bcmp. Despite these functions are deprecated, they are quite popular in legacy codes.

https://github.com/google/sanitizers/issues/750

[kcc]

please add tests adjacent and similar to test/asan/TestCases/Linux/memmem_test.cc

lib/sanitizer_common/sanitizer_platform_interceptors.h

277

I think these need to be SI_LINUX, not 1.
If we try to intercept a function that does not exist on other OSes things will fail there.

[ygribov]

lib/sanitizer_common/sanitizer_platform_interceptors.h

277

Note that these functions are also available on FreeBSD and OSX.

[kcwu]

add tests and limit intercept on (Linux|FreeBSD|Mac)

[kcwu]

fix typo

[kcc]

LGTM, and thanks a lot!
Please watch the bots as the patch is potentially risky for other platforms.

Do you have llvm commit access?
If not, I can land it.

[kcwu]

In D27659#622402, @kcc wrote:

Do you have llvm commit access?
If not, I can land it.

No, I don't have commit access.

[kcc]

r289690.

[kubamracek]

Note that this started causing some memory corruptions when using ASan/TSan from SVN trunk on Darwin. There's nothing wrong with this patch, but it's a binary-compatibility issue. I'll try to see if there's a workaround, but maybe I'll have to disable SANITIZER_INTERCEPT_BZERO on Darwin. I'll keep you posted.

[ygribov]

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

[kubamracek]

In D27659#623368, @ygribov wrote:

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

A system library requires that bzero doesn't touch some specific register. This is true for the current system implementation of bzero, but not when using the interceptor.

[kcc]

Should we change

#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD || SI_MAC

to

#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD

?

[ygribov]

Should we change

If we do, please add an ugly TODO.

[thakis]

In D27659#623405, @kubabrecka wrote:

In D27659#623368, @ygribov wrote:

There's nothing wrong with this patch, but it's a binary-compatibility issue

What is the issue with ABI?

A system library requires that bzero doesn't touch some specific register. This is true for the current system implementation of bzero, but not when using the interceptor.

We think this change breaks lots of tests on our mac/asan bots (https://bugs.chromium.org/p/chromium/issues/detail?id=674435). So +1 to adding a setting for opting out of this. I'd argue that it should be off by default on Darwin until system libraries no longer make this assumption too, else asanified binaries on darwin will be broken by default.

(This currently blocks us from updating clang in chromium.)

[hans]

Reverted in r289864 so unbreak the builds until someone lands a fix.

[kubamracek]

Re-opening since the patch was reverted. Please commit again removing SI_MAC and adding a TODO with my name:

// TODO(kuba.brecka): Add SI_MAC when strncpy on Darwin supports bzero
// interceptors. https://reviews.llvm.org/D27659.
#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD

No need to change the other #defines, the issue is only with the bzero interceptor.

[kcc]

dvyukov also says that using WRAP(memset) is a bad idea, even though we have such code in other places.

[kcwu]

SANITIZER_INTERCEPT_BZERO is changed as suggestion.

[kcwu]

(rebase)

[kcc]

please see internal b/33656003 about what went wrong with this patch (other than the Mac failure).
Also I've noticed that bzer_test.cc does not actually test anything -- it passes w/o the patch.
Probably you explained it here: https://github.com/google/sanitizers/issues/750#issuecomment-266262859

[kcwu]

In D27659#624707, @kcc wrote:

please see internal b/33656003 about what went wrong with this patch (other than the Mac failure).
Also I've noticed that bzer_test.cc does not actually test anything -- it passes w/o the patch.
Probably you explained it here: https://github.com/google/sanitizers/issues/750#issuecomment-266262859

bzero_test.cc is easy to fix (adding -std=c99). But I am not sure how to properly fix the problem of WRAP though. Does following approach work?

Proposal: a separate CL dedicated for WRAP() issue.

1. extract the body of "INTERCEPTOR(void*, memcpy, void *dst, const void *src, uptr size) {"

to a macro.

2. replace all WRAP(memcpy) by the macro.
3. repeat for others functions WRAP(*).

Any suggestions?

[kcc]

In D27659#625093, @kcwu wrote:

In D27659#624707, @kcc wrote:

please see internal b/33656003 about what went wrong with this patch (other than the Mac failure).
Also I've noticed that bzer_test.cc does not actually test anything -- it passes w/o the patch.
Probably you explained it here: https://github.com/google/sanitizers/issues/750#issuecomment-266262859

bzero_test.cc is easy to fix (adding -std=c99). But I am not sure how to properly fix the problem of WRAP though. Does following approach work?

Proposal: a separate CL dedicated for WRAP() issue.

1. extract the body of "INTERCEPTOR(void*, memcpy, void *dst, const void *src, uptr size) {"

to a macro.

2. replace all WRAP(memcpy) by the macro.
3. repeat for others functions WRAP(*).

Any suggestions?

Yes, this should work.
I would actually suggest that you do this only for the functions you change now.
If we need to fix the older interceptors, it's our yak to shave.

[ygribov]

What's the said problem with wrapping btw?

[kcc]

In D27659#625206, @ygribov wrote:

What's the said problem with wrapping btw?

First, it creates yet another frame. and may cause confusing stack traces.
Second, it adds a call to memset to the tsan run-time, and we have a test that checks for absence of such calls.

[vitalybuka]

FYI: https://reviews.llvm.org/D28039

lib/msan/msan_interceptors.cc

 INTERCEPTOR(void *, memmove, void *dest, const void *src, SIZE_T n) {
   return __msan_memmove(dest, src, n);
 }
 
 INTERCEPTOR(void *, memset, void *s, int c, SIZE_T n) {
   return __msan_memset(s, c, n);
 }
 
-INTERCEPTOR(void *, bcopy, const void *src, void *dest, SIZE_T n) {
-  return __msan_memmove(dest, src, n);
-}
-
 INTERCEPTOR(int, posix_memalign, void **memptr, SIZE_T alignment, SIZE_T size) {
   GET_MALLOC_STACK_TRACE;
   CHECK_EQ(alignment & (alignment - 1), 0);
   CHECK_NE(memptr, 0);
   *memptr = MsanReallocate(&stack, nullptr, size, alignment, false);
   CHECK_NE(*memptr, 0);
   __msan_unpoison(memptr, sizeof(*memptr));
   return 0;
void InitializeInterceptors() {
   INTERCEPT_FUNCTION(fread);
   MSAN_MAYBE_INTERCEPT_FREAD_UNLOCKED;
   INTERCEPT_FUNCTION(readlink);
   INTERCEPT_FUNCTION(memcpy);
   INTERCEPT_FUNCTION(memccpy);
   INTERCEPT_FUNCTION(mempcpy);
   INTERCEPT_FUNCTION(memset);
   INTERCEPT_FUNCTION(memmove);
-  INTERCEPT_FUNCTION(bcopy);
   INTERCEPT_FUNCTION(wmemset);
   INTERCEPT_FUNCTION(wmemcpy);
   INTERCEPT_FUNCTION(wmempcpy);
   INTERCEPT_FUNCTION(wmemmove);
   INTERCEPT_FUNCTION(strcpy);  // NOLINT
   INTERCEPT_FUNCTION(stpcpy);  // NOLINT
   INTERCEPT_FUNCTION(strdup);
   MSAN_MAYBE_INTERCEPT___STRDUP;

lib/sanitizer_common/sanitizer_common_interceptors.inc

 INTERCEPTOR(void *, __bzero, void *block, uptr size) {
   return WRAP(memset)(block, 0, size);
 }
 #define INIT___BZERO COMMON_INTERCEPT_FUNCTION(__bzero);
 #else
 #define INIT___BZERO
 #endif  // SANITIZER_INTERCEPT___BZERO
 
+#if SANITIZER_INTERCEPT_BZERO
+DECLARE_REAL_AND_INTERCEPTOR(void *, memset, void *, int, uptr)
+
+INTERCEPTOR(void, bzero, void *block, uptr size) {
+  WRAP(memset)(block, 0, size);
+}
+#define INIT_BZERO COMMON_INTERCEPT_FUNCTION(bzero);
+#else
+#define INIT_BZERO
+#endif  // SANITIZER_INTERCEPT_BZERO
+
+#if SANITIZER_INTERCEPT_BCOPY
+DECLARE_REAL_AND_INTERCEPTOR(void *, memmove, void *, const void *, uptr)
+
+INTERCEPTOR(void, bcopy, const void *src, void *dest, uptr size) {
+  WRAP(memmove)(dest, src, size);
+}
+#define INIT_BCOPY COMMON_INTERCEPT_FUNCTION(bcopy);
+#else
+#define INIT_BCOPY
+#endif  // SANITIZER_INTERCEPT_BCOPY
+
+#if SANITIZER_INTERCEPT_BCMP
+DECLARE_REAL_AND_INTERCEPTOR(int, memcmp, const void *, const void *, uptr)
+
+INTERCEPTOR(int, bcmp, const void *s1, const void *s2, uptr size) {
+  return WRAP(memcmp)(s1, s2, size);
+}
+#define INIT_BCMP COMMON_INTERCEPT_FUNCTION(bcmp);
+#else
+#define INIT_BCMP
+#endif  // SANITIZER_INTERCEPT_BCMP
+
 #if SANITIZER_INTERCEPT_FTIME
 INTERCEPTOR(int, ftime, __sanitizer_timeb *tp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, ftime, tp);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(ftime)(tp);
static void InitializeCommonInterceptors() {
   INIT_LISTXATTR;
   INIT_GETXATTR;
   INIT_GETRESID;
   INIT_GETIFADDRS;
   INIT_IF_INDEXTONAME;
   INIT_CAPGET;
   INIT_AEABI_MEM;
   INIT___BZERO;
+  INIT_BZERO;
+  INIT_BCOPY;
+  INIT_BCMP;
   INIT_FTIME;
   INIT_XDR;
   INIT_TSEARCH;
   INIT_LIBIO_INTERNALS;
   INIT_FOPEN;
   INIT_FOPEN64;
   INIT_OPEN_MEMSTREAM;
   INIT_OBSTACK;

lib/sanitizer_common/sanitizer_platform_interceptors.h

#define SANITIZER_INTERCEPT_IF_INDEXTONAME \
   SI_FREEBSD || SI_LINUX_NOT_ANDROID || SI_MAC
 #define SANITIZER_INTERCEPT_CAPGET SI_LINUX_NOT_ANDROID
 #if SI_LINUX && defined(__arm__)
 #define SANITIZER_INTERCEPT_AEABI_MEM 1
 #else
 #define SANITIZER_INTERCEPT_AEABI_MEM 0
 #endif
 #define SANITIZER_INTERCEPT___BZERO SI_MAC
+// TODO(kuba.brecka): Add SI_MAC when strncpy on Darwin supports bzero
+// interceptors. https://reviews.llvm.org/D27659.
+#define SANITIZER_INTERCEPT_BZERO SI_LINUX || SI_FREEBSD
+#define SANITIZER_INTERCEPT_BCOPY SI_LINUX || SI_FREEBSD || SI_MAC
+#define SANITIZER_INTERCEPT_BCMP SI_LINUX || SI_FREEBSD || SI_MAC
 #define SANITIZER_INTERCEPT_FTIME !SI_FREEBSD && SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_XDR SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_TSEARCH SI_LINUX_NOT_ANDROID || SI_MAC
 #define SANITIZER_INTERCEPT_LIBIO_INTERNALS SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_FOPEN SI_NOT_WINDOWS
 #define SANITIZER_INTERCEPT_FOPEN64 SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_OPEN_MEMSTREAM SI_LINUX_NOT_ANDROID
 #define SANITIZER_INTERCEPT_OBSTACK SI_LINUX_NOT_ANDROID

test/asan/TestCases/Linux/bcmp_test.cc

+// RUN: %clangxx_asan  %s -o %t
+// RUN: not %run %t   2>&1 | FileCheck %s --check-prefix=A1
+// RUN: not %run %t 1 2>&1 | FileCheck %s --check-prefix=A2
+// RUN: %env_asan_opts=intercept_memcmp=0 %run %t
+
+#include <strings.h>
+int main(int argc, char **argv) {
+  char a1[] = {1, 2, 3, 4, 5, 6, 7, 8};
+  char a2[] = {3, 4, 5, 6, 7, 8, 9};
+  int res;
+  if (argc == 1)
+    res = bcmp(a1, a2, sizeof(a1));  // BOOM
+  else
+    res = bcmp(a2, a1, sizeof(a1));  // BOOM
+  // A1: AddressSanitizer: stack-buffer-overflow
+  // A1: {{#0.*memcmp}}
+  // A1: 'a2' <== Memory access at offset
+  //
+  // A2: AddressSanitizer: stack-buffer-overflow
+  // A2: {{#0.*memcmp}}
+  // A2: 'a2' <== Memory access at offset
+  return res == 0;
+}

test/asan/TestCases/Linux/bcopy_test.cc

+// RUN: %clangxx_asan  %s -o %t
+// RUN: not %run %t   2>&1 | FileCheck %s --check-prefix=A1
+// RUN: not %run %t 1 2>&1 | FileCheck %s --check-prefix=A2
+// RUN: %env_asan_opts=replace_intrin=0 %run %t
+
+#include <strings.h>
+int main(int argc, char **argv) {
+  char a1[] = {1, 2, 3, 4, 5, 6, 7, 8};
+  char a2[] = {3, 4, 5, 6, 7, 8, 9};
+  if (argc == 1)
+    bcopy(a1, a2, sizeof(a1));  // BOOM
+  else
+    bcopy(a2, a1, sizeof(a1));  // BOOM
+  // A1: AddressSanitizer: stack-buffer-overflow
+  // A1: {{#0.*memmove}}
+  // A1: 'a2' <== Memory access at offset
+  //
+  // A2: AddressSanitizer: stack-buffer-overflow
+  // A2: {{#0.*memmove}}
+  // A2: 'a2' <== Memory access at offset
+  return 0;
+}

test/asan/TestCases/Linux/bzero_test.cc

+// RUN: %clangxx_asan  %s -o %t
+// RUN: not %run %t   2>&1 | FileCheck %s --check-prefix=A1
+// RUN: %env_asan_opts=replace_intrin=0 %run %t
+
+#include <strings.h>
+int main(int argc, char **argv) {
+  char a1[] = {1, 2, 3, 4, 5, 6, 7, 8};
+  bzero(a1, sizeof(a1) + 1);  // BOOM
+  // A1: AddressSanitizer: stack-buffer-overflow
+  // A1: {{#0.*memset}}
+  // A1: 'a1' <== Memory access at offset
+  return 0;
+}