sanitizers: Disable crypt, crypt_r interceptors for glibc

[fweimer]

Summary

On GNU/Linux (glibc), the crypt and crypt_r functions are not part of the main shared object (libc.so.6), but libcrypt (with multiple possible sonames). The sanitizer libraries do not depend on libcrypt, so it can happen that during sanitizer library initialization, no real implementation will be found because the crypt, crypt_r functions are not present in the process image (yet). If its interceptors are called nevertheless, this results in a call through a null pointer when the sanitizer library attempts to forward the call to the real implementation.

Many distributions have already switched to libxcrypt, a library that is separate from glibc and that can be build with sanitizers directly (avoiding the need for interceptors). This patch disables building the interceptor for glibc targets.

I could not get the test harness to skip the crypt_r test for glibc, so I'm removing it. It is Linux-specific anyway, Android was already excluded, so it has limited scope anyway.

Related: https://github.com/google/sanitizers/issues/1365
Related: https://bugzilla.redhat.com/show_bug.cgi?id=2169432

[vitalybuka]

Looks like it was added for glibc Linux in D68431 for fix a problem.

[fweimer]

In D144073#4153281, @vitalybuka wrote:

Looks like it was added for glibc Linux in D68431 for fix a problem.

I think it was just done to increase coverage? These days, it's possible to build libxcrypt with sanitizers enabled instead, so there is a clear alternative.

Anyway, given the bugs introduced by the current interceptors, could we please move this forward? Thanks.

[MaskRay]

I have tried to search for some crypt/crypt_r use cases but cannot find significant users. I think removing the interceptor will very unlikely cause issues for users with old glibc.

https://www.openwall.com/lists/musl/2019/11/08/10 musl still provides crypt/crypt_r. If it moves away, we can possibly remove the interceptor implementation.

[thesamesam]

Given glibc plans to remove its crypt() anyway, this seems fine.

@vitalybuka any objections?

[vitalybuka]

I don't have objection in general. It's not clear what was broken when original patch was landed.

However if we keep the code, we should keep the test.

I propose to revert D68431.

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_PCLOSE SI_POSIX
 #define SANITIZER_INTERCEPT_FUNOPEN (SI_NETBSD || SI_FREEBSD)
 #define SANITIZER_INTERCEPT_FUNOPEN2 SI_NETBSD
 #define SANITIZER_INTERCEPT_GETFSENT (SI_FREEBSD || SI_NETBSD || SI_MAC)
 #define SANITIZER_INTERCEPT_ARC4RANDOM (SI_FREEBSD || SI_NETBSD || SI_MAC)
 #define SANITIZER_INTERCEPT_FDEVNAME SI_FREEBSD
 #define SANITIZER_INTERCEPT_GETUSERSHELL (SI_POSIX && !SI_ANDROID)
 #define SANITIZER_INTERCEPT_SL_INIT (SI_FREEBSD || SI_NETBSD)
-#define SANITIZER_INTERCEPT_CRYPT (SI_POSIX && !SI_ANDROID)
-#define SANITIZER_INTERCEPT_CRYPT_R (SI_LINUX && !SI_ANDROID)
+#define SANITIZER_INTERCEPT_CRYPT (SI_POSIX && !SI_ANDROID && !SI_GLIBC)
+#define SANITIZER_INTERCEPT_CRYPT_R (SI_LINUX && !SI_ANDROID && !SI_GLIBC)
 
 #define SANITIZER_INTERCEPT_GETRANDOM \
   ((SI_LINUX && __GLIBC_PREREQ(2, 25)) || SI_FREEBSD)
 #define SANITIZER_INTERCEPT___CXA_ATEXIT SI_NETBSD
 #define SANITIZER_INTERCEPT_ATEXIT SI_NETBSD
 #define SANITIZER_INTERCEPT_PTHREAD_ATFORK SI_NETBSD
 #define SANITIZER_INTERCEPT_GETENTROPY SI_FREEBSD
 #define SANITIZER_INTERCEPT_QSORT \

compiler-rt/test/sanitizer_common/TestCases/Linux/crypt_r.cpp

-// RUN: %clangxx -O0 -g %s -lcrypt -o %t && %run %t
-
-// crypt.h is missing from Android.
-// UNSUPPORTED: android
-
-#include <assert.h>
-#include <unistd.h>
-#include <cstring>
-#include <crypt.h>
-
-int main(int argc, char **argv) {
-  {
-    crypt_data cd;
-    cd.initialized = 0;
-    char *p = crypt_r("abcdef", "xz", &cd);
-    volatile size_t z = strlen(p);
-  }
-  {
-    crypt_data cd;
-    cd.initialized = 0;
-    char *p = crypt_r("abcdef", "$1$", &cd);
-    volatile size_t z = strlen(p);
-  }
-  {
-    crypt_data cd;
-    cd.initialized = 0;
-    char *p = crypt_r("abcdef", "$5$", &cd);
-    volatile size_t z = strlen(p);
-  }
-  {
-    crypt_data cd;
-    cd.initialized = 0;
-    char *p = crypt_r("abcdef", "$6$", &cd);
-    volatile size_t z = strlen(p);
-  }
-}