From fe7f65b081d5d0b32270cb1a82e2ee9a0391a6d1 Mon Sep 17 00:00:00 2001 From: Russell Gallop Date: Thu, 10 Oct 2019 10:56:52 +0000 Subject: Revert "[ASan] Do not misrepresent high value address dereferences as null dereferences" As it was breaking bots running sanitizer lint check This reverts r374265 (git b577efe4567f1f6a711ad36e1d17280dd1c4f009) git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@374308 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/asan/asan_errors.h | 3 +- lib/sanitizer_common/sanitizer_common.h | 9 +--- lib/sanitizer_common/sanitizer_linux.cpp | 6 --- lib/sanitizer_common/sanitizer_mac.cpp | 6 --- .../sanitizer_symbolizer_report.cpp | 17 ++------ lib/sanitizer_common/sanitizer_win.cpp | 5 --- .../TestCases/Posix/high-address-dereference.c | 50 ---------------------- 7 files changed, 6 insertions(+), 90 deletions(-) delete mode 100644 test/asan/TestCases/Posix/high-address-dereference.c diff --git a/lib/asan/asan_errors.h b/lib/asan/asan_errors.h index a7fda2fd9..b84f56c18 100644 --- a/lib/asan/asan_errors.h +++ b/lib/asan/asan_errors.h @@ -48,8 +48,7 @@ struct ErrorDeadlySignal : ErrorBase { scariness.Scare(10, "stack-overflow"); } else if (!signal.is_memory_access) { scariness.Scare(10, "signal"); - } else if (signal.is_true_faulting_addr && - signal.addr < GetPageSizeCached()) { + } else if (signal.addr < GetPageSizeCached()) { scariness.Scare(10, "null-deref"); } else if (signal.addr == signal.pc) { scariness.Scare(60, "wild-jump"); diff --git a/lib/sanitizer_common/sanitizer_common.h b/lib/sanitizer_common/sanitizer_common.h index 87b8f02b5..ad056df38 100644 --- a/lib/sanitizer_common/sanitizer_common.h +++ b/lib/sanitizer_common/sanitizer_common.h @@ -881,11 +881,6 @@ struct SignalContext { bool is_memory_access; enum WriteFlag { UNKNOWN, READ, WRITE } write_flag; - // In some cases the kernel cannot provide the true faulting address; `addr` - // will be zero then. This field allows to distinguish between these cases - // and dereferences of null. - bool is_true_faulting_addr; - // VS2013 doesn't implement unrestricted unions, so we need a trivial default // constructor SignalContext() = default; @@ -898,8 +893,7 @@ struct SignalContext { context(context), addr(GetAddress()), is_memory_access(IsMemoryAccess()), - write_flag(GetWriteFlag()), - is_true_faulting_addr(IsTrueFaultingAddress()) { + write_flag(GetWriteFlag()) { InitPcSpBp(); } @@ -920,7 +914,6 @@ struct SignalContext { uptr GetAddress() const; WriteFlag GetWriteFlag() const; bool IsMemoryAccess() const; - bool IsTrueFaultingAddress() const; }; void InitializePlatformEarly(); diff --git a/lib/sanitizer_common/sanitizer_linux.cpp b/lib/sanitizer_common/sanitizer_linux.cpp index 0b53da6c3..d23009075 100644 --- a/lib/sanitizer_common/sanitizer_linux.cpp +++ b/lib/sanitizer_common/sanitizer_linux.cpp @@ -1849,12 +1849,6 @@ SignalContext::WriteFlag SignalContext::GetWriteFlag() const { #endif } -bool SignalContext::IsTrueFaultingAddress() const { - auto si = static_cast(siginfo); - // SIGSEGV signals without a true fault address have si_code set to 128. - return si->si_signo == SIGSEGV && si->si_code != 128; -} - void SignalContext::DumpAllRegisters(void *context) { // FIXME: Implement this. } diff --git a/lib/sanitizer_common/sanitizer_mac.cpp b/lib/sanitizer_common/sanitizer_mac.cpp index ea4bd02aa..8eb1dfbde 100644 --- a/lib/sanitizer_common/sanitizer_mac.cpp +++ b/lib/sanitizer_common/sanitizer_mac.cpp @@ -754,12 +754,6 @@ SignalContext::WriteFlag SignalContext::GetWriteFlag() const { #endif } -bool SignalContext::IsTrueFaultingAddress() const { - auto si = static_cast(siginfo); - // "Real" SIGSEGV codes (e.g., SEGV_MAPERR, SEGV_MAPERR) are non-zero. - return si->si_signo == SIGSEGV && si->si_code != 0; -} - static void GetPcSpBp(void *context, uptr *pc, uptr *sp, uptr *bp) { ucontext_t *ucontext = (ucontext_t*)context; # if defined(__aarch64__) diff --git a/lib/sanitizer_common/sanitizer_symbolizer_report.cpp b/lib/sanitizer_common/sanitizer_symbolizer_report.cpp index fe9ea1a82..d6699f3ed 100644 --- a/lib/sanitizer_common/sanitizer_symbolizer_report.cpp +++ b/lib/sanitizer_common/sanitizer_symbolizer_report.cpp @@ -191,14 +191,9 @@ static void ReportDeadlySignalImpl(const SignalContext &sig, u32 tid, SanitizerCommonDecorator d; Printf("%s", d.Warning()); const char *description = sig.Describe(); - if (sig.is_memory_access && !sig.is_true_faulting_addr) - Report("ERROR: %s: %s on unknown address (pc %p bp %p sp %p T%d)\n", - SanitizerToolName, description, (void *)sig.pc, (void *)sig.bp, - (void *)sig.sp, tid); - else - Report("ERROR: %s: %s on unknown address %p (pc %p bp %p sp %p T%d)\n", - SanitizerToolName, description, (void *)sig.addr, (void *)sig.pc, - (void *)sig.bp, (void *)sig.sp, tid); + Report("ERROR: %s: %s on unknown address %p (pc %p bp %p sp %p T%d)\n", + SanitizerToolName, description, (void *)sig.addr, (void *)sig.pc, + (void *)sig.bp, (void *)sig.sp, tid); Printf("%s", d.Default()); if (sig.pc < GetPageSizeCached()) Report("Hint: pc points to the zero page.\n"); @@ -208,11 +203,7 @@ static void ReportDeadlySignalImpl(const SignalContext &sig, u32 tid, ? "WRITE" : (sig.write_flag == SignalContext::READ ? "READ" : "UNKNOWN"); Report("The signal is caused by a %s memory access.\n", access_type); - if (!sig.is_true_faulting_addr) - Report("Hint: this fault was caused by a dereference of a high value " - "address (see registers below). Dissassemble the provided pc " - "to learn which register value was used.\n"); - else if (sig.addr < GetPageSizeCached()) + if (sig.addr < GetPageSizeCached()) Report("Hint: address points to the zero page.\n"); } MaybeReportNonExecRegion(sig.pc); diff --git a/lib/sanitizer_common/sanitizer_win.cpp b/lib/sanitizer_common/sanitizer_win.cpp index 95e788217..ce2a4314a 100644 --- a/lib/sanitizer_common/sanitizer_win.cpp +++ b/lib/sanitizer_common/sanitizer_win.cpp @@ -945,11 +945,6 @@ bool SignalContext::IsMemoryAccess() const { return GetWriteFlag() != SignalContext::UNKNOWN; } -bool SignalContext::IsTrueFaultingAddress() const { - // TODO: Provide real implementation for this. See Linux and Mac variants. - return IsMemoryAccess(); -} - SignalContext::WriteFlag SignalContext::GetWriteFlag() const { EXCEPTION_RECORD *exception_record = (EXCEPTION_RECORD *)siginfo; // The contents of this array are documented at diff --git a/test/asan/TestCases/Posix/high-address-dereference.c b/test/asan/TestCases/Posix/high-address-dereference.c deleted file mode 100644 index 785033028..000000000 --- a/test/asan/TestCases/Posix/high-address-dereference.c +++ /dev/null @@ -1,50 +0,0 @@ -// On x86_64, the kernel does not provide the faulting address for dereferences -// of addresses greater than the 48-bit hardware addressable range, i.e., -// `siginfo.si_addr` is zero in ASan's SEGV signal handler. This test checks -// that ASan does not misrepresent such cases as "NULL dereferences". - -// REQUIRES: x86_64-target-arch -// RUN: %clang_asan %s -o %t -// RUN: export %env_asan_opts=print_scariness=1 -// RUN: not %run %t 0x0000000000000000 2>&1 | FileCheck %s --check-prefixes=ZERO,HINT-PAGE0 -// RUN: not %run %t 0x0000000000000FFF 2>&1 | FileCheck %s --check-prefixes=LOW1,HINT-PAGE0 -// RUN: not %run %t 0x0000000000001000 2>&1 | FileCheck %s --check-prefixes=LOW2,HINT-NONE -// RUN: not %run %t 0x4141414141414141 2>&1 | FileCheck %s --check-prefixes=HIGH,HINT-HIGHADDR -// RUN: not %run %t 0xFFFFFFFFFFFFFFFF 2>&1 | FileCheck %s --check-prefixes=MAX,HINT-HIGHADDR - -#include -#include - -int main(int argc, const char *argv[]) { - const char *hex = argv[1]; - uint64_t *addr = (uint64_t *)strtoull(hex, NULL, 16); - uint64_t x = *addr; // segmentation fault - return x; -} - -// ZERO: SEGV on unknown address 0x000000000000 (pc -// LOW1: SEGV on unknown address 0x000000000fff (pc -// LOW2: SEGV on unknown address 0x000000001000 (pc -// HIGH: SEGV on unknown address (pc -// MAX: SEGV on unknown address (pc - -// HINT-PAGE0-NOT: Hint: this fault was caused by a dereference of a high value address -// HINT-PAGE0: Hint: address points to the zero page. - -// HINT-NONE-NOT: Hint: this fault was caused by a dereference of a high value address -// HINT-NONE-NOT: Hint: address points to the zero page. - -// HINT-HIGHADDR: Hint: this fault was caused by a dereference of a high value address -// HINT-HIGHADDR-NOT: Hint: address points to the zero page. - -// ZERO: SCARINESS: 10 (null-deref) -// LOW1: SCARINESS: 10 (null-deref) -// LOW2: SCARINESS: 20 (wild-addr-read) -// HIGH: SCARINESS: 20 (wild-addr-read) -// MAX: SCARINESS: 20 (wild-addr-read) - -// TODO: Currently, register values are only printed on Mac. Once this changes, -// remove the 'TODO_' prefix in the following lines. -// TODO_HIGH,TODO_MAX: Register values: -// TODO_HIGH: = 0x4141414141414141 -// TODO_MAX: = 0xffffffffffffffff -- cgit v1.2.1