This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH BZ#20422] Do not allow asan/msan/tsan and fortify at the same time.


Hi!

When fortify is used with MSan it will cause MSan false positives.

#include <stdio.h>
#include <string.h>
int main()
{
        char text[100];
        sprintf(text, "hello");
        printf("%lu\n", strlen(text));
}

% clang test.c -fsanitize=memory   -O3 && ./a.out
5
% clang test.c -fsanitize=memory -D_FORTIFY_SOURCE=2  -O3 && ./a.out
Uninitialized bytes in __interceptor_strlen at offset 0 inside [0x7ffe259e4d20, 6)
==26297==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x4869cc in main

With ASan, this will not cause false positives, but may case false negatives or just confuse people with "wrong" reports when fortify catches the error.

Although fortify is good thing as it (and it's enabled by default on some major distros e.g. Ubuntu and Gentoo), people still complain about {A, M}San vs fortify interaction, see e.g. https://github.com/google/sanitizers/issues/689. One possible solution would be to extend {A, M}San to support foo_chk() functions, but this would increase the complexity of sanitizer tools with quite small benefit. Another choice would be to warn users when they compile their code with {A, M, T}San and fortify enabled.

This patch implements the second approach. The simplest way to warn is to modify the Glibc headers to check if fortify and one of the sanitizers is enabled. Does this look reasonable?

I've tried to add a testcase for new warning into Glibc testsuite, but failed to see how exactly I can do it. Does Glibc have some framework for compilation tests? Could someone help me with this issue? For now, I've tested this patch locally with GCC 4.8, fresh GCC and fresh Clang on my Ubuntu box:

gcc test.c -fsanitize=address -D_FORTIFY_SOURCE=2 -O3 -L${SYSROOT}/usr/lib -I${SYSROOT}/include -Wl,-rpath=${SYSROOT}/lib -Wl,--dynamic-linker=${SYSROOT}/lib/ld-2.24.90.so -S In file included from /home/max/install/glibc//include/bits/libc-header-start.h:33:0,
                 from /home/max/install/glibc//include/stdio.h:28,
                 from test.c:1:
/home/max/install/glibc//include/features.h:374:3: warning: #warning _FORTIFY_SOURCE is not compatible with sanitizer [-Wcpp]
 # warning _FORTIFY_SOURCE is not compatible with sanitizer


~/install/master/bin/gcc test.c -fsanitize=address -D_FORTIFY_SOURCE=2 -O3 -L${SYSROOT}/usr/lib -I${SYSROOT}/include -Wl,-rpath=${SYSROOT}/lib -Wl,--dynamic-linker=${SYSROOT}/lib/ld-2.24.90.so -S In file included from /home/max/install/glibc//include/bits/libc-header-start.h:33:0,
                 from /home/max/install/glibc//include/stdio.h:28,
                 from test.c:1:
/home/max/install/glibc//include/features.h:374:3: warning: #warning _FORTIFY_SOURCE is not compatible with sanitizer [-Wcpp]
 # warning _FORTIFY_SOURCE is not compatible with sanitizer

clang test.c -fsanitize=address -D_FORTIFY_SOURCE=2 -O3 -L${SYSROOT}/usr/lib -I${SYSROOT}/include -Wl,-rpath=${SYSROOT}/lib -Wl,--dynamic-linker=${SYSROOT}/lib/ld-2.24.90.so
In file included from test.c:1:
In file included from /home/max/install/glibc//include/stdio.h:28:
In file included from /home/max/install/glibc//include/bits/libc-header-start.h:33: /home/max/install/glibc//include/features.h:374:3: warning: _FORTIFY_SOURCE is not compatible with sanitizer [-W#warnings]
# warning _FORTIFY_SOURCE is not compatible with sanitizer
  ^
1 warning generated.


-Maxim
commit b153492ed25b1bc70141566e6644897b89a82e26
Author: Maxim Ostapenko <m.ostapenko@samsung.com>
Date:   Wed Aug 31 15:29:49 2016 +0300

    Do not allow asan/msan/tsan and fortify at the same time.

    When fortify is used with msan it will cause msan false positives. This is
    happening because fortify replaces libc functions (e.g. sprintf) with its own
    variants (__sprintf_chk) and the sanitizers don't know about these variants.
    Supporting fortify in *san makes little sense because fortify does not add
    anything to the sanitizers and it will only increase the complexity. So, the
    better way is to warn the user that the sanitizers and fortify are
    incompatible.

	[BZ #20422]
	* include/features.h (__SANITIZER_ENABLED): New macros. Define it if
	compiler has {A, T, M}San enabled. Warn if __SANITIZER_ENABLED and
	_FORTIFY_SOURCE are both defined.

diff --git a/ChangeLog b/ChangeLog
index d24536b..ee513cd 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,10 @@
+2016-09-05  Maxim Ostapenko  <m.ostapenko@samsung.com>
+
+	[BZ #20422]
+	* include/features.h (__SANITIZER_ENABLED): New macros. Define it if
+	compiler has {A, T, M}San enabled. Warn if __SANITIZER_ENABLED and
+	_FORTIFY_SOURCE are both defined.
+
 2016-08-30  Svante Signell  <svante.signell@gmail.com>
 
 	* sysdeps/mach/hurd/adjtime.c (__adjtime): When OLDDELTA is NULL, make
diff --git a/include/features.h b/include/features.h
index 650d4c5..1158e5a 100644
--- a/include/features.h
+++ b/include/features.h
@@ -355,11 +355,23 @@
 # define __USE_REENTRANT	1
 #endif
 
+#if !defined(__has_feature)
+# define __has_feature(x) 0
+#endif
+
+#if defined __SANITIZE_ADDRESS__ || defined __SANITIZE_THREAD__		    \
+    || __has_feature(address_sanitizer) || __has_feature(thread_sanitizer)  \
+    || __has_feature(memory_sanitizer)
+# define __SANITIZER_ENABLED 1
+#endif
+
 #if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0
 # if !defined __OPTIMIZE__ || __OPTIMIZE__ <= 0
 #  warning _FORTIFY_SOURCE requires compiling with optimization (-O)
 # elif !__GNUC_PREREQ (4, 1)
 #  warning _FORTIFY_SOURCE requires GCC 4.1 or later
+# elif defined __SANITIZER_ENABLED
+# warning _FORTIFY_SOURCE is not compatible with sanitizer
 # elif _FORTIFY_SOURCE > 1
 #  define __USE_FORTIFY_LEVEL 2
 # else

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]