[sanitizer] Don't unpoison buffer in getpw/getgr functions

Summary:
Buffer should be referenced by results so used parts will be unpoisoned with unpoison_group and unpoison_passwd.

This fixes TSAN performance issue made us to disable this interceptors.

Reviewers: eugenis, dvyukov

Subscribers: srhines, kubamracek, krytarowski, #sanitizers

Tags: #sanitizers

Differential Revision: https://reviews.llvm.org/D57731

llvm-svn: 353351

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

@@ -1822,27 +1822,27 @@ UNUSED static void unpoison_passwd(void *ctx, __sanitizer_passwd *pwd) {
   if (pwd) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd, sizeof(*pwd));
     if (pwd->pw_name)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(pwd->pw_name,
-                                          REAL(strlen)(pwd->pw_name) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd->pw_name,
+                                     REAL(strlen)(pwd->pw_name) + 1);
     if (pwd->pw_passwd)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(pwd->pw_passwd,
-                                          REAL(strlen)(pwd->pw_passwd) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd->pw_passwd,
+                                     REAL(strlen)(pwd->pw_passwd) + 1);
 #if !SANITIZER_ANDROID
     if (pwd->pw_gecos)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(pwd->pw_gecos,
-                                          REAL(strlen)(pwd->pw_gecos) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd->pw_gecos,
+                                     REAL(strlen)(pwd->pw_gecos) + 1);
 #endif
-#if SANITIZER_MAC
+#if SANITIZER_MAC || SANITIZER_FREEBSD || SANITIZER_NETBSD || SANITIZER_OPENBSD
     if (pwd->pw_class)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(pwd->pw_class,
-                                          REAL(strlen)(pwd->pw_class) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd->pw_class,
+                                     REAL(strlen)(pwd->pw_class) + 1);
 #endif
     if (pwd->pw_dir)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(pwd->pw_dir,
-                                          REAL(strlen)(pwd->pw_dir) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd->pw_dir,
+                                     REAL(strlen)(pwd->pw_dir) + 1);
     if (pwd->pw_shell)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(pwd->pw_shell,
-                                          REAL(strlen)(pwd->pw_shell) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwd->pw_shell,
+                                     REAL(strlen)(pwd->pw_shell) + 1);
   }
 }
 
@@ -1850,17 +1850,17 @@ UNUSED static void unpoison_group(void *ctx, __sanitizer_group *grp) {
   if (grp) {
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, grp, sizeof(*grp));
     if (grp->gr_name)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(grp->gr_name,
-                                          REAL(strlen)(grp->gr_name) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, grp->gr_name,
+                                     REAL(strlen)(grp->gr_name) + 1);
     if (grp->gr_passwd)
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(grp->gr_passwd,
-                                          REAL(strlen)(grp->gr_passwd) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, grp->gr_passwd,
+                                     REAL(strlen)(grp->gr_passwd) + 1);
     char **p = grp->gr_mem;
     for (; *p; ++p) {
-      COMMON_INTERCEPTOR_INITIALIZE_RANGE(*p, REAL(strlen)(*p) + 1);
+      COMMON_INTERCEPTOR_WRITE_RANGE(ctx, *p, REAL(strlen)(*p) + 1);
     }
-    COMMON_INTERCEPTOR_INITIALIZE_RANGE(grp->gr_mem,
-                                        (p - grp->gr_mem + 1) * sizeof(*p));
+    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, grp->gr_mem,
+                                   (p - grp->gr_mem + 1) * sizeof(*p));
   }
 }
 #endif  // SANITIZER_POSIX
@@ -1916,10 +1916,8 @@ INTERCEPTOR(int, getpwnam_r, const char *name, __sanitizer_passwd *pwd,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(getpwnam_r)(name, pwd, buf, buflen, result);
-  if (!res) {
-    if (result && *result) unpoison_passwd(ctx, *result);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && result && *result)
+    unpoison_passwd(ctx, *result);
   if (result) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
   return res;
 }
@@ -1931,10 +1929,8 @@ INTERCEPTOR(int, getpwuid_r, u32 uid, __sanitizer_passwd *pwd, char *buf,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(getpwuid_r)(uid, pwd, buf, buflen, result);
-  if (!res) {
-    if (result && *result) unpoison_passwd(ctx, *result);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && result && *result)
+    unpoison_passwd(ctx, *result);
   if (result) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
   return res;
 }
@@ -1947,10 +1943,8 @@ INTERCEPTOR(int, getgrnam_r, const char *name, __sanitizer_group *grp,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(getgrnam_r)(name, grp, buf, buflen, result);
-  if (!res) {
-    if (result && *result) unpoison_group(ctx, *result);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && result && *result)
+    unpoison_group(ctx, *result);
   if (result) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
   return res;
 }
@@ -1962,10 +1956,8 @@ INTERCEPTOR(int, getgrgid_r, u32 gid, __sanitizer_group *grp, char *buf,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(getgrgid_r)(gid, grp, buf, buflen, result);
-  if (!res) {
-    if (result && *result) unpoison_group(ctx, *result);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && result && *result)
+    unpoison_group(ctx, *result);
   if (result) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, result, sizeof(*result));
   return res;
 }
@@ -2031,10 +2023,8 @@ INTERCEPTOR(int, getpwent_r, __sanitizer_passwd *pwbuf, char *buf,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(getpwent_r)(pwbuf, buf, buflen, pwbufp);
-  if (!res) {
-    if (pwbufp && *pwbufp) unpoison_passwd(ctx, *pwbufp);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && pwbufp && *pwbufp)
+    unpoison_passwd(ctx, *pwbufp);
   if (pwbufp) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwbufp, sizeof(*pwbufp));
   return res;
 }
@@ -2046,10 +2036,8 @@ INTERCEPTOR(int, getgrent_r, __sanitizer_group *pwbuf, char *buf, SIZE_T buflen,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(getgrent_r)(pwbuf, buf, buflen, pwbufp);
-  if (!res) {
-    if (pwbufp && *pwbufp) unpoison_group(ctx, *pwbufp);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && pwbufp && *pwbufp)
+    unpoison_group(ctx, *pwbufp);
   if (pwbufp) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwbufp, sizeof(*pwbufp));
   return res;
 }
@@ -2069,10 +2057,8 @@ INTERCEPTOR(int, fgetpwent_r, void *fp, __sanitizer_passwd *pwbuf, char *buf,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(fgetpwent_r)(fp, pwbuf, buf, buflen, pwbufp);
-  if (!res) {
-    if (pwbufp && *pwbufp) unpoison_passwd(ctx, *pwbufp);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && pwbufp && *pwbufp)
+    unpoison_passwd(ctx, *pwbufp);
   if (pwbufp) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwbufp, sizeof(*pwbufp));
   return res;
 }
@@ -2091,10 +2077,8 @@ INTERCEPTOR(int, fgetgrent_r, void *fp, __sanitizer_group *pwbuf, char *buf,
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   int res = REAL(fgetgrent_r)(fp, pwbuf, buf, buflen, pwbufp);
-  if (!res) {
-    if (pwbufp && *pwbufp) unpoison_group(ctx, *pwbufp);
-    COMMON_INTERCEPTOR_WRITE_RANGE(ctx, buf, buflen);
-  }
+  if (!res && pwbufp && *pwbufp)
+    unpoison_group(ctx, *pwbufp);
   if (pwbufp) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pwbufp, sizeof(*pwbufp));
   return res;
 }

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

@@ -2203,19 +2203,6 @@ static void HandleRecvmsg(ThreadState *thr, uptr pc,
 #include "sanitizer_common/sanitizer_platform_interceptors.h"
 // Causes interceptor recursion (getaddrinfo() and fopen())
 #undef SANITIZER_INTERCEPT_GETADDRINFO
-// There interceptors do not seem to be strictly necessary for tsan.
-// But we see cases where the interceptors consume 70% of execution time.
-// Memory blocks passed to fgetgrent_r are "written to" by tsan several times.
-// First, there is some recursion (getgrnam_r calls fgetgrent_r), and each
-// function "writes to" the buffer. Then, the same memory is "written to"
-// twice, first as buf and then as pwbufp (both of them refer to the same
-// addresses).
-#undef SANITIZER_INTERCEPT_GETPWENT
-#undef SANITIZER_INTERCEPT_GETPWENT_R
-#undef SANITIZER_INTERCEPT_FGETPWENT
-#undef SANITIZER_INTERCEPT_FGETPWENT_R
-#undef SANITIZER_INTERCEPT_GETPWNAM_AND_FRIENDS
-#undef SANITIZER_INTERCEPT_GETPWNAM_R_AND_FRIENDS
 // We define our own.
 #if SANITIZER_INTERCEPT_TLS_GET_ADDR
 #define NEED_TLS_GET_ADDR

compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc

@@ -0,0 +1,112 @@
+// RUN: %clangxx %s -o %t && %run %t
+// UNSUPPORTED: android, ios
+
+#include <assert.h>
+#include <grp.h>
+#include <memory>
+#include <pwd.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <string>
+
+std::string any_group;
+const int N = 123456;
+
+void Check(const char *str) {
+  assert(strlen(str) != N);
+}
+
+void Check(const passwd *result) {
+  Check(result->pw_name);
+  Check(result->pw_passwd);
+  assert(result->pw_uid != N);
+  assert(result->pw_gid != N);
+#if !defined(__ANDROID__)
+  Check(result->pw_gecos);
+#endif
+  Check(result->pw_dir);
+
+#if defined(__APPLE__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
+  assert(result->pw_change != N);
+  Check(result->pw_class);
+  assert(result->pw_expire != N);
+#endif
+
+#if defined(__FreeBSD__)
+  assert(result->pw_fields != N);
+#endif
+
+  // SunOS also has pw_age and pw_comment which are documented as unused.
+}
+
+void Check(const group *result) {
+  Check(result->gr_name);
+  Check(result->gr_passwd);
+  assert(result->gr_gid != N);
+  for (char **mem = result->gr_mem; *mem; ++mem)
+    Check(*mem);
+  if (any_group.empty())
+    any_group = result->gr_name;
+}
+
+template <class T, class Fn, class... Args>
+void test(Fn f, Args... args) {
+  T *result = f(args...);
+  Check(result);
+}
+
+template <class T, class Fn, class... Args>
+void test_r(Fn f, Args... args) {
+  T gr;
+  T *result;
+  char buff[10000];
+  assert(!f(args..., &gr, buff, sizeof(buff), &result));
+  Check(&gr);
+  Check(result);
+}
+
+int main(int argc, const char *argv[]) {
+  test<passwd>(&getpwuid, 0);
+  test<passwd>(&getpwnam, "root");
+  test<group>(&getgrgid, 0);
+  test<group>(&getgrnam, any_group.c_str());
+
+#if !defined(__ANDROID__)
+  setpwent();
+  test<passwd>(&getpwent);
+  setgrent();
+  test<group>(&getgrent);
+
+#if !defined(__APPLE__)
+  setpwent();
+  test_r<passwd>(&getpwent_r);
+  setgrent();
+  test_r<group>(&getgrent_r);
+#endif
+
+  test_r<passwd>(&getpwuid_r, 0);
+  test_r<passwd>(&getpwnam_r, "root");
+
+  test_r<group>(&getgrgid_r, 0);
+  test_r<group>(&getgrnam_r, any_group.c_str());
+
+#if defined(__linux__)
+  auto pwd_file = [] {
+    return std::unique_ptr<FILE, decltype(&fclose)>(fopen("/etc/passwd", "r"),
+                                                    &fclose);
+  };
+  auto gr_file = [] {
+    return std::unique_ptr<FILE, decltype(&fclose)>(fopen("/etc/group", "r"),
+                                                    &fclose);
+  };
+  test<passwd>(&fgetpwent, pwd_file().get());
+  test<group>(&fgetgrent, gr_file().get());
+  test_r<passwd>(&fgetpwent_r, pwd_file().get());
+  test_r<group>(&fgetgrent_r, gr_file().get());
+#endif
+
+#endif // __ANDROID__
+
+  return 0;
+}