From 873cc218108826e4dd4413dccd5c5f6cb0757c84 Mon Sep 17 00:00:00 2001 From: David Justo Date: Wed, 27 Nov 2024 13:16:12 -0800 Subject: [PATCH 1/9] honor allocator_may_return_null when set through user-function in windows --- compiler-rt/lib/asan/asan_flags.cpp | 7 +++++ .../sanitizer_common/sanitizer_allocator.cpp | 6 ++++- .../lib/sanitizer_common/sanitizer_win.cpp | 18 ++++++++++++- ..._may_return_null_set_via_user_function.cpp | 26 +++++++++++++++++++ .../allocator_may_return_null_limits.cpp | 22 ++++++++++++++++ 5 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp create mode 100644 compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp diff --git a/compiler-rt/lib/asan/asan_flags.cpp b/compiler-rt/lib/asan/asan_flags.cpp index 56deb1b0d082b..1499c63fd1901 100644 --- a/compiler-rt/lib/asan/asan_flags.cpp +++ b/compiler-rt/lib/asan/asan_flags.cpp @@ -240,6 +240,13 @@ void InitializeFlags() { DisplayHelpMessages(&asan_parser); ProcessFlags(); + + // TODO: Update other globals and data structures that may change after + // initialization due to these flags potentially changing. + // These flags can be identified by looking at the work done in + // `AsanInitInternal` inside of `asan_rtl.cpp`. + // See GH issue 'https://github.com/llvm/llvm-project/issues/117925' for details. + SetAllocatorMayReturnNull(common_flags()->allocator_may_return_null); }); # if CAN_SANITIZE_UB diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp index 9d899371c2dd9..b8c772d7df6c6 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp @@ -85,8 +85,12 @@ static void NORETURN ReportInternalAllocatorOutOfMemory(uptr requested_size) { void *InternalAlloc(uptr size, InternalAllocatorCache *cache, uptr alignment) { void *p = RawInternalAlloc(size, cache, alignment); - if (UNLIKELY(!p)) + if (UNLIKELY(!p)) { + if (AllocatorMayReturnNull()){ + return nullptr; + } ReportInternalAllocatorOutOfMemory(size); + } return p; } diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp index ea513d5f263fe..045a9a9d7b0b2 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp @@ -164,7 +164,23 @@ void UnmapOrDie(void *addr, uptr size, bool raw_report) { static void *ReturnNullptrOnOOMOrDie(uptr size, const char *mem_type, const char *mmap_type) { error_t last_error = GetLastError(); - if (last_error == ERROR_NOT_ENOUGH_MEMORY) + + // Assumption: VirtualAlloc is the last system call that was invoked before + // this method. + // VirtualAlloc emits one of 2 error codes when running out of memory + // 1. ERROR_NOT_ENOUGH_MEMORY: + // There's not enough memory to execute the command + // 2. ERROR_INVALID_PARAMETER: + // VirtualAlloc will return this if the request would allocate memory at an + // address exceeding or being very close to the maximum application address + // (the `lpMaximumApplicationAddress` field within the `SystemInfo` struct). + // This does not seem to be officially documented, but is corroborated here: + // https://stackoverflow.com/questions/45833674/why-does-virtualalloc-fail-for-lpaddress-greater-than-0x6ffffffffff + + // Note - It's possible that 'ERROR_COMMITMENT_LIMIT' needs to be handled here as well. + // It is currently not handled due to the lack of a reproducer that induces the error code. + if (last_error == ERROR_NOT_ENOUGH_MEMORY || + last_error == ERROR_INVALID_PARAMETER) return nullptr; ReportMmapFailureAndDie(size, mem_type, mmap_type, last_error); } diff --git a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp new file mode 100644 index 0000000000000..23af2224ec4f0 --- /dev/null +++ b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp @@ -0,0 +1,26 @@ +// RUN: %clangxx_asan -O0 %s -o %t +// RUN: %run %t 2>&1 +// CHECK: Success + +#include +#include +#include + +// On Windows, flags configured through the user-defined function `__asan_default_options` +// are suspected to not always be honored according to this GH issue: +// https://github.com/llvm/llvm-project/issues/117925 +// This issue is resolved for the `allocator_may_return_null` flag, but not for all others. +// This test ensures we do not regress on `allocator_may_return_null` specifically. +extern "C" __declspec(dllexport) extern const char *__asan_default_options() { + return "allocator_may_return_null=1"; +} + +int main() { + // Attempt to allocate an excessive amount of memory, which should + // terminate the program unless `allocator_may_return_null` is set. + size_t max = std::numeric_limits::max(); + + free(malloc(max)); + printf("Success"); + return 0; +} diff --git a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp new file mode 100644 index 0000000000000..515ac2643e161 --- /dev/null +++ b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp @@ -0,0 +1,22 @@ +// RUN: %clangxx_asan -O0 %s -o %t +// RUN: %env_asan_opts=allocator_may_return_null=0 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK1 +// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK2 + +// CHECK1: exceeds maximum supported size +// CHECK1: ABORT + +// CHECK2: Success + +#include +#include +#include + +int main() { + // Attempt to allocate an excessive amount of memory, which should + // terminate the program unless `allocator_may_return_null` is set. + size_t max = std::numeric_limits::max(); + + free(malloc(max)); + printf("Success"); + return 0; +} From 7e7dbfb7fc22e9e57bf5f6dca2c43fc9a2244d52 Mon Sep 17 00:00:00 2001 From: David Justo Date: Wed, 27 Nov 2024 13:37:29 -0800 Subject: [PATCH 2/9] simplify comment in weak function callback --- compiler-rt/lib/asan/asan_flags.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/compiler-rt/lib/asan/asan_flags.cpp b/compiler-rt/lib/asan/asan_flags.cpp index 1499c63fd1901..e055d72d7cdb8 100644 --- a/compiler-rt/lib/asan/asan_flags.cpp +++ b/compiler-rt/lib/asan/asan_flags.cpp @@ -241,10 +241,9 @@ void InitializeFlags() { DisplayHelpMessages(&asan_parser); ProcessFlags(); - // TODO: Update other globals and data structures that may change after - // initialization due to these flags potentially changing. - // These flags can be identified by looking at the work done in - // `AsanInitInternal` inside of `asan_rtl.cpp`. + // TODO: Update other globals and data structures that may need to change + // after initialization due to new flags potentially being set changing after + // `__asan_default_options` is registered. // See GH issue 'https://github.com/llvm/llvm-project/issues/117925' for details. SetAllocatorMayReturnNull(common_flags()->allocator_may_return_null); }); From 71918b10be6ecbab6acab42752d01bd607b2b220 Mon Sep 17 00:00:00 2001 From: David Justo Date: Wed, 27 Nov 2024 13:47:14 -0800 Subject: [PATCH 3/9] incorporate clang-format feedback --- compiler-rt/lib/asan/asan_flags.cpp | 3 ++- compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp | 2 +- compiler-rt/lib/sanitizer_common/sanitizer_win.cpp | 5 +++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/compiler-rt/lib/asan/asan_flags.cpp b/compiler-rt/lib/asan/asan_flags.cpp index e055d72d7cdb8..9cfb70bd00c78 100644 --- a/compiler-rt/lib/asan/asan_flags.cpp +++ b/compiler-rt/lib/asan/asan_flags.cpp @@ -244,7 +244,8 @@ void InitializeFlags() { // TODO: Update other globals and data structures that may need to change // after initialization due to new flags potentially being set changing after // `__asan_default_options` is registered. - // See GH issue 'https://github.com/llvm/llvm-project/issues/117925' for details. + // See GH issue 'https://github.com/llvm/llvm-project/issues/117925' for + // details. SetAllocatorMayReturnNull(common_flags()->allocator_may_return_null); }); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp index b8c772d7df6c6..cef175c6f6641 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp @@ -86,7 +86,7 @@ static void NORETURN ReportInternalAllocatorOutOfMemory(uptr requested_size) { void *InternalAlloc(uptr size, InternalAllocatorCache *cache, uptr alignment) { void *p = RawInternalAlloc(size, cache, alignment); if (UNLIKELY(!p)) { - if (AllocatorMayReturnNull()){ + if (AllocatorMayReturnNull()) { return nullptr; } ReportInternalAllocatorOutOfMemory(size); diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp index 045a9a9d7b0b2..1eef16fbde3e7 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp @@ -177,8 +177,9 @@ static void *ReturnNullptrOnOOMOrDie(uptr size, const char *mem_type, // This does not seem to be officially documented, but is corroborated here: // https://stackoverflow.com/questions/45833674/why-does-virtualalloc-fail-for-lpaddress-greater-than-0x6ffffffffff - // Note - It's possible that 'ERROR_COMMITMENT_LIMIT' needs to be handled here as well. - // It is currently not handled due to the lack of a reproducer that induces the error code. + // Note - It's possible that 'ERROR_COMMITMENT_LIMIT' needs to be handled here + // as well. It is currently not handled due to the lack of a reproducer that + // induces the error code. if (last_error == ERROR_NOT_ENOUGH_MEMORY || last_error == ERROR_INVALID_PARAMETER) return nullptr; From 9de99222eb07a2aefdb7532e81ce37ee8b808730 Mon Sep 17 00:00:00 2001 From: David Justo Date: Wed, 27 Nov 2024 14:04:41 -0800 Subject: [PATCH 4/9] add cstdio for printf --- .../Windows/allocator_may_return_null_set_via_user_function.cpp | 1 + .../test/asan/TestCases/allocator_may_return_null_limits.cpp | 1 + 2 files changed, 2 insertions(+) diff --git a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp index 23af2224ec4f0..6169e42ab9cbd 100644 --- a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp +++ b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp @@ -3,6 +3,7 @@ // CHECK: Success #include +#include #include #include diff --git a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp index 515ac2643e161..b9fd0a7144ea8 100644 --- a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp +++ b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp @@ -8,6 +8,7 @@ // CHECK2: Success #include +#include #include #include From ebc3d0df3d55f1e03add8007970042b566954d32 Mon Sep 17 00:00:00 2001 From: David Justo Date: Mon, 9 Dec 2024 10:35:20 -0800 Subject: [PATCH 5/9] incorporate PR feedback: merge tests --- ..._may_return_null_set_via_user_function.cpp | 27 ------------------- .../allocator_may_return_null_limits.cpp | 24 ++++++++++++----- 2 files changed, 17 insertions(+), 34 deletions(-) delete mode 100644 compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp diff --git a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp deleted file mode 100644 index 6169e42ab9cbd..0000000000000 --- a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp +++ /dev/null @@ -1,27 +0,0 @@ -// RUN: %clangxx_asan -O0 %s -o %t -// RUN: %run %t 2>&1 -// CHECK: Success - -#include -#include -#include -#include - -// On Windows, flags configured through the user-defined function `__asan_default_options` -// are suspected to not always be honored according to this GH issue: -// https://github.com/llvm/llvm-project/issues/117925 -// This issue is resolved for the `allocator_may_return_null` flag, but not for all others. -// This test ensures we do not regress on `allocator_may_return_null` specifically. -extern "C" __declspec(dllexport) extern const char *__asan_default_options() { - return "allocator_may_return_null=1"; -} - -int main() { - // Attempt to allocate an excessive amount of memory, which should - // terminate the program unless `allocator_may_return_null` is set. - size_t max = std::numeric_limits::max(); - - free(malloc(max)); - printf("Success"); - return 0; -} diff --git a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp index b9fd0a7144ea8..3ad167aaa5f13 100644 --- a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp +++ b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp @@ -1,14 +1,21 @@ // RUN: %clangxx_asan -O0 %s -o %t -// RUN: %env_asan_opts=allocator_may_return_null=0 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK1 -// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK2 +// RUN: %env_asan_opts=allocator_may_return_null=0 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-ABORT +// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RETURN_NULL -// CHECK1: exceeds maximum supported size -// CHECK1: ABORT +// RUN: %clangxx_asan -O0 %s -o %t -DUSER_FUNCTION +// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RETURN_NULL -// CHECK2: Success +#if USER_FUNCTION +// On Windows, flags configured through the user-defined function `__asan_default_options` +// are suspected to not always be honored according to GitHub bug: +// https://github.com/llvm/llvm-project/issues/117925 +// This test ensures we do not regress on `allocator_may_return_null` specifically. +extern "C" __declspec(dllexport) extern const char *__asan_default_options() { + return "allocator_may_return_null=1"; +} +#endif #include -#include #include #include @@ -17,7 +24,10 @@ int main() { // terminate the program unless `allocator_may_return_null` is set. size_t max = std::numeric_limits::max(); + // CHECK-ABORT: exceeds maximum supported size + // CHECK-ABORT: ABORT free(malloc(max)); - printf("Success"); + + printf("Success"); // CHECK-RETURN_NULL: Success return 0; } From 7f0e5c0adb8fa4e0e238751a5df4e3640c6a72ac Mon Sep 17 00:00:00 2001 From: David Justo Date: Mon, 9 Dec 2024 11:02:34 -0800 Subject: [PATCH 6/9] revert changes to sanitizer_allocator --- compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp index cef175c6f6641..9d899371c2dd9 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp @@ -85,12 +85,8 @@ static void NORETURN ReportInternalAllocatorOutOfMemory(uptr requested_size) { void *InternalAlloc(uptr size, InternalAllocatorCache *cache, uptr alignment) { void *p = RawInternalAlloc(size, cache, alignment); - if (UNLIKELY(!p)) { - if (AllocatorMayReturnNull()) { - return nullptr; - } + if (UNLIKELY(!p)) ReportInternalAllocatorOutOfMemory(size); - } return p; } From caac8b534c7e22f6ce661cb708b36eaa0fbb9074 Mon Sep 17 00:00:00 2001 From: David Justo Date: Mon, 9 Dec 2024 14:31:05 -0800 Subject: [PATCH 7/9] add missing cstdio --- .../test/asan/TestCases/allocator_may_return_null_limits.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp index 3ad167aaa5f13..00eba4c340fa8 100644 --- a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp +++ b/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp @@ -16,6 +16,7 @@ extern "C" __declspec(dllexport) extern const char *__asan_default_options() { #endif #include +#include #include #include From 4f8607712c5e9eafef1a9abacb94fe9457eeacdd Mon Sep 17 00:00:00 2001 From: David Justo Date: Mon, 9 Dec 2024 21:51:43 -0800 Subject: [PATCH 8/9] move allocator_may_return_null_limits to windows test folder --- .../TestCases/{ => Windows}/allocator_may_return_null_limits.cpp | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename compiler-rt/test/asan/TestCases/{ => Windows}/allocator_may_return_null_limits.cpp (100%) diff --git a/compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp similarity index 100% rename from compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp rename to compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp From 1f66929b1991827bdd068bd5b8f2c1afe72b129e Mon Sep 17 00:00:00 2001 From: David Justo Date: Tue, 10 Dec 2024 09:36:06 -0800 Subject: [PATCH 9/9] incorporate PR feedback --- .../TestCases/Windows/allocator_may_return_null_limits.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp index 00eba4c340fa8..90db0d45af2d7 100644 --- a/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp +++ b/compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp @@ -1,9 +1,9 @@ // RUN: %clangxx_asan -O0 %s -o %t // RUN: %env_asan_opts=allocator_may_return_null=0 not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-ABORT -// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RETURN_NULL +// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RETURN-NULL // RUN: %clangxx_asan -O0 %s -o %t -DUSER_FUNCTION -// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RETURN_NULL +// RUN: %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RETURN-NULL #if USER_FUNCTION // On Windows, flags configured through the user-defined function `__asan_default_options` @@ -29,6 +29,6 @@ int main() { // CHECK-ABORT: ABORT free(malloc(max)); - printf("Success"); // CHECK-RETURN_NULL: Success + printf("Success"); // CHECK-RETURN-NULL: Success return 0; }