[compiler-rt] Intercept the bcmp() function.

[courbet]

Summary

I have not introduced a separate hook for bcmp() as I don't think there
should be any reason for a sanitizer to treat it differently from memcmp().

This is only enabled when building on POSIX with GNU extensions.

Context: this is to avoid losing coverage when emitting bcmp() == 0 instead
of memcmp() == 0 in llvm, see https://reviews.llvm.org/D56593.

[krytarowski]

bcmp is BSD and it first appeard in 4.2BSD... it's not specific to GNU source but compat layer for BSD software.

Please enable it for NetBSD.

[courbet]

Please enable it for NetBSD.

Done, thanks.

[courbet]

• Enable bcmp interception on NetBSD.

[krytarowski]

Please enable the tests for NetBSD as well.

<strings.h> should be included unconditionally.

[courbet]

• Unconditionally test bcmp() in weak_hook_test

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

874 ↗

(On Diff #187325)

Can we reuse code with memcmp(3)? bcmp(3) is a subset of memcmp(3).

[courbet]

In D58379#1401986, @krytarowski wrote:

Please enable the tests for NetBSD as well.

<strings.h> should be included unconditionally.

Just to make sure I understand (I'm not familiar with this code base):

• Will the tests in Posix/ be compiled only for platforms supporting this ? Is that why <strings.h> can be included unconditionally ?
• lib/asan/tests/asan_mem_test.cc will be compiled for platforms without <strings.h>, right ?

[krytarowski]

<strings.h> should be included unconditionally.

Sorry, I've overlooked that this is stringS.h. Including string.hunconditionally is fine. The stringS form maybe is needed for GNU, it's on BSDs too as it's a legacy header, but NetBSD both work and string.h is preferred.

lib/asan/tests/asan_mem_test.cc

248 ↗

(On Diff #187327)

|| defined(__NetBSD__)

[courbet]

lib/sanitizer_common/sanitizer_common_interceptors.inc

874 ↗

(On Diff #187325)

Looking at COMMON_INTERCEPTOR_ENTER implementation for, e.g. tsan:

#define SCOPED_TSAN_INTERCEPTOR(func, ...) \
    SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
    if (REAL(func) == 0) { \
      Printf("FATAL: ThreadSanitizer: failed to intercept %s\n", #func); \
      Die(); \
    } \
    if (thr->in_rtl > 1) \
      return REAL(func)(__VA_ARGS__); 


#define COMMON_INTERCEPTOR_ENTER(ctx, func, ...)      \
  SCOPED_TSAN_INTERCEPTOR(func, __VA_ARGS__);         \
  TsanInterceptorContext _ctx = {thr, caller_pc, pc}; \
  ctx = (void *)&_ctx;                                \
  (void) ctx;

Given that the implementation calls REAL(func), it feels preferable to call the real bcmp instead of real memcmp, even though calling memcmp does work. But again, I'm not familiar with how you guys typically approach this, so you tell me :)

[courbet]

• guard with NetBSD

[MaskRay]

lib/asan/tests/asan_mem_test.cc

15 ↗

(On Diff #187328)

Maybe.. You can also enable it for OpenBSD string.h and FreeBSD strings.h

[krytarowski]

lib/asan/tests/asan_mem_test.cc

248 ↗

(On Diff #187327)

This syntax is not valid.

#if defined(_GNU_SOURCE) || defined(__NetBSD__)

[courbet]

• ifdef A || defined(B) -> if defined(A) || defined(B)

[courbet]

• Add FreeBSD and OpenBSD.

[jyknight]

lib/asan/tests/asan_mem_test.cc

15 ↗

(On Diff #187328)

Both openbsd and netbsd appear to expose it from <strings.h> (plural) as the primary location too.

On both systems, <string.h> (singular) does a conditional #include of <strings.h>, so this probably works too, but seems better to consolidate around strings.h.

[joerg]

Actually, since it used to be part of XSI, I would include it unconditionally and only conditionalize it if a system starts to actually drop it.

[krytarowski]

lib/asan/tests/asan_mem_test.cc

15 ↗

(On Diff #187328)

I would include <string.h> unconditionally and only <strings.h> there where needed.

#include <string.h>
#if defined (_GNU_SOURCE)
#include <strings.h>
#endif

FreeBSD doesn't need <strings.h> either.

test/sanitizer_common/TestCases/Posix/weak_hook_test.cc

11 ↗

(On Diff #187348)

No need for __FreeBSD__.

[courbet]

lib/asan/tests/asan_mem_test.cc

15 ↗

(On Diff #187328)

I would include <string.h> unconditionally and only <strings.h> there where needed.

Right, and that's consistent with weak_hook_test.

On both systems, <string.h> (singular) does a conditional #include of <strings.h>, so this probably works too, but seems better to consolidate around strings.h.

The freebsd man page has:

A bcmp() function first appeared in 4.2BSD.  Its prototype	existed	previ-
ously in <string.h> before	it was moved to	<strings.h> for	IEEE Std
1003.1-2001 (``POSIX.1'') compliance.

In any case, on all systems strings.h seems to be doing the right thing to accommodate this so I kept strings.h only for _GNU_SOURCE for simplicity.

[courbet]

• Address review comments.

[vitalybuka]

lib/asan/tests/asan_mem_test.cc

212 ↗

(On Diff #187533)

Maybe https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#how-to-write-value-parameterized-tests instead of function?

lib/sanitizer_common/sanitizer_common_interceptors.inc

869 ↗

(On Diff #187533)

usually we don't have empty line here

874 ↗

(On Diff #187325)

Sometimes we do macro
But I'd prefer inline function, e.g.

int memcpy_interceptor_impl(ctx, a1, a2, size, real_fn_ptr) {
....
}

lib/sanitizer_common/sanitizer_platform_interceptors.h

145 ↗

(On Diff #187533)

#define SANITIZER_INTERCEPT_BCMP (SANITIZER_INTERCEPT_MEMCMP && ( \
    (SI_POSIX && _GNU_SOURCE) || \
    SI_NETBSD || SI_OPENBSD || SI_FREEBSD))

also please reformat it with "git clang-format -f --style=file --extensions=inc,cc,h,c,cpp HEAD^"

[courbet]

• Address review comments
• clang-format

[courbet]

Thanks

lib/asan/tests/asan_mem_test.cc

212 ↗

(On Diff #187533)

The template is more consistent with the two other tests above. If we want to be even more consistent I can define functors:

class MemCmpWrapper {
 public:
  static int transfer(const void *l, const void *r, size_t size) {
    return Ident(memcmp)(l, r, size);
  }
};

but that seems overkill to me.

lib/sanitizer_common/sanitizer_common_interceptors.inc

874 ↗

(On Diff #187325)

I'm not sure I understand your suggestion correctly, but if you're suggesting we refactor using a function, then the issue is that REAL(x) only works with x a macro parameter, not if x is a function pointer:

# define PTR_TO_REAL(x) real_##x
...
# define REAL(x) __interception::PTR_TO_REAL(x)

Refactoring using a macro would be possible, but it would look quite ugly.

[vitalybuka]

lib/sanitizer_common/sanitizer_common_interceptors.inc

874 ↗

(On Diff #187325)

Looks like you can easily extract into a function code before REAL call

[courbet]

• Refactor common code for memcmp and bcmp.

[courbet]

lib/sanitizer_common/sanitizer_common_interceptors.inc

874 ↗

(On Diff #187325)

Right, done.
Note that this has the side-effect of adding one more function in the call stack (see test/asan/TestCases/memcmp_test.cc)

[vitalybuka]

LGTM

[courbet]

Thanks for the review.

compiler-rt/trunk/lib/asan/tests/asan_mem_test.cc

 //===-- asan_mem_test.cc --------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of AddressSanitizer, an address sanity checker.
 //
 //===----------------------------------------------------------------------===//
+#include <string.h>
 #include "asan_test_utils.h"
+#if defined(_GNU_SOURCE)
+#include <strings.h>  // for bcmp
+#endif
 #include <vector>
 
 template<typename T>
 void MemSetOOBTestTemplate(size_t length) {
   if (length == 0) return;
   size_t size = Ident(sizeof(T) * length);
   T *array = Ident((T*)malloc(size));
   int element = Ident(42);
public:
   }
 };
 
 TEST(AddressSanitizer, MemMoveOOBTest) {
   MemTransferOOBTestTemplate<char, MemMoveWrapper>(100);
   MemTransferOOBTestTemplate<int, MemMoveWrapper>(1024);
 }
 
-
-TEST(AddressSanitizer, MemCmpOOBTest) {
+template <int (*cmpfn)(const void *, const void *, size_t)>
+void CmpOOBTestCommon() {
   size_t size = Ident(100);
   char *s1 = MallocAndMemsetString(size);
   char *s2 = MallocAndMemsetString(size);
-  // Normal memcmp calls.
-  Ident(memcmp(s1, s2, size));
-  Ident(memcmp(s1 + size - 1, s2 + size - 1, 1));
-  Ident(memcmp(s1 - 1, s2 - 1, 0));
+  // Normal cmpfn calls.
+  Ident(cmpfn(s1, s2, size));
+  Ident(cmpfn(s1 + size - 1, s2 + size - 1, 1));
+  Ident(cmpfn(s1 - 1, s2 - 1, 0));
   // One of arguments points to not allocated memory.
-  EXPECT_DEATH(Ident(memcmp)(s1 - 1, s2, 1), LeftOOBReadMessage(1));
-  EXPECT_DEATH(Ident(memcmp)(s1, s2 - 1, 1), LeftOOBReadMessage(1));
-  EXPECT_DEATH(Ident(memcmp)(s1 + size, s2, 1), RightOOBReadMessage(0));
-  EXPECT_DEATH(Ident(memcmp)(s1, s2 + size, 1), RightOOBReadMessage(0));
+  EXPECT_DEATH(Ident(cmpfn)(s1 - 1, s2, 1), LeftOOBReadMessage(1));
+  EXPECT_DEATH(Ident(cmpfn)(s1, s2 - 1, 1), LeftOOBReadMessage(1));
+  EXPECT_DEATH(Ident(cmpfn)(s1 + size, s2, 1), RightOOBReadMessage(0));
+  EXPECT_DEATH(Ident(cmpfn)(s1, s2 + size, 1), RightOOBReadMessage(0));
   // Hit unallocated memory and die.
-  EXPECT_DEATH(Ident(memcmp)(s1 + 1, s2 + 1, size), RightOOBReadMessage(0));
-  EXPECT_DEATH(Ident(memcmp)(s1 + size - 1, s2, 2), RightOOBReadMessage(0));
+  EXPECT_DEATH(Ident(cmpfn)(s1 + 1, s2 + 1, size), RightOOBReadMessage(0));
+  EXPECT_DEATH(Ident(cmpfn)(s1 + size - 1, s2, 2), RightOOBReadMessage(0));
   // Zero bytes are not terminators and don't prevent from OOB.
   s1[size - 1] = '\0';
   s2[size - 1] = '\0';
-  EXPECT_DEATH(Ident(memcmp)(s1, s2, size + 1), RightOOBReadMessage(0));
+  EXPECT_DEATH(Ident(cmpfn)(s1, s2, size + 1), RightOOBReadMessage(0));
 
   // Even if the buffers differ in the first byte, we still assume that
-  // memcmp may access the whole buffer and thus reporting the overflow here:
+  // cmpfn may access the whole buffer and thus reporting the overflow here:
   s1[0] = 1;
   s2[0] = 123;
-  EXPECT_DEATH(Ident(memcmp)(s1, s2, size + 1), RightOOBReadMessage(0));
+  EXPECT_DEATH(Ident(cmpfn)(s1, s2, size + 1), RightOOBReadMessage(0));
 
   free(s1);
   free(s2);
 }
 
+TEST(AddressSanitizer, MemCmpOOBTest) { CmpOOBTestCommon<memcmp>(); }
 
-
+TEST(AddressSanitizer, BCmpOOBTest) {
+#if defined(_GNU_SOURCE) || defined(__NetBSD__) || defined(__FreeBSD__) || \
+    defined(__OpenBSD__)
+  CmpOOBTestCommon<bcmp>();
+#endif
+}

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

do {                                               \
     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)
 
-INTERCEPTOR(int, memcmp, const void *a1, const void *a2, uptr size) {
-  if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
-    return internal_memcmp(a1, a2, size);
-  void *ctx;
-  COMMON_INTERCEPTOR_ENTER(ctx, memcmp, a1, a2, size);
+// 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) {
     if (common_flags()->strict_memcmp) {
       // Check the entire regions even if the first bytes of the buffers are
       // different.
       COMMON_INTERCEPTOR_READ_RANGE(ctx, a1, size);
       COMMON_INTERCEPTOR_READ_RANGE(ctx, a2, size);
       // Fallthrough to REAL(memcmp) below.
     } else {
if (common_flags()->strict_memcmp) {
       COMMON_INTERCEPTOR_READ_RANGE(ctx, s1, Min(i + 1, size));
       COMMON_INTERCEPTOR_READ_RANGE(ctx, s2, Min(i + 1, size));
       int r = CharCmpX(c1, c2);
       CALL_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_memcmp, GET_CALLER_PC(),
                                  a1, a2, size, r);
       return r;
     }
   }
-  int result = REAL(memcmp(a1, a2, size));
+  int result = real_fn(a1, a2, size);
   CALL_WEAK_INTERCEPTOR_HOOK(__sanitizer_weak_hook_memcmp, GET_CALLER_PC(), a1,
                              a2, size, result);
   return result;
 }
 
+INTERCEPTOR(int, memcmp, const void *a1, const void *a2, uptr size) {
+  if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
+    return internal_memcmp(a1, a2, size);
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, memcmp, a1, a2, size);
+  return MemcmpInterceptorCommon(ctx, REAL(memcmp), a1, a2, size);
+}
+
 #define INIT_MEMCMP COMMON_INTERCEPT_FUNCTION(memcmp)
 #else
 #define INIT_MEMCMP
 #endif
 
+#if SANITIZER_INTERCEPT_BCMP
+INTERCEPTOR(int, bcmp, const void *a1, const void *a2, uptr size) {
+  if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
+    return internal_memcmp(a1, a2, size);
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, bcmp, a1, a2, size);
+  return MemcmpInterceptorCommon(ctx, REAL(bcmp), a1, a2, size);
+}
+
+#define INIT_BCMP COMMON_INTERCEPT_FUNCTION(bcmp)
+#else
+#define INIT_BCMP
+#endif
+
 #if SANITIZER_INTERCEPT_MEMCHR
 INTERCEPTOR(void*, memchr, const void *s, int c, SIZE_T n) {
   if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
     return internal_memchr(s, c, n);
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, memchr, s, c, n);
 #if SANITIZER_WINDOWS
   void *res;
static void InitializeCommonInterceptors() {
   INIT_STRPBRK;
   INIT_STRXFRM;
   INIT___STRXFRM_L;
   INIT_MEMSET;
   INIT_MEMMOVE;
   INIT_MEMCPY;
   INIT_MEMCHR;
   INIT_MEMCMP;
+  INIT_BCMP;
   INIT_MEMRCHR;
   INIT_MEMMEM;
   INIT_READ;
   INIT_FREAD;
   INIT_PREAD;
   INIT_PREAD64;
   INIT_READV;
   INIT_PREADV;

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

 #define SANITIZER_INTERCEPT_STRSPN SI_NOT_FUCHSIA
 #define SANITIZER_INTERCEPT_STRPBRK SI_NOT_FUCHSIA
 #define SANITIZER_INTERCEPT_TEXTDOMAIN SI_LINUX_NOT_ANDROID || SI_SOLARIS
 #define SANITIZER_INTERCEPT_STRCASECMP SI_POSIX
 #define SANITIZER_INTERCEPT_MEMSET 1
 #define SANITIZER_INTERCEPT_MEMMOVE 1
 #define SANITIZER_INTERCEPT_MEMCPY 1
 #define SANITIZER_INTERCEPT_MEMCMP SI_NOT_FUCHSIA
+#define SANITIZER_INTERCEPT_BCMP \
+  SANITIZER_INTERCEPT_MEMCMP &&  \
+      ((SI_POSIX && _GNU_SOURCE) || SI_NETBSD || SI_OPENBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_STRNDUP SI_POSIX
 #define SANITIZER_INTERCEPT___STRNDUP SI_LINUX_NOT_FREEBSD
 #if defined(__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__) && \
     __ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 1070
 # define SI_MAC_DEPLOYMENT_BELOW_10_7 1
 #else
 # define SI_MAC_DEPLOYMENT_BELOW_10_7 0
 #endif

compiler-rt/trunk/test/asan/TestCases/Posix/bcmp_test.cc

+// RUN: %clangxx_asan -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O1 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O2 %s -o %t && not %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_asan -O3 %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+// REQUIRES: compiler-rt-optimized
+
+#include <string.h>
+int main(int argc, char **argv) {
+  char a1[] = {static_cast<char>(argc), 2, 3, 4};
+  char a2[] = {1, static_cast<char>(2 * argc), 3, 4};
+  int res = bcmp(a1, a2, 4 + argc); // BOOM
+  // CHECK: AddressSanitizer: stack-buffer-overflow
+  // CHECK: {{#1.*bcmp}}
+  // CHECK: {{#2.*main}}
+  return res;
+}

compiler-rt/trunk/test/asan/TestCases/memcmp_test.cc

 // RUN: %clangxx_asan -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
 // RUN: %clangxx_asan -O1 %s -o %t && not %run %t 2>&1 | FileCheck %s
 // RUN: %clangxx_asan -O2 %s -o %t && not %run %t 2>&1 | FileCheck %s
 // RUN: %clangxx_asan -O3 %s -o %t && not %run %t 2>&1 | FileCheck %s
 
 // REQUIRES: compiler-rt-optimized
 
 #include <string.h>
 int main(int argc, char **argv) {
   char a1[] = {static_cast<char>(argc), 2, 3, 4};
   char a2[] = {1, static_cast<char>(2*argc), 3, 4};
   int res = memcmp(a1, a2, 4 + argc);  // BOOM
   // CHECK: AddressSanitizer: stack-buffer-overflow
-  // CHECK: {{#0.*memcmp}}
-  // CHECK: {{#1.*main}}
+  // CHECK: {{#1.*memcmp}}
+  // CHECK: {{#2.*main}}
   return res;
 }

compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/weak_hook_test.cc

 // Test the weak hooks.
 // RUN: %clangxx %s -o %t
 // RUN: %run %t
 
 // Hooks are not implemented for lsan.
 // XFAIL: lsan
 // XFAIL: ubsan
 
-#include <string.h>
 #include <assert.h>
+#include <string.h>
+#if defined(_GNU_SOURCE)
+#include <strings.h> // for bcmp
+#endif
 
 bool seen_memcmp, seen_strncmp, seen_strncasecmp, seen_strcmp, seen_strcasecmp,
     seen_strstr, seen_strcasestr, seen_memmem;
 
 extern "C" {
 void __sanitizer_weak_hook_memcmp(void *called_pc, const void *s1,
                                   const void *s2, size_t n, int result) {
   seen_memcmp = true;

 static volatile void *ptr_sink;
 
 int main() {
   assert(sizeof(s2) < sizeof(s1));
 
   int_sink = memcmp(s1, s2, sizeof(s2));
   assert(seen_memcmp);
 
+#if defined(_GNU_SOURCE) || defined(__NetBSD__) || defined(__FreeBSD__) || \
+    defined(__OpenBSD__)
+  seen_memcmp = false;
+  int_sink = bcmp(s1, s2, sizeof(s2));
+  assert(seen_memcmp);
+#endif
+
   int_sink = strncmp(s1, s2, sizeof(s2));
   assert(seen_strncmp);
 
   int_sink = strncasecmp(s1, s2, sizeof(s2));
   assert(seen_strncasecmp);
 
   int_sink = strcmp(s1, s2);
   assert(seen_strcmp);