Skip to content
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

[Support] Report OOM from allocate_buffer #85449

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Mar 15, 2024

Previously, it called ::operator new which may throw std::bad_alloc,
regardless of whether LLVM itself was built with exception handling, and
this can cause safety issues if outside code has destructors that will
call back into LLVM. Now we use ::operator new(..., nothrow) and call
llvm::report_bad_alloc_error when allocation fails, which will abort
when LLVM is built without exceptions.

Ref: #85281

@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2024

@llvm/pr-subscribers-llvm-support

Author: Josh Stone (cuviper)

Changes

Previously, it called ::operator new which may throw std::bad_alloc,
regardless of whether LLVM itself was built with exception handling, and
this can cause safety issues if outside code has destructors that will
call back into LLVM. Now we use ::operator new(..., nothrow) and call
llvm::report_bad_alloc_error when allocation fails, which will abort
when LLVM is built without exceptions.

Ref: #85281


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

1 Files Affected:

  • (modified) llvm/lib/Support/MemAlloc.cpp (+7-4)
diff --git a/llvm/lib/Support/MemAlloc.cpp b/llvm/lib/Support/MemAlloc.cpp
index 07a26cf26480b3..6adc9abd75c5b9 100644
--- a/llvm/lib/Support/MemAlloc.cpp
+++ b/llvm/lib/Support/MemAlloc.cpp
@@ -13,12 +13,15 @@
 
 LLVM_ATTRIBUTE_RETURNS_NONNULL LLVM_ATTRIBUTE_RETURNS_NOALIAS void *
 llvm::allocate_buffer(size_t Size, size_t Alignment) {
-  return ::operator new(Size
+  void *Result = ::operator new(Size,
 #ifdef __cpp_aligned_new
-                        ,
-                        std::align_val_t(Alignment)
+                                std::align_val_t(Alignment),
 #endif
-  );
+                                std::nothrow);
+  if (Result == nullptr) {
+    report_bad_alloc_error("Buffer allocation failed");
+  }
+  return Result;
 }
 
 void llvm::deallocate_buffer(void *Ptr, size_t Size, size_t Alignment) {

Previously, it called `::operator new` which may throw `std::bad_alloc`,
regardless of whether LLVM itself was built with exception handling, and
this can cause safety issues if outside code has destructors that will
call back into LLVM. Now we use `::operator new(..., nothrow)` and call
`llvm::report_bad_alloc_error` when allocation fails, which will abort
when LLVM is built without exceptions.

Ref: llvm#85281
@cuviper cuviper force-pushed the allocate_buffer-OOM branch from fd9ddc5 to 24f1938 Compare January 24, 2025 21:59
@cuviper cuviper requested review from aeubanks and aganea January 24, 2025 22:01
Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

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

seems good, but could you run this through llvm-compile-time-tracker?

@cuviper
Copy link
Member Author

cuviper commented Jan 24, 2025

Ok, I'll look into that.

@cuviper
Copy link
Member Author

cuviper commented Jan 26, 2025

Thanks @nikic!

If we're good to go, please do merge it for me, as I don't have write access.

@aeubanks aeubanks merged commit b80965e into llvm:main Jan 27, 2025
8 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Jan 27, 2025

LLVM Buildbot has detected a new failure on builder llvm-clang-aarch64-darwin running on doug-worker-5 while building llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/13554

Here is the relevant piece of the build log for the reference
Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'Clang :: Analysis/live-stmts.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/clang -cc1 -internal-isystem /Users/buildbot/buildbot-root/aarch64-darwin/build/lib/clang/20/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -w -analyzer-checker=debug.DumpLiveExprs /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/test/Analysis/live-stmts.cpp 2>&1   | /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/test/Analysis/live-stmts.cpp
+ /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/clang -cc1 -internal-isystem /Users/buildbot/buildbot-root/aarch64-darwin/build/lib/clang/20/include -nostdsysteminc -analyze -analyzer-constraints=range -setup-static-analyzer -w -analyzer-checker=debug.DumpLiveExprs /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/test/Analysis/live-stmts.cpp
+ /Users/buildbot/buildbot-root/aarch64-darwin/build/bin/FileCheck /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/test/Analysis/live-stmts.cpp
�[1m/Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/test/Analysis/live-stmts.cpp:239:16: �[0m�[0;1;31merror: �[0m�[1mCHECK-EMPTY: is not on the line after the previous match
�[0m// CHECK-EMPTY:
�[0;1;32m               ^
�[0m�[1m<stdin>:180:1: �[0m�[0;1;30mnote: �[0m�[1m'next' match was here
�[0m
�[0;1;32m^
�[0m�[1m<stdin>:177:1: �[0m�[0;1;30mnote: �[0m�[1mprevious match ended here
�[0m
�[0;1;32m^
�[0m�[1m<stdin>:178:1: �[0m�[0;1;30mnote: �[0m�[1mnon-matching line after previous match is here
�[0mImplicitCastExpr 0x138134f78 '_Bool' <LValueToRValue>
�[0;1;32m^
�[0m
Input file: <stdin>
Check file: /Users/buildbot/buildbot-root/aarch64-darwin/llvm-project/clang/test/Analysis/live-stmts.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
�[1m�[0m�[0;1;30m           1: �[0m�[1m�[0;1;46m �[0m
�[0;1;30m           2: �[0m�[1m�[0;1;46m�[0m[ B0 (live expressions at block exit) ]�[0;1;46m �[0m
�[0;1;32mcheck:21      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;30m           3: �[0m�[1m�[0;1;46m�[0m �[0m
�[0;1;32mempty:22      ^
�[0m�[0;1;30m           4: �[0m�[1m�[0;1;46m�[0m �[0m
�[0;1;32mempty:23      ^
�[0m�[0;1;30m           5: �[0m�[1m�[0;1;46m�[0m[ B1 (live expressions at block exit) ]�[0;1;46m �[0m
�[0;1;32mcheck:24      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;30m           6: �[0m�[1m�[0;1;46m�[0m �[0m
�[0;1;32mempty:25      ^
�[0m�[0;1;30m           7: �[0m�[1m�[0;1;46m�[0m �[0m
�[0;1;32mempty:26      ^
�[0m�[0;1;30m           8: �[0m�[1m�[0;1;46m�[0m[ B2 (live expressions at block exit) ]�[0;1;46m �[0m
�[0;1;32mcheck:27      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;30m           9: �[0m�[1m�[0;1;46m�[0m �[0m
�[0;1;32mempty:28      ^
�[0m�[0;1;30m          10: �[0m�[1m�[0;1;46m�[0mDeclRefExpr 0x1381322e0 'int' lvalue ParmVar 0x138115a70 'y' 'int'�[0;1;46m �[0m
�[0;1;32mnext:29       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
�[0m�[0;1;30m          11: �[0m�[1m�[0;1;46m�[0m �[0m
�[0;1;32mempty:30      ^
�[0m�[0;1;30m          12: �[0m�[1m�[0;1;46m�[0mDeclRefExpr 0x138132300 'int' lvalue ParmVar 0x138115af0 'z' 'int'�[0;1;46m �[0m
...

@cuviper
Copy link
Member Author

cuviper commented Jan 27, 2025

It doesn't make sense to me that this failure would have anything to do with my change.

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.

None yet

5 participants