[libc++abi] Use fallback_malloc to allocate __cxa_eh_globals in case of dynamic memory exhaustion.

[ikudrin]

Summary

Throwing an exception for the first time may lead to calling calloc to allocate memory for __cxa_eh_globals. If the memory pool is exhausted at that moment, it results in abnormal termination of the program.

This patch addresses the issue by using fallback_malloc in that case.

[ikudrin]

Ping.

[ikudrin]

Ping?..

[bcraig]

LGTM. I don't know if that's good enough for a submit though :)

In my opinion, requiring heap allocation during an exception is one of the worst parts of the Itanium ABI. At least this particular path isn't triggered for platforms with true thread_local support.

[EricWF]

By including fallback_malloc.ipp in a new C++ file we now have 2 different OOM emergancy buffers. I think it would be preferable to use only one. This would require a fairly large restructuring of fallback_malloc.ipp though.

[ikudrin]

Thanks! I'll see what I can do about it.

[ikudrin]

Make cxa_exception.cpp and cxa_exception_storage.cpp to use the same emergency memory pool.

[ikudrin]

Ping...

[ikudrin]

Ping...

[ikudrin]

Ping! Could anyone check this patch, please?

[EricWF]

Could you generate a diff against the normal directory layout? Your's is against "libcxxabi/trunk/" where 'trunk' is the actual libcxxabi directory.

[ikudrin]

Regenerated the patch against project's root.

[EricWF]

It's almost there.

Please move "fallback_malloc.ipp" into "fallback_malloc.cpp" and then delete it all together. We can't have other files trying to include it.

src/fallback_malloc.cpp

23 ↗

(On Diff #54151)

I think we should have the pragma GCC visibility push(hidden) in this file as well, but I'm not 100% sure.

test/test_exception_storage_nodynmem.pass.cpp

13 ↗

(On Diff #54151)

Let's check that we actually replace calloc here and assert that our replacement has been called at the end of main.

16 ↗

(On Diff #54151)

Let's perform this test a couple of times so we're testing

• The memory get's returned properly
• The returned memory can be reused.

[ikudrin]

• Moved the content of fallback_malloc.ipp into falback_malloc.cpp.
• Removed fallback_malloc.ipp.
• Added pragma GCC visibility push(hidden) for the function's definitions.
• Added a check that the overwritten calloc is called to the test.
• Added a loop to run the test several times.

Please note that test_fallback_malloc.pass.cpp now has to #include fallback_malloc.cpp, because fallback_malloc.ipp was removed. I'd considered some other options, including declaring additional functions especially for testing purpose and even moving all fallback_malloc functionality into a separate class which could be tested independently. However, eventually, I found them more difficult and even much worse than the presented approach.

[ikudrin]

Ping...

[ikudrin]

Ping?..

[ikudrin]

Ping.

[ikudrin]

Ping...

This patch is under review for more than two months, and I believe I've addressed all the comments. Could someone take a look at it, please?

[ikudrin]

Hi,

Ping.

The issue in the current implementation of the libc++abi library looks like a time bomb. Even though its probability could be considered as very low, in fact, it depends on the type of an application. At least, we ran into it in our environment.

I'd guess that no one would expect their program to terminate when a legal exception is thrown, or when memory is allocated. I'd like this issue to be fixed in one or another way.

@EricWF, can you look at the patch again, please?

[ikudrin]

Ping...

[ikudrin]

Hi,

Ping.

The patch is under review for a long time and the described problem is still here. As we've seen the issue in practice, I'm sure that others may also run into it, and I do believe it should be fixed, one way or another.

@EricWF, can I improve the patch somehow to pass the review? Or maybe you prefer it to be fixed in some other way?

[EricWF]

LGTM after addressing the inline comments.

src/fallback_malloc.cpp

58 ↗

(On Diff #54189)

A static const variable would be better here.

59 ↗

(On Diff #54189)

This should have __attribute__((aligned)) on it.

src/fallback_malloc.h

20 ↗

(On Diff #54189)

Please don't use the __cxa prefix on these functions. It makes it seem like they are part of the Itanium spec but they're really internal details.

[rmaprath]

@ikudrin: Looks like you've reverted this soon after. I'm just about to commit D24864, which will affect this slightly. D24864 basically refactors all pthread dependencies behind a separate API. It would be pretty straightforward for you to update this patch though, just replacing pthread calls with ones in thread_support.h (perhaps adding anything missing).

Hope you don't mind me going first? If you are going to commit this soon, I can hold off D24864. Let me know.

Cheers,

/ Asiri

[ikudrin]

@rmaprath: There are some issues in the test which upset build bots and I need some time to fix them. You don't need to wait for me, I'll put my patch on the top of yours when my fix is ready.

libcxxabi/trunk/src/cxa_exception_storage.cpp

extern "C" {
     __cxa_eh_globals * __cxa_get_globals_fast () { return __globals (); }
     }
 }
 
 #else
 
 #include <pthread.h>
 #include <cstdlib>          // for calloc, free
+#include <cstring>          // for memset
 #include "abort_message.h"
 
 //  In general, we treat all pthread errors as fatal.
 //  We cannot call std::terminate() because that will in turn
 //  call __cxa_get_globals() and cause infinite recursion.
 
 namespace __cxxabiv1 {
+
+#include "fallback_malloc.ipp"
+
+//  Allocate some memory from _somewhere_
+static void *do_calloc(size_t count, size_t size) {
+    void *ptr = std::calloc(count, size);
+    if (NULL == ptr) { // if calloc fails, fall back to emergency stash
+        ptr = fallback_malloc(size * count);
+        if (NULL != ptr)
+            std::memset(ptr, 0, size * count);
+    }
+    return ptr;
+}
+
+static void do_free(void *ptr) {
+    is_fallback_ptr(ptr) ? fallback_free(ptr) : std::free(ptr);
+}
+
 namespace {
     pthread_key_t  key_;
     pthread_once_t flag_ = PTHREAD_ONCE_INIT;
 
     void destruct_ (void *p) {
-        std::free ( p );
+        do_free ( p );
         if ( 0 != ::pthread_setspecific ( key_, NULL ) ) 
             abort_message("cannot zero out thread value for __cxa_get_globals()");
         }
 
     void construct_ () {
         if ( 0 != pthread_key_create ( &key_, destruct_ ) )
             abort_message("cannot create pthread key for __cxa_get_globals()");
         }
 }   
 
 extern "C" {
     __cxa_eh_globals * __cxa_get_globals () {
     //  Try to get the globals for this thread
         __cxa_eh_globals* retVal = __cxa_get_globals_fast ();
     
     //  If this is the first time we've been asked for these globals, create them
         if ( NULL == retVal ) {
             retVal = static_cast<__cxa_eh_globals*>
-                        (std::calloc (1, sizeof (__cxa_eh_globals)));
+                        (do_calloc (1, sizeof (__cxa_eh_globals)));
             if ( NULL == retVal )
                 abort_message("cannot allocate __cxa_eh_globals");
             if ( 0 != pthread_setspecific ( key_, retVal ) )
                abort_message("pthread_setspecific failure in __cxa_get_globals()");
            }
         return retVal;
         }
 

libcxxabi/trunk/test/test_exception_storage_nodynmem.pass.cpp

+//===--------------- test_exception_storage_nodynmem.cpp ------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include <cstdlib>
+
+// Override calloc to simulate exhaustion of dynamic memory
+void *calloc(size_t, size_t) { return 0; }
+
+int main(int argc, char *argv[]) {
+  try {
+    throw 42;
+  } catch (...) {
+  }
+  return 0;
+}