[compiler-rt] r353351 - [sanitizer] Don't unpoison buffer in getpw/getgr functions
via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 7 03:37:30 PST 2019
Hi Vitaly,
The test you added in this commit seems to be hitting a MemorySanitizer issue on our internal build bot:
Script:
--
: 'RUN: at line 1'; /home/siadmin/jenkins/w/opensource/opensource_build/build/./bin/clang --driver-mode=g++ -gline-tables-only -fsanitize=memory -m64 -ldl /home/siadmin/jenkins/w/opensource/opensource_build/llvm/projects/compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc -o /home/siadmin/jenkins/w/opensource/opensource_build/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Posix/Output/getpw_getgr.cc.tmp && /home/siadmin/jenkins/w/opensource/opensource_build/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Posix/Output/getpw_getgr.cc.tmp
--
Exit Code: 77
Command Output (stderr):
--
Uninitialized bytes in __interceptor_getgrnam at offset 4 inside [0x7020000002f8, 5)
==183670==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x4965e5 in void test<group, group* (*)(char const*), char const*>(group* (*)(char const*), char const*) /home/siadmin/jenkins/w/opensource/opensource_build/llvm/projects/compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc:55:15
#1 0x495bfb in main /home/siadmin/jenkins/w/opensource/opensource_build/llvm/projects/compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc:73:3
#2 0x7fd3beef5f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
#3 0x41bc93 in _start (/home/siadmin/jenkins/w/opensource/opensource_build/build/projects/compiler-rt/test/sanitizer_common/msan-x86_64-Linux/Posix/Output/getpw_getgr.cc.tmp+0x41bc93)
SUMMARY: MemorySanitizer: use-of-uninitialized-value /home/siadmin/jenkins/w/opensource/opensource_build/llvm/projects/compiler-rt/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc:55:15 in void test<group, group* (*)(char const*), char const*>(group* (*)(char const*), char const*)
Exiting
--
I suspect this is a bug with the older version of libc (Ubuntu EGLIBC 2.19-0ubuntu6.5) that is installed on our build bot since compiling your test with the commit immediately preceding your commit produces the same error, while compiling your change on a machine with a newer libc version (Ubuntu GLIBC 2.27-3ubuntu1) works just fine. Looking through the existing tests, it appears none of them called getgrnam() previously, so it likely was never encountered.
I did a little bit of investigation, and it seems that the libc implementation on our build machine is doing something with the name argument that is hitting the memory sanitizer error. To test this, I modified line 1889 in the getgrnam interceptor definition to use a string constant with the same value the test passes in, instead of a pointer to a const char * and that seems to prevent the memory sanitizer warning.
Unmodified source:
__sanitizer_group *res = REAL(getgrnam)(name);
Modified source:
__sanitizer_group *res = REAL(getgrnam)("root");
Would it be possible to restructure the test by any chance in order to avoid this problem on our build bot?
Douglas Yung
-----Original Message-----
From: llvm-commits <llvm-commits-bounces at lists.llvm.org> On Behalf Of Vitaly Buka via llvm-commits
Sent: Wednesday, February 6, 2019 16:08
To: llvm-commits at lists.llvm.org
Subject: [compiler-rt] r353351 - [sanitizer] Don't unpoison buffer in getpw/getgr functions
Author: vitalybuka
Date: Wed Feb 6 16:08:14 2019
New Revision: 353351
URL: http://llvm.org/viewvc/llvm-project?rev=353351&view=rev
Log:
[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
Added:
compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc
Modified:
compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc
Modified: compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc?rev=353351&r1=353350&r2=353351&view=diff
==============================================================================
--- compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc (original)
+++ compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors
+++ .inc Wed Feb 6 16:08:14 2019
@@ -1822,27 +1822,27 @@ UNUSED static void unpoison_passwd(void
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 *
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
// 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, __
// 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
// 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, __
// 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
// 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
// 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,
// 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,
// 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;
}
Modified: compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc?rev=353351&r1=353350&r2=353351&view=diff
==============================================================================
--- compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc (original)
+++ compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc Wed Feb 6
+++ 16:08:14 2019
@@ -2203,19 +2203,6 @@ static void HandleRecvmsg(ThreadState *t #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
Added: compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc
URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc?rev=353351&view=auto
==============================================================================
--- compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/getpw_getgr.cc (added)
+++ compiler-rt/trunk/test/sanitizer_common/TestCases/Posix/getpw_getgr.
+++ cc Wed Feb 6 16:08:14 2019
@@ -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;
+}
_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list