[libc++abi] Use std::abort() instead of std::terminate() on failure to allocate

[ldionne]

Summary

Inside the Itanium demangler, we would previously call std::terminate()
after failing to (re)allocate. However, programs are free to install a
custom terminate_handler which does non-trivial things. In fact, by
default libc++abi itself installs a demangling_terminate_handler() which
tries to demangle the currently active exception before aborting the
program.

In case of an out-of-memory exception caused by the system truly having
no more memory (as opposed to attempting to allocate INT_MAX just once),
we will end up trying to demangle the exception, failing to do so because
we can't grow the OutputBuffer inside ItaniumDemangle.h, and then calling
std::terminate() there. That will call the demangling_terminate_handler(),
which will then start this loop again. We eventually end up crashing due
to a stack overflow.

To fix this problem, this patch calls std::abort() directly from the
demangler instead of going through std::terminate(). After all, calling
std::abort() is the default behavior for std::terminate() according
to the Standard, so this behavior is definitely valid. The downside of
this approach is that in case of a "true" out-of-memory condition:

1. the program will throw an exception
2. std::terminate() will be called if uncaught
3. demangling_terminate_handler() will be called and will fail
4. abort() will be called

We'll end up aborting the program without mentioning the cause, which
normally looks like:

terminating due to uncaught exception of type <TYPE>: <what()-MESSAGE>

Another option would be to properly handle failure-to-allocate inside
ItaniumDemangle.h and to propagate something like an error code or a
std::expected to the caller of all functions in the demangler that
can allocate. Then, we could make sure that cxa_demangle returns
nullptr when it fails to demangle the input due to any error, as it is
supposed to (but today "true" out-of-memory conditions are not handled
properly). The demangling_terminate_handler() would then see that
cxa_demangle failed to do its job and would still print the
appropriate message, simply using the non-demangled exception type.
However, this is akin to a partial rewrite of the demangler code since
a large number of functions would now have to return a std::expected
to account for out-of-memory conditions.

Using exceptions would be a lot simpler in terms of code changes and
would achieve the same result, however the demangler can't use exceptions
because it is used inside LLVM and libc++abi implements the exception
runtime anyway (so while it might be possible to use them in that
context, I'd argue we'd only be playing with fire).

rdar://110767664

[ldionne]

I have no idea how to reliably test this.

I'm also looking for feedback on whether folks think this is reasonable, or whether we should pursue this alternative explained in the commit message:

Another option would be to properly handle failure-to-allocate inside
ItaniumDemangle.h and to propagate something like an error code or a
std::expected to the caller of all functions in the demangler that
can allocate. Then, we could make sure that cxa_demangle returns
nullptr when it fails to demangle the input due to any error, as it is
supposed to (but today "true" out-of-memory conditions are not handled
properly). The demangling_terminate_handler() would then see that
cxa_demangle failed to do its job and would still print the
appropriate message, simply using the non-demangled exception type.
However, this is akin to a partial rewrite of the demangler code since
a large number of functions would now have to return a std::expected
to account for out-of-memory conditions.

[philnik]

Just aborting on being out of memory really doesn't seem super nice to me, especially when there is a proper way to tell the user. So IMO fixing the code to return nullptr on failure is the right thing to do.

[nickdesaulniers]

In D155598#4510499, @ldionne wrote:

I have no idea how to reliably test this.

I'm also looking for feedback on whether folks think this is reasonable, or whether we should pursue this alternative explained in the commit message:

Another option would be to properly handle failure-to-allocate inside
ItaniumDemangle.h and to propagate something like an error code or a
std::expected to the caller of all functions in the demangler that
can allocate. Then, we could make sure that cxa_demangle returns
nullptr when it fails to demangle the input due to any error, as it is
supposed to (but today "true" out-of-memory conditions are not handled
properly). The demangling_terminate_handler() would then see that
cxa_demangle failed to do its job and would still print the
appropriate message, simply using the non-demangled exception type.
However, this is akin to a partial rewrite of the demangler code since
a large number of functions would now have to return a std::expected
to account for out-of-memory conditions.

I'm all for updating the interface. +1 to @philnik 's comment.

libcxxabi/src/demangle/README.txt

37 ↗

(On Diff #541512)

this LGTM if you want to commit this change to libcxxabi/src/demangle/README.txt separately of this commit.

[MaskRay]

LGTM.

I have a reliable reproduce on Linux using ulimit -v to limit the virtual memory size.

printf 'int main() { for(;;) *(new char[995]) = 0; }' > a.cc
myclang++ -stdlib=libc++ --rtlib=compiler-rt --unwindlib=libunwind a.cc -Wl,-rpath,$(dirname $(myclang --print-file-name=libc++.so.1)) -g -o aaa

% (ulimit -v 10000; ./aaa)
[1]    992598 segmentation fault  ( ulimit -v 10000; ./aaa; )

We can pick sizes different from 995. Smaller sizes are more likely to fail.
I think D119177 made us more likely to get into a libc++ demangling_terminate_handler / libc++abi __cxa_demangle loop, eventually leading to a stach overflow.
But a small new char[995] will give us a stack overflow even if we revert D119177.

This patch will change the stack overflow cases (erroneous cases within our runtime libraries) to SIGABRT, which I think is an improvement.

Just aborting on being out of memory really doesn't seem super nice to me, especially when there is a proper way to tell the user. So IMO fixing the code to return nullptr on failure is

the right thing to do.

I do not worry about these cases. I think this patch would bring us to a much better state.

push_back is called 30+ times within src/demangle. They all need to be changed to propagate an error. That would be too much engineering for very little benefit.

Another alternative is to refactor OutputBuffer to use a stack buffer in the frame of __cxa_demangle. This seems challenging as well.

libcxxabi/src/demangle/README.txt

37 ↗

(On Diff #541512)

LGTM +1

[ldionne]

[back from OOO]

I agree with @MaskRay here in terms of the "bang for the buck". I think it would be much better to actually return nullptr from __cxa_demangle, however I don't know when I'd have time to get to it given the size of that change. I filed https://github.com/llvm/llvm-project/issues/64505 to track the improvement to return nullptr from __cxa_demangle, but I'd like to move forward with this change since it already makes things somewhat better.

Any objections @nickdesaulniers @philnik?

[ldionne]

Rebase

[philnik]

In D155598#4566939, @ldionne wrote:

[back from OOO]

I agree with @MaskRay here in terms of the "bang for the buck". I think it would be much better to actually return nullptr from __cxa_demangle, however I don't know when I'd have time to get to it given the size of that change. I filed https://github.com/llvm/llvm-project/issues/64505 to track the improvement to return nullptr from __cxa_demangle, but I'd like to move forward with this change since it already makes things somewhat better.

Any objections @nickdesaulniers @philnik?

This feels more like a temporary fix, but I guess it's fine.

[nickdesaulniers]

no objections

[ldionne]

Rebase

libcxxabi/src/demangle/ItaniumDemangle.h

void clearInline() {
     Cap = Inline + N;
   }
 
   void reserve(size_t NewCap) {
     size_t S = size();
     if (isInline()) {
       auto *Tmp = static_cast<T *>(std::malloc(NewCap * sizeof(T)));
       if (Tmp == nullptr)
-        std::terminate();
+        std::abort();
       std::copy(First, Last, Tmp);
       First = Tmp;
     } else {
       First = static_cast<T *>(std::realloc(First, NewCap * sizeof(T)));
       if (First == nullptr)
-        std::terminate();
+        std::abort();
     }
     Last = First + S;
     Cap = First + NewCap;
   }
 
 public:
   PODSmallVector() : First(Inline), Last(First), Cap(Inline + N) {}
 

libcxxabi/src/demangle/Utility.h

 
 #include "DemangleConfig.h"
 
 #include <array>
 #include <cassert>
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
-#include <exception>
 #include <limits>
 #include <string_view>
 
 DEMANGLE_NAMESPACE_BEGIN
 
 // Stream that AST nodes write their string representation into after the AST
 // has been parsed.
 class OutputBuffer {
if (Need > BufferCapacity) {
       // number here is chosen so the first allocation will more-than-likely not
       // allocate more than 1K.
       Need += 1024 - 32;
       BufferCapacity *= 2;
       if (BufferCapacity < Need)
         BufferCapacity = Need;
       Buffer = static_cast<char *>(std::realloc(Buffer, BufferCapacity));
       if (Buffer == nullptr)
-        std::terminate();
+        std::abort();
     }
   }
 
   OutputBuffer &writeUnsigned(uint64_t N, bool isNeg = false) {
     std::array<char, 21> Temp;
     char *TempPtr = Temp.data() + Temp.size();
 
     // Output at least one character.

llvm/include/llvm/Demangle/ItaniumDemangle.h

void clearInline() {
     Cap = Inline + N;
   }
 
   void reserve(size_t NewCap) {
     size_t S = size();
     if (isInline()) {
       auto *Tmp = static_cast<T *>(std::malloc(NewCap * sizeof(T)));
       if (Tmp == nullptr)
-        std::terminate();
+        std::abort();
       std::copy(First, Last, Tmp);
       First = Tmp;
     } else {
       First = static_cast<T *>(std::realloc(First, NewCap * sizeof(T)));
       if (First == nullptr)
-        std::terminate();
+        std::abort();
     }
     Last = First + S;
     Cap = First + NewCap;
   }
 
 public:
   PODSmallVector() : First(Inline), Last(First), Cap(Inline + N) {}
 

llvm/include/llvm/Demangle/Utility.h

 
 #include "DemangleConfig.h"
 
 #include <array>
 #include <cassert>
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
-#include <exception>
 #include <limits>
 #include <string_view>
 
 DEMANGLE_NAMESPACE_BEGIN
 
 // Stream that AST nodes write their string representation into after the AST
 // has been parsed.
 class OutputBuffer {
if (Need > BufferCapacity) {
       // number here is chosen so the first allocation will more-than-likely not
       // allocate more than 1K.
       Need += 1024 - 32;
       BufferCapacity *= 2;
       if (BufferCapacity < Need)
         BufferCapacity = Need;
       Buffer = static_cast<char *>(std::realloc(Buffer, BufferCapacity));
       if (Buffer == nullptr)
-        std::terminate();
+        std::abort();
     }
   }
 
   OutputBuffer &writeUnsigned(uint64_t N, bool isNeg = false) {
     std::array<char, 21> Temp;
     char *TempPtr = Temp.data() + Temp.size();
 
     // Output at least one character.