[compiler-rt] [ASan] Honor `allocator_may_return_null` when set through user-function and fix large alloc edge case (PR #117929)
David Justo via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 9 14:31:29 PST 2024
https://github.com/davidmrdavid updated https://github.com/llvm/llvm-project/pull/117929
>From 873cc218108826e4dd4413dccd5c5f6cb0757c84 Mon Sep 17 00:00:00 2001
From: David Justo <dajusto at microsoft.com>
Date: Wed, 27 Nov 2024 13:16:12 -0800
Subject: [PATCH 1/7] 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 56deb1b0d082b8..1499c63fd19014 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 9d899371c2dd90..b8c772d7df6c68 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 ea513d5f263fe2..045a9a9d7b0b2d 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 00000000000000..23af2224ec4f08
--- /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 <cstdint>
+#include <cstdlib>
+#include <limits>
+
+// 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<size_t>::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 00000000000000..515ac2643e161a
--- /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 <cstdint>
+#include <cstdlib>
+#include <limits>
+
+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<size_t>::max();
+
+ free(malloc(max));
+ printf("Success");
+ return 0;
+}
>From 7e7dbfb7fc22e9e57bf5f6dca2c43fc9a2244d52 Mon Sep 17 00:00:00 2001
From: David Justo <dajusto at microsoft.com>
Date: Wed, 27 Nov 2024 13:37:29 -0800
Subject: [PATCH 2/7] 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 1499c63fd19014..e055d72d7cdb82 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 <dajusto at microsoft.com>
Date: Wed, 27 Nov 2024 13:47:14 -0800
Subject: [PATCH 3/7] 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 e055d72d7cdb82..9cfb70bd00c786 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 b8c772d7df6c68..cef175c6f66415 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 045a9a9d7b0b2d..1eef16fbde3e7d 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 <dajusto at microsoft.com>
Date: Wed, 27 Nov 2024 14:04:41 -0800
Subject: [PATCH 4/7] 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 23af2224ec4f08..6169e42ab9cbd8 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 <cstdint>
+#include <cstdio>
#include <cstdlib>
#include <limits>
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 515ac2643e161a..b9fd0a7144ea8a 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 <cstdint>
+#include <cstdio>
#include <cstdlib>
#include <limits>
>From ebc3d0df3d55f1e03add8007970042b566954d32 Mon Sep 17 00:00:00 2001
From: David Justo <dajusto at microsoft.com>
Date: Mon, 9 Dec 2024 10:35:20 -0800
Subject: [PATCH 5/7] 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 6169e42ab9cbd8..00000000000000
--- 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 <cstdint>
-#include <cstdio>
-#include <cstdlib>
-#include <limits>
-
-// 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<size_t>::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 b9fd0a7144ea8a..3ad167aaa5f13d 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 <cstdint>
-#include <cstdio>
#include <cstdlib>
#include <limits>
@@ -17,7 +24,10 @@ int main() {
// terminate the program unless `allocator_may_return_null` is set.
size_t max = std::numeric_limits<size_t>::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 <dajusto at microsoft.com>
Date: Mon, 9 Dec 2024 11:02:34 -0800
Subject: [PATCH 6/7] 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 cef175c6f66415..9d899371c2dd90 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 <dajusto at microsoft.com>
Date: Mon, 9 Dec 2024 14:31:05 -0800
Subject: [PATCH 7/7] 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 3ad167aaa5f13d..00eba4c340fa88 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 <cstdint>
+#include <cstdio>
#include <cstdlib>
#include <limits>
More information about the llvm-commits
mailing list