Skip to content

[ASan] Honor allocator_may_return_null when set through user-function and fix large alloc edge case #117929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

davidmrdavid
Copy link
Contributor

@davidmrdavid davidmrdavid commented Nov 27, 2024

Related: #117925

About this PR:
This PR performs 3 small but related fixes for ASan users on Windows:

  1. It ensures that the allocator_may_return_null flag is honored when set through the user function __asan_default_options. For more details, please see: [Asan] ASan flags set through a user-defined __asan_default_options function may not be honored in Windows #117925
  2. It adds a missing AllocatorMayReturnNull() check inside InternalAlloc that's needed to avoid error'ing out when the allocator correctly returns null when allocator_may_return_null is set.
  3. In sanitizer_win's ReturnNullptrOnOOMOrDie, it allows returning null when the last error is set to ERROR_INVALID_PARAMETER which may be set by VirtualAlloc on WIndows when attempting to allocate exceedingly large memory.

I've added test cases that should cover these new behaviors. Happy to take on any feedback as well. Thank you :-)

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Nov 27, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: David Justo (davidmrdavid)

Changes

**Related: ** #117925

About this PR:
This PR performs 3 small but related fixes for ASan users on Windows:

  1. It ensures that the allocator_may_return_null flag is honored when set through the user function __asan_default_options. For more details, please see: #117925
  2. It adds a missing AllocatorMayReturnNull() check inside InternalAlloc that's needed to avoid error'ing out when the allocator correctly returns null when allocator_may_return_null is set.
  3. In sanitizer_win's ReturnNullptrOnOOMOrDie, it allows returning null when the last error is set to ERROR_INVALID_PARAMETER which may be set by VirtualAlloc on WIndows when attempting to allocate exceedingly large memory.

I've added test cases that should cover these new behaviors. Happy to take on any feedback as well. Thank you :-)


Full diff: https://github.com/llvm/llvm-project/pull/117929.diff

5 Files Affected:

  • (modified) compiler-rt/lib/asan/asan_flags.cpp (+7)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_allocator.cpp (+5-1)
  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_win.cpp (+17-1)
  • (added) compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_set_via_user_function.cpp (+26)
  • (added) compiler-rt/test/asan/TestCases/allocator_may_return_null_limits.cpp (+22)
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;
+}

@vitalybuka vitalybuka self-requested a review November 27, 2024 21:35
Copy link

github-actions bot commented Nov 27, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 2f55de4e317ee93cdca839558acf8be2b5ac2b46 1f66929b1991827bdd068bd5b8f2c1afe72b129e --extensions cpp -- compiler-rt/test/asan/TestCases/Windows/allocator_may_return_null_limits.cpp compiler-rt/lib/asan/asan_flags.cpp compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
View the diff from clang-format here.
diff --git a/compiler-rt/lib/asan/asan_flags.cpp b/compiler-rt/lib/asan/asan_flags.cpp
index 9cfb70bd00..52ef0b746e 100644
--- a/compiler-rt/lib/asan/asan_flags.cpp
+++ b/compiler-rt/lib/asan/asan_flags.cpp
@@ -241,8 +241,9 @@ void InitializeFlags() {
         DisplayHelpMessages(&asan_parser);
         ProcessFlags();
 
-        // TODO: Update other globals and data structures that may need to change
-        // after initialization due to new flags potentially being set changing after
+        // 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.

@davidmrdavid
Copy link
Contributor Author

Just leaving a friendly "Ping" here. Thanks!

// 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 ||
Copy link
Collaborator

@vitalybuka vitalybuka Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine, CC @barcharcraz @zmodem

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me to.

// 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 ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me to.

@@ -0,0 +1,27 @@
// RUN: %clangxx_asan -O0 %s -o %t
// RUN: %run %t 2>&1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to pipe the output through FileCheck for the CHECK comment below to do anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch, thanks for letting me know. I was somehow under the impression that the CHECK pattern was implicitly always checked. I merged the tests, based on your suggestion, so that address this issue as well.

#include <cstdlib>
#include <limits>

// On Windows, flags configured through the user-defined function `__asan_default_options`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and allocator_may_return_null_limits.cpp are really variations of the same test. I wonder if they could be folded together?

For example, the __asan_default_options() definition could be guarded by an #ifdef and then you could run multiple invocations on the same file. Something like:

// RUN: %clangxx_asan -DASAN_DEFAULT_OPTIONS %s -o %t
// RUN: %run %t 2>&1 | FileCheck --check-prefix=RETURN_NULL
// RUN: %clangxx_asan %s -o %t
// RUN: %env_asan_opts=allocator_may_return_null=1 %run %t 2>&1 | FileCheck --check-prefix=RETURN_NULL
// RUN: %env_asan_opts=allocator_may_return_null=0 not %run %t 2>&1 | FileCheck --check-prefix=ABORT

Copy link
Contributor Author

@davidmrdavid davidmrdavid Dec 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, good idea! I actually had a local branch where I'm tackling #117925 more generally, and I had already combined the tests there. So I'm porting that over, here: ebc3d0d

Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two more nits from my side.

// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The %env_asan_opts=allocator_may_return_null=1 part seems redundant here, and defeating the purpose of defining __asan_default_options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I had meant to set that to 0 and have the user function actually override that 0 to 1.
But might as well remove it. Done that here: 1f66929

@@ -0,0 +1,34 @@
// 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(nit: mixing dashes and underscores in the check name doesn't feel right style wise; I think CHECK-RETURN-NULL would be more common llvm test style)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I should have payed more attention (had not realized I was mixing - with _).
Addressed: 1f66929

Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@davidmrdavid
Copy link
Contributor Author

Thanks @zmodem for the thorough review. I'm unable to merge this myself, I don't have commit access (I'd love to get there eventually). Can you help me merge this or what is the procedure at this point?

@zmodem
Copy link
Collaborator

zmodem commented Dec 10, 2024

Let's see if @vitalybuka has any further comments. Then I can push the Merge button for you.

@zmodem zmodem merged commit 2dc2261 into llvm:main Dec 11, 2024
6 of 7 checks passed
Copy link

@davidmrdavid Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@davidmrdavid davidmrdavid deleted the dev/dajusto/honor-allocator-may-return-null-under-large-mallocs branch December 11, 2024 17:24
Comment on lines +180 to +182
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funnily enough, we're applying a patch to add ERROR_COMMITMENT_LIMIT downstream. It was submitted as https://reviews.llvm.org/D130781 but never ended up being merged.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks reasonable to me. Want to send it again as a PR? I think we could merge that without a test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. Please tag me on that PR if you end up opening it, @glandium. Can also assist in opening it if you don't find the cycles, I think the phabricator bug report suffices to me as well to just add it the check without a repro.

vitalybuka pushed a commit that referenced this pull request Jul 2, 2025
…he user function (#122990)

**Related to:** #117925 
**Follow up to:** #117929

**Context:**
As noted in the linked issue, some ASan configuration flags are not
honored on Windows when set through the `__asan_default_options` user
function. The reason for this is that `__asan_default_options` is not
available by the time `AsanInitInternal` executes, which is responsible
for applying the ASan flags.

To fix this properly, we'll probably need a deep re-design of ASan
initialization so that it is consistent across OS'es.
In the meantime, this PR offers a practical workaround.

**This PR:** refactors part of `AsanInitInternal` so that **idempotent**
flag-applying steps are extracted into a new function `ApplyOptions`.
This function is **also** invoked in the "weak function callback" on
Windows (which gets called when `__asan_default_options` is available)
so that, if any flags were set through the user-function, they are
safely applied _then_.

Today, `ApplyOptions` contains only a subset of flags. My hope is that
`ApplyOptions` will over time, through incremental refactorings
`AsanInitInternal` so that **all** flags are eventually honored.

Other minor changes:
* The introduction of a `ApplyAllocatorOptions` helper method, needed to
implement `ApplyOptions` for allocator options without re-initializing
the entire allocator. Reinitializing the entire allocator is expensive,
as it may do a whole pass over all the marked memory. To my knowledge,
this isn't needed for the options captured in `ApplyAllocatorOptions`.
* Rename `ProcessFlags` to `ValidateFlags`, which seems like a more
accurate name to what that function does, and prevents confusion when
compared to the new `ApplyOptions` function.
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Jul 2, 2025
…t through the user function (#122990)

**Related to:** llvm/llvm-project#117925
**Follow up to:** llvm/llvm-project#117929

**Context:**
As noted in the linked issue, some ASan configuration flags are not
honored on Windows when set through the `__asan_default_options` user
function. The reason for this is that `__asan_default_options` is not
available by the time `AsanInitInternal` executes, which is responsible
for applying the ASan flags.

To fix this properly, we'll probably need a deep re-design of ASan
initialization so that it is consistent across OS'es.
In the meantime, this PR offers a practical workaround.

**This PR:** refactors part of `AsanInitInternal` so that **idempotent**
flag-applying steps are extracted into a new function `ApplyOptions`.
This function is **also** invoked in the "weak function callback" on
Windows (which gets called when `__asan_default_options` is available)
so that, if any flags were set through the user-function, they are
safely applied _then_.

Today, `ApplyOptions` contains only a subset of flags. My hope is that
`ApplyOptions` will over time, through incremental refactorings
`AsanInitInternal` so that **all** flags are eventually honored.

Other minor changes:
* The introduction of a `ApplyAllocatorOptions` helper method, needed to
implement `ApplyOptions` for allocator options without re-initializing
the entire allocator. Reinitializing the entire allocator is expensive,
as it may do a whole pass over all the marked memory. To my knowledge,
this isn't needed for the options captured in `ApplyAllocatorOptions`.
* Rename `ProcessFlags` to `ValidateFlags`, which seems like a more
accurate name to what that function does, and prevents confusion when
compared to the new `ApplyOptions` function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants