[libcxxabi] Manually align pointers in __cxa_allocate_exception - Fixes PR24604

[EricWF]

Summary

In 32 bit builds the __cxa_exception class has an alignment requirement greater than the maximal alignment supported by malloc. If this is the case we need to manually align the pointers returned from malloc in ordered to prevent undefined behavior. This patch does exactly that.

See PR24604 for more information - https://llvm.org/bugs/show_bug.cgi?id=24604

[majnemer]

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

[EricWF]

In D12512#236984, @majnemer wrote:

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

[majnemer]

In D12512#236987, @EricWF wrote:

In D12512#236984, @majnemer wrote:

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Say you have two binaries, foo.exe and bar.so. Foo.exe statically links against an older libc++abi and bar.so links against a newer libc++abi. In this instance, our program has two copies of libc++abi statically linked with no ill effects and such a scenario was supported before this patch (at least AFAICT).

However, we might have problems after this patch if foo.exe is linked against a newer static library than bar.so

[EricWF]

In D12512#236988, @majnemer wrote:

In D12512#236987, @EricWF wrote:

In D12512#236984, @majnemer wrote:

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Say you have two binaries, foo.exe and bar.so. Foo.exe statically links against an older libc++abi and bar.so links against a newer libc++abi. In this instance, our program has two copies of libc++abi statically linked with no ill effects and such a scenario was supported before this patch (at least AFAICT).

However, we might have problems after this patch if foo.exe is linked against a newer static library than bar.so

If foo.exe is already getting __cxa_allocate_exception(...) from one libc++abi version and __cxa_free_exception from another then they are already in trouble because __cxa_free_exception(ptr) checks to see if ptr has been allocated from a builtin buffer. This only happens when malloc is out of memory but it would still be an existing problem in the scenario you describe.

Furthermore the new code is only executed in cases where we already have undefined behavior. We don't add any extra padding to the pointers returned from malloc unless not doing so would lead to UB.

I know if multiple instances of libc++abi in one executable is not supported on OS X (see FAQ at the bottom of libcxxabi.llvm.org). I don't know if thats supported on linux though. People really shouldn't be using a static libc++abi. In order for the exception handling globals to work there should only be one copy of the in the entire program.

Hopefully this change isn't too problematic and thanks for looking at it.

[compnerd]

While I can see the argument you raise, the saving grace here is that this is buried in libc++abi. The only real piece of libc++abi of real use to users is __cxa_demangle (which is only relevant for Windows, and Darwin where you have two level namespaces). For everyone else, libc++abi is merely an implementation detail for libc++ and will not be linked to directly. If you are mixing and matching the ABI support interfaces, or the runtime, you are already in a pretty bad shape, and allowing things to break in such a circumstances doesn't seem entirely terrible. Unfortunately, for compatibility with GCC, we do need to maintain this alignment, which is not guaranteed without this patch.

[joerg]

Please don't commit this as is. Many platforms have posix_memalign or equivalent, which makes this both simpler and potentially without wasting memory. Compare e.g. D12001.

[jroelofs]

In D12512#236988, @majnemer wrote:

In D12512#236987, @EricWF wrote:

In D12512#236984, @majnemer wrote:

Wouldn't this change be problematic if you threw to code which was statically linked with a prior version of libcxxabi?

How do you mean? As in you have two different versions of libc++abi linked into one executable? If so your already in bad shape.

Say you have two binaries, foo.exe and bar.so. Foo.exe statically links against an older libc++abi and bar.so links against a newer libc++abi. In this instance, our program has two copies of libc++abi statically linked with no ill effects and such a scenario was supported before this patch (at least AFAICT).

However, we might have problems after this patch if foo.exe is linked against a newer static library than bar.so

I don't think that is/was supported. The abi library holds some global state, which wouldn't be shared between the two instances of it.

[EricWF]

In D12512#237175, @joerg wrote:

Please don't commit this as is. Many platforms have posix_memalign or equivalent, which makes this both simpler and potentially without wasting memory. Compare e.g. D12001.

Will do. Any advice on detecting posix_memalign? I don't see anything in the patch you pointed me to.

PS. Thanks for actually rejecting the patch. It makes the phab workflow easier to use.

[danalbert]

Android has posix_memalign too.

[EricWF]

Address @joerg's comment and use posix_memalign when available. This patch does not fix the case where posix_memalign is unavailable but that should be rare and can come in another patch.

Thi patch also defers all changes to fallback_malloc to D12669.

[EricWF]

Whats preventing this from landing? @joerg @danalbert Do we want to use "posix_memalign" on Android or not?

[emaste]

Is this going to be committed?
Libunwind is about to grow the same alignment attribute (D22543)

[danalbert]

The configure-time check LGTM. Android has it for all recent versions (since JB), but that will cover the case of GB and ICS.

src/cxa_exception.cpp

127

#warning until that's done?

[emaste]

As it happens on FreeBSD/i386 malloc returns a 16-byte-aligned pointer for allocations with size >= 16 so will not be affected by this issue.

[emaste]

test/test_cxa_allocate_exception.pass.cpp

34

requires_over_alignment not used?

[emaste]

Note that the addition of the referenceCount at the beginning of __cxa_exception for 64-bit builds causes the _Unwind_Exception unwindHeader to become misaligned. libstdc++ had this same issue (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38732, now fixed) and libcxxrt does as well (https://github.com/pathscale/libcxxrt/issues/38).

CMakeLists.txt

 if (NOT LIBCXXABI_ENABLE_SHARED)
   list(APPEND LIBCXXABI_COMPILE_FLAGS -D_LIBCPP_BUILD_STATIC)
 endif()
 
 if (NOT LIBCXXABI_ENABLE_THREADS)
   add_definitions(-DLIBCXXABI_HAS_NO_THREADS=1)
 endif()
 
+if (NOT LIBCXXABI_HAS_POSIX_MEMALIGN)
+  add_definitions(-DLIBCXXABI_HAS_NO_POSIX_MEMALIGN=1)
+endif()
+
 # This is the _ONLY_ place where add_definitions is called.
 if (MSVC)
   add_definitions(-D_CRT_SECURE_NO_WARNINGS)
 endif()
 
 # Define LIBCXXABI_USE_LLVM_UNWINDER for conditional compilation.
 if (LIBCXXABI_USE_LLVM_UNWINDER OR LLVM_NATIVE_ARCH MATCHES ARM)
   add_definitions(-DLIBCXXABI_USE_LLVM_UNWINDER=1)

cmake/config-ix.cmake

 include(CheckLibraryExists)
 include(CheckCCompilerFlag)
 include(CheckCXXCompilerFlag)
+include(CheckSymbolExists)
 
 # Check compiler flags
 check_c_compiler_flag(-funwind-tables         LIBCXXABI_HAS_FUNWIND_TABLES)
 check_cxx_compiler_flag(-fPIC                 LIBCXXABI_HAS_FPIC_FLAG)
 check_cxx_compiler_flag(-fno-exceptions       LIBCXXABI_HAS_NO_EXCEPTIONS_FLAG)
 check_cxx_compiler_flag(-fno-rtti             LIBCXXABI_HAS_NO_RTTI_FLAG)
 check_cxx_compiler_flag(-fstrict-aliasing     LIBCXXABI_HAS_FSTRICT_ALIASING_FLAG)
 check_cxx_compiler_flag(-nodefaultlibs        LIBCXXABI_HAS_NODEFAULTLIBS_FLAG)

 
 # Check libraries
 check_library_exists(c printf "" LIBCXXABI_HAS_C_LIB)
 check_library_exists(dl dladdr "" LIBCXXABI_HAS_DL_LIB)
 check_library_exists(pthread pthread_once "" LIBCXXABI_HAS_PTHREAD_LIB)
 check_library_exists(gcc_eh _Unwind_GetRegionStart "" LIBCXXABI_HAS_GCC_EH_LIB)
 check_library_exists(c __cxa_thread_atexit_impl ""
   LIBCXXABI_HAS_CXA_THREAD_ATEXIT_IMPL)
+
+# Check symbols
+check_symbol_exists(posix_memalign stdlib.h LIBCXXABI_HAS_POSIX_MEMALIGN)

src/cxa_exception.cpp

static_assert(alignof(FallbackMaxAlignType) == alignof(__cxa_exception),
     "__cxa_exception");
 
 static_assert(alignof(FallbackMaxAlignType) >= alignof(std::max_align_t),
     "FallbackMaxAlignType must have an alignment requirement greater or equal "
     "to the alignment of std::max_align_t");
 
 //  Allocate some memory from _somewhere_
 static void *do_malloc(size_t size) {
-    void *ptr = std::malloc(size);
+    void* ptr = nullptr;
+#ifndef LIBCXXABI_HAS_NO_POSIX_MEMALIGN
+    (void)posix_memalign(&ptr, alignof(__cxa_exception), size);
+#else
+    // FIXME: The output of malloc needs to be manually aligned to a 16 byte
+    // on 32 bit targets.
+    ptr = std::malloc(size);
+#endif
     if (NULL == ptr) // if malloc fails, fall back to emergency stash
         ptr = fallback_malloc(size);
     return ptr;
 }
 
 static void do_free(void *ptr) {
+    // NOTE: std::free can free pointers allocated with both std::malloc
+    // and posix_memalign.
     is_fallback_ptr(ptr) ? fallback_free(ptr) : std::free(ptr);
 }
 
 /*
     If reason isn't _URC_FOREIGN_EXCEPTION_CAUGHT, then the terminateHandler
     stored in exc is called.  Otherwise the exceptionDestructor stored in 
     exc is called, and then the memory for the exception is deallocated.
 

test/test_cxa_allocate_exception.pass.cpp

+//===--------------------- test_fallback_malloc.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.
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: c++98, c++03
+
+// void* __cxa_allocate_exception(size_t);
+// void  __cxa_free_exception(void*);
+// void* __cxa_allocate_dependent_exception();
+// void  __cxa_free_dependent_exception();
+
+// __cxa_exception is specified with __attribute__((aligned)) for GNU unwind.
+// This means that during 32 bit builds __cxa_exception is "over aligned". This
+// test checks that __cxa_allocate_exception return correctly overaligned memory.
+// See PR24604 - https://llvm.org/bugs/show_bug.cgi?id=24604
+
+#include "../src/cxa_exception.hpp"
+#include <cstddef> // for std::max_align_t
+#include <cassert>
+
+#if defined(__GNUC__) && !defined(_LP64)
+#define SHOULD_BE_OVERALIGNED 1
+#endif
+
+using namespace __cxxabiv1;
+
+const std::size_t max_alignment = alignof(std::max_align_t);
+const std::size_t required_alignment = alignof(__cxa_exception);
+const bool requires_over_alignment = max_alignment < required_alignment;
+
+static_assert(alignof(__cxa_exception) == alignof(__cxa_dependent_exception),
+              "They should have the same alignment requirements");
+
+#if defined(__GNUC__) && !defined(_LP64)
+static_assert(alignof(__cxa_exception) > alignof(std::max_align_t),
+              "On 32 bit platforms __cxa_exception is expected to be over aligned.");
+#endif
+
+void test_cxa_allocate_exception() {
+    for (int i=0; i < 4096; ++i) {
+        void* ptr = __cxa_allocate_exception(i);
+        assert(ptr);
+        assert(reinterpret_cast<std::size_t>(ptr) % required_alignment == 0);
+        __cxa_free_exception(ptr);
+    }
+}
+
+void test_cxa_allocate_dependent_exception() {
+    for (int i=0; i < 100; ++i) {
+        void* ptr = __cxa_allocate_dependent_exception();
+        assert(ptr);
+        assert(reinterpret_cast<std::size_t>(ptr) % required_alignment == 0);
+        __cxa_free_dependent_exception(ptr);
+    }
+}
+
+int main() {
+    test_cxa_allocate_exception();
+    test_cxa_allocate_dependent_exception();
+}