Add fgets, fputs and puts to sanitizer_common. This adds ASAN coverage
for these functions, extends MSAN support from fgets to fputs/puts and
extends TSAN support from puts to fputs.
Details
Diff Detail
- Repository
- rCRT Compiler Runtime
Event Timeline
Why it's "cost of false negatives for MSAN" ?
test/asan/TestCases/Posix/fgets_fputs.cc | ||
---|---|---|
3 | could you please also create sanitizer_common test for each of moved functions? |
It looks fine. It would be nicer to use separate tests for each function. Not all in one.
If a "good weather" test has to be added, I'll probably combine fgets/fputs/gets in a single sanitizer_common test.
In a future patch, I'll split the existing ASAN test into three separate programs.
lib/esan/esan_interceptors.cpp | ||
---|---|---|
497 | FYI: this seemed a leftover after https://reviews.llvm.org/D31456 (https://github.com/google/sanitizers/issues/793) | |
lib/msan/msan_interceptors.cc | ||
753 | FYI: This was added in https://reviews.llvm.org/D42884, but I believe that the COMMON_INTERCEPTOR_ENTER macro in lib/msan/msan_interceptors.cc covers this now. | |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
1203 | This is what I meant by "false negatives for msan". Possible choices for the length parameter:
| |
test/asan/TestCases/Posix/fgets_fputs.cc | ||
3 | Do you mean a test that checks that everything works fine with valid boundaries, for example test/sanitizer_common/TestCases/Posix/access.cc? TSAN already has an additional test in test/tsan/race_on_puts.c. Should I create a similar race_on_fgets/race_on_fputs? It seems that the previous fgets interceptor in the msan was not tested either, that could also be a point of improvement. |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
1203 | Let not mix refactoring with behavior change. | |
test/asan/TestCases/Posix/fgets_fputs.cc | ||
3 | Yes, it's hard to test reports in sanitizer_common |
We are now requiring this on NetBSD for MSanitized userland. Some programs break without puts(3) / fputs(3) interceptors there.
Is there a link/discussion to this issue? (f)puts just read from a buffer, how would lack of interceptors cause MSan to break?
I found https://github.com/google/sanitizers/issues/955, but it does not directly mention such an issue.
(This change is still on my TODO list.)
https://github.com/google/sanitizers/issues/955 context "libfuzzer integration with the NetBSD userland #955"
There are so many issues with local utilities that we don't bother with reporting them.. as we need to fix them on our own.
https://github.com/plusun/compiler-rt/commit/bbcbdbd2d6714c6d2e4436b17e694c355670e58c our fix for this issue "Add interceptors for puts and fputs, remove old ones"
If I remember correctly, puts(3)/fputs(3) was breaking GNU groff in the LLVM style of NetBSD distribution. (CC: @tomsun.0.7 -- the author)
It causes breakage because there is called an interceptor inside libc that is reported as false positive.
Changes:
- Restore strlen(s)+1 length calculation for fgets(s, size, fp) as suggested (matching previous TSAN behavior).
- Add MSAN, TSAN test coverage for bad cases.
- Add sanitizer_common tests for good cases.
Changed since last patch: small fix to make the fputs test deterministically fail. No further test regressions caught during check-all.
--- a/test/asan/TestCases/Posix/fgets_fputs.cc +++ b/test/asan/TestCases/Posix/fgets_fputs.cc @@ -17,7 +17,7 @@ int test_fgets() { int test_fputs() { FILE *fp = fopen("/dev/null", "w"); - char buf[2]; + char buf[1] = { 'x' }; // Note: not nul-terminated fputs(buf, fp); // BOOM return fclose(fp); }
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
1197 | It seems still valid, COMMON_INTERCEPTOR_ENTER does not check the parameters, so REAL(fgets) below can overwrite invalid memory. Reproducer with fread (which has the same issue): #include <stdio.h> #include <stdlib.h> int main() { FILE *fp = fopen("/proc/cpuinfo", "r"); if (!fp) return 1; void *p = malloc(4096); if (!p) return 1; free(p); if (!fread(p, 4096, 1, fp)) perror("fread"); fclose(fp); return 0; } Trace: ==4458==ERROR: AddressSanitizer: heap-use-after-free on address 0x621000000100 at pc 0x00000048f8c0 bp 0x7ffe28520fa0 sp 0x7ffe28520750 WRITE of size 4096 at 0x621000000100 thread T0 #0 0x48f8bf in __interceptor_fread.part.52 projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:991:16 #1 0x5274eb in main fread.c:12:10 #2 0x7f6a35eb106a in __libc_start_main (/usr/lib/libc.so.6+0x2306a) #3 0x41d039 in _start (fread+0x41d039) 0x621000000100 is located 0 bytes inside of 4096-byte region [0x621000000100,0x621000001100) freed by thread T0 here: ==4458==AddressSanitizer CHECK failed: projects/compiler-rt/lib/asan/asan_descriptions.cc:179 "((res.trace)) != (0)" (0x0, 0x0) #0 0x4f9575 in __asan::AsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) projects/compiler-rt/lib/asan/asan_rtl.cc:70:3 #1 0x512009 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) projects/compiler-rt/lib/sanitizer_common/sanitizer_termination.cc:79:24 #2 0x42b264 in GetStackTraceFromId projects/compiler-rt/lib/asan/asan_descriptions.cc:179:3 #3 0x42b264 in __asan::HeapAddressDescription::Print() const projects/compiler-rt/lib/asan/asan_descriptions.cc:420:62 #4 0x42ea03 in __asan::AddressDescription::Print(char const*) const projects/compiler-rt/lib/asan/asan_descriptions.h:224:31 #5 0x42ea03 in __asan::ErrorGeneric::Print() projects/compiler-rt/lib/asan/asan_errors.cc:597:25 #6 0x4f9086 in __asan::ErrorDescription::Print() projects/compiler-rt/lib/asan/asan_errors.h:422:7 #7 0x4f9086 in __asan::ScopedInErrorReport::~ScopedInErrorReport() projects/compiler-rt/lib/asan/asan_report.cc:142:55 #8 0x4f9086 in __asan::ReportGenericError(unsigned long, unsigned long, unsigned long, unsigned long, bool, unsigned long, unsigned int, bool) projects/compiler-rt/lib/asan/asan_report.cc:460:38 #9 0x48f8e1 in __interceptor_fread.part.52 projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:991:16 #10 0x5274eb in main fread.c:12:10 #11 0x7f6a35eb106a in __libc_start_main (/usr/lib/libc.so.6+0x2306a) #12 0x41d039 in _start (fread+0x41d039) |
lib/sanitizer_common/sanitizer_common_interceptors.inc | ||
---|---|---|
1200 | I'd expect if (res) COMMON_INTERCEPTOR_WRITE_RANGE Could you try: git clang-format -f --style=file --extensions=inc,cc,h,c,cpp HEAD^ |
Thanks Vitaly, that's a very useful tool!
This version only has whitespace changes:
--- a/lib/sanitizer_common/sanitizer_common_interceptors.inc +++ b/lib/sanitizer_common/sanitizer_common_interceptors.inc @@ -1190,7 +1190,7 @@ INTERCEPTOR(SSIZE_T, pwritev64, int fd, __sanitizer_iovec *iov, int iovcnt, #endif #if SANITIZER_INTERCEPT_FGETS -INTERCEPTOR(char*, fgets, char *s, SIZE_T size, void *file) { +INTERCEPTOR(char *, fgets, char *s, SIZE_T size, void *file) { // libc file streams can call user-supplied functions, see fopencookie. void *ctx; COMMON_INTERCEPTOR_ENTER(ctx, fgets, s, size, file); @@ -1198,7 +1198,8 @@ INTERCEPTOR(char*, fgets, char *s, SIZE_T size, void *file) { // its metadata. See // https://github.com/google/sanitizers/issues/321. char *res = REAL(fgets)(s, size, file); - if (res) COMMON_INTERCEPTOR_WRITE_RANGE(ctx, s, REAL(strlen)(s) + 1); + if (res) + COMMON_INTERCEPTOR_WRITE_RANGE(ctx, s, REAL(strlen)(s) + 1); return res; } #define INIT_FGETS COMMON_INTERCEPT_FUNCTION(fgets) --- a/test/asan/TestCases/Posix/fgets_fputs.cc +++ b/test/asan/TestCases/Posix/fgets_fputs.cc @@ -17,8 +17,8 @@ int test_fgets() { int test_fputs() { FILE *fp = fopen("/dev/null", "w"); - char buf[1] = { 'x' }; // Note: not nul-terminated - fputs(buf, fp); // BOOM + char buf[1] = {'x'}; // Note: not nul-terminated + fputs(buf, fp); // BOOM return fclose(fp); }
This is breaking the Android sanitizer bot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/11563.
Please take a look and/or revert.
I had a look already and am trying to set up an environment to reproduce it. It however seems an impossible situation. Either the test suite fails to execute the commands correctly, or the Android runtime or test suite are stripping paths. It is not the first test that would fail on Android either: https://github.com/google/sanitizers/issues/316
Another issue that popped up is a failure on macOS: http://green.lab.llvm.org/green/job/clang-stage1-configure-RA/46011/
Perhaps this issue occurs because the runtime interceptor does not catch the symbol on macOS.
Since the code appears to be functionally correct, I am considering adding XFAIL for both cases.
For the record, this is the test output:
FAIL: AddressSanitizer-aarch64-android :: TestCases/Posix/fgets_fputs.cc (126 of 1137) ******************** TEST 'AddressSanitizer-aarch64-android :: TestCases/Posix/fgets_fputs.cc' FAILED ******************** Script: -- : 'RUN: at line 1'; /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/sanitizer_common/android_commands/android_compile.py /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm_build64/bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only --target=aarch64-linux-android --sysroot=/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/standalone-aarch64/sysroot -B/var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/android_ndk/standalone-aarch64 -pie -fuse-ld=gold -Wl,--enable-new-dtags -shared-libasan -g /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc -o /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_aarch64/test/asan/AARCH64AndroidConfig/TestCases/Posix/Output/fgets_fputs.cc.tmp : 'RUN: at line 2'; echo data > /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_aarch64/test/asan/AARCH64AndroidConfig/TestCases/Posix/Output/fgets_fputs.cc.tmp-testdata : 'RUN: at line 3'; not /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_aarch64/test/asan/AARCH64AndroidConfig/TestCases/Posix/Output/fgets_fputs.cc.tmp 1 /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_aarch64/test/asan/AARCH64AndroidConfig/TestCases/Posix/Output/fgets_fputs.cc.tmp-testdata 2>&1 | FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc --check-prefix=CHECK-FGETS : 'RUN: at line 4'; not /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_aarch64/test/asan/AARCH64AndroidConfig/TestCases/Posix/Output/fgets_fputs.cc.tmp 2 2>&1 | FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc --check-prefix=CHECK-FPUTS : 'RUN: at line 5'; not /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/compiler_rt_build_android_aarch64/test/asan/AARCH64AndroidConfig/TestCases/Posix/Output/fgets_fputs.cc.tmp 3 2>&1 | FileCheck /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc --check-prefix=CHECK-PUTS -- Exit Code: 1 Command Output (stderr): -- /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc:51:17: error: expected string not found in input // CHECK-FGETS: {{.*ERROR: AddressSanitizer: stack-buffer-overflow}} ^ <stdin>:1:1: note: scanning from here /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc:15: int test_fgets(const char *): assertion "fp" failed ^ <stdin>:1:3: note: possible intended match here /var/lib/buildbot/sanitizer-buildbot6/sanitizer-x86_64-linux-android/build/llvm/projects/compiler-rt/test/asan/TestCases/Posix/fgets_fputs.cc:15: int test_fgets(const char *): assertion "fp" failed ^
Please do this for now to get the bot green again. If you figure out the root cause you can submit a fix later.
FYI: this seemed a leftover after https://reviews.llvm.org/D31456 (https://github.com/google/sanitizers/issues/793)