CXX-3222 add UBSan coverage with extra alignment and fix extended alignment issues

[eramongodb]

Summary

Resolves CXX-3222. Extends UBSan task coverage to include extra alignment configurations and addresses UBSan runtime errors due to aligned allocation requriement violations when the C Driver is configured to use extra alignment.

Important

The solution proposed in this PR is a temporary measure that is only required until CDRIVER-2813 is resolved.

Motivation

Attempting to extend UBSan task coverage to extra alignment configurations as part of #1328 revealed runtime alignment errors:

runtime error: constructor call on misaligned address [...] for type 'bsoncxx::v_noabi::builder::core::impl', which requires 128 byte alignment
#0 0x667560 in [...] bsoncxx::detail::make_unique_impl[...] src/bsoncxx/lib/bsoncxx/v_noabi/bsoncxx/private/make_unique.hh:52:35
     #1 0x664c83 in [...] bsoncxx::make_unique[...] src/bsoncxx/lib/bsoncxx/v_noabi/bsoncxx/private/make_unique.hh:121:12
     #2 0x661455 in bsoncxx::v_noabi::builder::core::core(bool) src/bsoncxx/lib/bsoncxx/v_noabi/bsoncxx/builder/core.cpp:254:13
     #3 0x442e01 in [...] bsoncxx::v_noabi::builder::basic::make_document[...] src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/builder/basic/document.hpp:116:14
     #4 0x4423f5 in [...] bsoncxx::v_noabi::types::bson_value::make_value[...] src/bsoncxx/include/bsoncxx/v_noabi/bsoncxx/types/bson_value/make_value.hpp:34:16
     

The root cause of this problem is that operator new does not support extended alignment until C++17 via std::align_val_t.

static constexpr std::size_t alignment = 128; // e.g. bson_t

struct X {} __attribute__((aligned(alignment)));

int main() { delete new X(); } // <-- runtime error

runtime error: constructor call on misaligned address [...] for type 'X', which requires 128 byte alignment

Support for extended alignment is implementation-defined prior to C++17. Many implementations do not support it. operator new often only supports alignment up to alignof(std::max_align_t).

Furthermore, this problem is viral: any parent class which contains a data member with extended alignment requirements inherits those extended alignment requirements:

// Inherits extended alignment requirements.
struct P { X x; }; // e.g. builder::core::impl.

static_assert(alignof(P) == alignof(X), "P inherited extended alignment requirements of X");

int main() { delete new P(); } // <-- runtime error

runtime error: constructor call on misaligned address [...] for type 'P', which requires 128 byte alignment

Neither alignas nor std::aligned_storage are viable workarounds, as they only apply alignment requirements the object or data member itself, which does not improve the status quo, as the parent object may still be allocated via operator new, in which case the allocation function remains oblivious to the extended alignment requirements:

struct P {
    alignas(X) unsigned char storage[sizeof(X)]; // or `std::aligned_storage<sizeof(T), alignof(T)>::type storage;
    X* ptr;

    ~P() { ptr->~X(); }
    P() : ptr(new &(storage) X()) {}
};

static_assert(alignof(P) == alignof(X), "P inherited extended alignment requirements of X");

// `operator new` remains oblivious to alignment requirements.
int main() { delete new P(); } // <-- runtime error

runtime error: constructor call on misaligned address [...] for type 'P', which requires 128 byte alignment

Proposed Solution

To address this problem without leaking custom allocation details into the ABI (that is, preserving the current type of std::unique_ptr<impl> data members), a new bsoncxx::aligned_storage<T> helper is introduced to avoid leaking extended alignment requirements to parent objects. By reserving x2 the size of T and using std::align to obtain an aligned pointer into the reserved storage region, correct alignment is guaranteed, but at the cost of x2 an additional alignof(T) bytes more than is normally required for the allocated object.

struct P {
    // Reserves memory that is x2 the size of `X` such that regardless of the alignment of P,
    // there is enough space to obtain an aligned pointer within the reserved storage region.
    bsoncxx::aligned_storage<sizeof(X), alignof(X)> storage;
    X* ptr;

    ~P() { ptr->~X(); }

    // `.get()` returns a properly-aligned pointer into the reserved storage region.
    P() : ptr(new (storage.get()) X()) {}
};

static_assert(alignof(P) <= alignof(std::max_align_t), "P does NOT inherit extended alignment requirements of X");
static_assert(alignof(P) < alignof(X), "P does NOT inherit extended alignment requirements of X");

int main() { delete new P(); } // OK!

Important

This workaround reserves x2 an additional alignof(T) bytes more than is normally required for the allocated object.

Note

This is only required for objects or data members which may be dynamically allocated via operator new. Stack-allocated objects do not require this workaround (i.e. all current usage of scoped_bson_t). AFAICT this is currently limited to the managed_bson_t, builder::core::impl::frame, and client_session::impl classes.

---

Why aligned_storage<sizeof(T), alignof(T)> instead of aligned_storage<T>?

Attempting to use alignof(T) within aligned_storage<T> does not reliably produce desired behavior, as observed with Clang 12.0.1 on RHEL 8.0:

template <typename T>
struct sanity_check {
    static_assert(alignof(T) == alignof(bson_t), "T and bson_t must have the same alignment");
};

sanity_check<bson_t> check; // error

 error: static_assert failed due to requirement 'alignof(_bson_t) == alignof(_bson_t)' "T and bson_t must have the same alignment"
  static_assert(alignof(T) == alignof(bson_t), "T and bson_t must have the same alignment");
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in instantiation of template class 'sanity_check<_bson_t>' requested here
  sanity_check<bson_t> check;
                       ^

Inspection reveals alignof(T) == 4 despite T = bson_t and alignof(bson_t) == 128. It is unclear if this is a compiler bug. I am unable to reproduce this behavior locally or with Compiler Explorer. I suspect the cause is that template parameters may not inherit attributes on some compilers or compiler versions, which may includes the aligned attribute, as indicated by GCC 13.3:

template <typename T>
struct aligned_storage { ... };

aligned_storage<bson_t> storage; // warning

warning: ignoring attributes on template argument ‘bson_t’ {aka ‘_bson_t’} [-Wignored-attributes]

Therefore, aligned_storage is defined in terms of size and alignment only, not on T.

Why the new specializations and overloads for bson_t*?

Because attributes do not appear to be inherited by template parameters, types with extended alignment such as bson_t need to be "explicit" for the compiler to see and deduce extended alignment requirements to resulting pointers. For example:

bsoncxx::v_noabi::document::view uri::options() const {
    auto opts_bson = libmongoc::uri_get_options(_impl->uri_t);
    return bsoncxx::v_noabi::document::view{::bson_get_data(opts_bson), opts_bson->len};
}

warning: passing 4-byte aligned argument to 8-byte aligned parameter 1 of 'bson_get_data' may result in an unaligned pointer access [-Walign-mismatch]

libmongoc::uri_get_options mocks bson_t* mongoc_uri_get_options(mongoc_uri_t const* uri) by providing a function call operator overload which returns bson_t*. However, because the return type is specified in terms of a template parameter R, per template <typename R, typename... Args> class mock<R(MONGOCXX_ABI_CDECL*)(Args...)>, the compiler fails to observe the extended alignment attributes for bson_t*. The resulting opts_bson pointer, when passed to bson_get_data, is assumed to have a fundamental alignment of 4 rather than an extended alignment of 128, producing the warning above. The solution is therefore to explicitly specialize and overload with an explicit reference to bson_t* in its declaration (not via a template parameter) to preserve alignment attributes.

[kevinAlbs]

Nicely done. LGTM with a suggestion to reduce the storage size.

[eramongodb]

Validation is blocked on #1332.

[vector-of-bool]

The fact that alignof gives a different answer if the operand is a template parameter is beyond horrifying, but oh well.

LGTM.