-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[sanitizer_common] Return nullptr from ASan on ERROR_COMMITMENT_LIMIT #119753
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
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Mike Hommey (glandium) ChangesFull diff: https://github.com/llvm/llvm-project/pull/119753.diff 1 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
index 1eef16fbde3e7d..fd0f989ee392bb 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_win.cpp
@@ -167,7 +167,7 @@ static void *ReturnNullptrOnOOMOrDie(uptr size, const char *mem_type,
// 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
+ // VirtualAlloc emits one of 3 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:
@@ -176,12 +176,12 @@ static void *ReturnNullptrOnOOMOrDie(uptr size, const char *mem_type,
// (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.
+ // 3. ERROR_COMMITMENT_LIMIT:
+ // VirtualAlloc will return this if e.g. the pagefile is too small to commit
+ // the requested amount of memory.
if (last_error == ERROR_NOT_ENOUGH_MEMORY ||
- last_error == ERROR_INVALID_PARAMETER)
+ last_error == ERROR_INVALID_PARAMETER ||
+ last_error == ERROR_COMMITMENT_LIMIT)
return nullptr;
ReportMmapFailureAndDie(size, mem_type, mmap_type, last_error);
}
|
Cc: @davidmrdavid @zmodem |
Since the PR description is empty, just calling out that this is related to: #117929 (comment) |
if (last_error == ERROR_NOT_ENOUGH_MEMORY || | ||
last_error == ERROR_INVALID_PARAMETER) | ||
last_error == ERROR_INVALID_PARAMETER || | ||
last_error == ERROR_COMMITMENT_LIMIT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be nice to extend a test if it's reasonably easy to reproduce
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found a trivial way to trigger it, unfortunately. At least not in the time I had to try.
Could you merge this for me? |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/10242 Here is the relevant piece of the build log for the reference
|
Interesting. @vitalybuka - is that a failure above ^ running on Windows ( I can't see to find any evidence of that)? Otherwise, I can't see why the PR would cause it, since the PR is windows specific. |
It's unrelated. That bot is not running Windows, and the test was not using any sanitizer. |
Followup to #117929