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

[vitalybuka]

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.

[vitalybuka]

fixed not linux test

[vitalybuka]

tweaks

[vitalybuka]

Check more fields

[vitalybuka]

Fix pw_fields check

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

1830–1831

This might cause TSan problems if some of these fields point to a static string constant. Let's hope this is not the case.

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

3

why not android?

[vitalybuka]

Android #if defined

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

1830–1831

it returns non const pointers, so it would be bad.

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

3

I've added Android conditions, but I'd like to re-enable them in separate patch.

[eugenis]

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

#if SANITIZER_INTERCEPT_GETPWNAM_AND_FRIENDS || \
     SANITIZER_INTERCEPT_GETPWENT_R || \
     SANITIZER_INTERCEPT_GETPWNAM_R_AND_FRIENDS || \
     SANITIZER_INTERCEPT_FGETPWENT_R || \
     SANITIZER_INTERCEPT_FGETGRENT_R
 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,
     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,
 #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,
 #endif
 #if SANITIZER_MAC
     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,
 #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,
     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,
   }
 }
 
 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,
     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,
     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,
   }
 }
 #endif  // SANITIZER_INTERCEPT_GETPWNAM_AND_FRIENDS ||
         // SANITIZER_INTERCEPT_GETPWENT || SANITIZER_INTERCEPT_FGETPWENT ||
         // SANITIZER_INTERCEPT_GETPWENT_R ||
         // SANITIZER_INTERCEPT_GETPWNAM_R_AND_FRIENDS
 
 #if SANITIZER_INTERCEPT_GETPWNAM_AND_FRIENDS
INTERCEPTOR(int, getpwnam_r, const char *name, __sanitizer_passwd *pwd,
             char *buf, SIZE_T buflen, __sanitizer_passwd **result) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getpwnam_r, name, pwd, buf, buflen, result);
   COMMON_INTERCEPTOR_READ_RANGE(ctx, name, REAL(strlen)(name) + 1);
   // 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(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;
 }
 INTERCEPTOR(int, getpwuid_r, u32 uid, __sanitizer_passwd *pwd, char *buf,
             SIZE_T buflen, __sanitizer_passwd **result) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getpwuid_r, uid, pwd, buf, buflen, result);
   // 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(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;
 }
 INTERCEPTOR(int, getgrnam_r, const char *name, __sanitizer_group *grp,
             char *buf, SIZE_T buflen, __sanitizer_group **result) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getgrnam_r, name, grp, buf, buflen, result);
   COMMON_INTERCEPTOR_READ_RANGE(ctx, name, REAL(strlen)(name) + 1);
   // 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(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;
 }
 INTERCEPTOR(int, getgrgid_r, u32 gid, __sanitizer_group *grp, char *buf,
             SIZE_T buflen, __sanitizer_group **result) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getgrgid_r, gid, grp, buf, buflen, result);
   // 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(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;
 }
 #define INIT_GETPWNAM_R_AND_FRIENDS      \
   COMMON_INTERCEPT_FUNCTION(getpwnam_r); \
   COMMON_INTERCEPT_FUNCTION(getpwuid_r); \
   COMMON_INTERCEPT_FUNCTION(getgrnam_r); \
   COMMON_INTERCEPT_FUNCTION(getgrgid_r);

 INTERCEPTOR(int, getpwent_r, __sanitizer_passwd *pwbuf, char *buf,
             SIZE_T buflen, __sanitizer_passwd **pwbufp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getpwent_r, pwbuf, buf, buflen, pwbufp);
   // 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(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;
 }
 INTERCEPTOR(int, getgrent_r, __sanitizer_group *pwbuf, char *buf, SIZE_T buflen,
             __sanitizer_group **pwbufp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getgrent_r, pwbuf, buf, buflen, pwbufp);
   // 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(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;
 }
 #define INIT_GETPWENT_R                   \
   COMMON_INTERCEPT_FUNCTION(getpwent_r);  \
   COMMON_INTERCEPT_FUNCTION(getgrent_r);
 #else
 #define INIT_GETPWENT_R
 #endif
 
 #if SANITIZER_INTERCEPT_FGETPWENT_R
 INTERCEPTOR(int, fgetpwent_r, void *fp, __sanitizer_passwd *pwbuf, char *buf,
             SIZE_T buflen, __sanitizer_passwd **pwbufp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, fgetpwent_r, fp, pwbuf, buf, buflen, pwbufp);
   // 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(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;
 }
 #define INIT_FGETPWENT_R                  \
   COMMON_INTERCEPT_FUNCTION(fgetpwent_r);
 #else
 #define INIT_FGETPWENT_R
 #endif
 
 #if SANITIZER_INTERCEPT_FGETGRENT_R
 INTERCEPTOR(int, fgetgrent_r, void *fp, __sanitizer_group *pwbuf, char *buf,
             SIZE_T buflen, __sanitizer_group **pwbufp) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, fgetgrent_r, fp, pwbuf, buf, buflen, pwbufp);
   // 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(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;
 }
 #define INIT_FGETGRENT_R                  \
   COMMON_INTERCEPT_FUNCTION(fgetgrent_r);
 #else
 #define INIT_FGETGRENT_R
 #endif

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

static void HandleRecvmsg(ThreadState *thr, uptr pc,
   for (int i = 0; i < cnt; i++)
     FdEventCreate(thr, pc, fds[i]);
 }
 #endif
 
 #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
 #endif
 #undef SANITIZER_INTERCEPT_TLS_GET_ADDR
 
 #define COMMON_INTERCEPT_FUNCTION(name) INTERCEPT_FUNCTION(name)
 #define COMMON_INTERCEPT_FUNCTION_VER(name, ver)                          \

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

+// 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);
+  Check(result->pw_dir);
+  assert(result->pw_uid != N);
+  assert(result->pw_gid != N);
+  Check(result->pw_gecos);
+#if defined(__APPLE__)
+  Check(result->pw_class);
+#endif
+}
+
+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[]) {
+  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<passwd>(&getpwnam, "root");
+  test_r<passwd>(&getpwnam_r, "root");
+  test<passwd>(&getpwuid, 0);
+  test_r<passwd>(&getpwuid_r, 0);
+
+  test<group>(&getgrgid, 0);
+  test_r<group>(&getgrgid_r, 0);
+  test<group>(&getgrnam, any_group.c_str());
+  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
+
+  return 0;
+}