diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0e02b2fa..b29bfeca 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -255,10 +255,22 @@ jobs: # on a runner with no display. QT_QPA_PLATFORM: offscreen run: | + # morph::testkit::OomInjector (tests/oom_injector.cpp) overrides + # the process-wide operator new/delete to force std::bad_alloc on + # demand -- ASan and TSan's own runtimes already interpose + # operator new/delete themselves, and linking a second, competing + # definition fails with "multiple definition of `operator + # new(unsigned long)'" (confirmed in CI). Excluded by test name on + # exactly these two legs; every other CI leg (plain clang/gcc, + # Windows, ubsan, coverage) runs these tests normally. + EXCLUDE_ARGS=() + if [ "${{ matrix.preset }}" = "clang-asan" ] || [ "${{ matrix.preset }}" = "clang-tsan" ]; then + EXCLUDE_ARGS+=(-E "OomInjector|morph#108") + fi if [ "${{ matrix.preset }}" = "clang-coverage" ]; then - LLVM_PROFILE_FILE="build/clang-coverage/%p.profraw" ctest --preset clang-coverage + LLVM_PROFILE_FILE="build/clang-coverage/%p.profraw" ctest --preset clang-coverage "${EXCLUDE_ARGS[@]}" else - ctest --preset ${{ matrix.preset }} + ctest --preset ${{ matrix.preset }} "${EXCLUDE_ARGS[@]}" fi - name: Generate coverage report @@ -688,6 +700,18 @@ jobs: exit 1 fi echo "── memcheck: $suite ──" + # [oom-injector]/[issue108]: morph::testkit::OomInjector + # overrides the process-wide operator new/delete + # (tests/oom_injector.cpp) to force std::bad_alloc on demand -- + # Valgrind's own memcheck instrumentation intercepts allocations + # at a layer this override does not reach, so the injector + # silently never fires under Valgrind (confirmed: even its own + # self-tests fail here). Skipped for this leg only; every other + # CI leg (plain clang/gcc, Windows) runs these tests normally. + EXTRA_ARGS=() + if [ "$suite" = "tests/morph_tests" ]; then + EXTRA_ARGS=("~[oom-injector]" "~[issue108]") + fi valgrind \ --tool=memcheck \ --leak-check=full \ @@ -695,7 +719,7 @@ jobs: --errors-for-leak-kinds=definite,indirect \ --error-exitcode=1 \ --suppressions=cmake/valgrind.supp \ - "$binary" || status=1 + "$binary" "${EXTRA_ARGS[@]}" || status=1 done exit "$status" diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4897343f..94e5e8b7 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -5,6 +5,8 @@ morph_generate_pinned_facts_header( ) add_executable(morph_tests + oom_injector.cpp + test_oom_injector.cpp test_executor.cpp test_example.cpp test_executor_extra.cpp diff --git a/tests/oom_injector.cpp b/tests/oom_injector.cpp new file mode 100644 index 00000000..ef0af605 --- /dev/null +++ b/tests/oom_injector.cpp @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 + +#include "oom_injector.hpp" + +#include +#include +#include + +// ASan/TSan already define their own operator new/operator new[]/operator +// delete/operator delete[] inside their runtime (libclang_rt.{asan,tsan}_cxx.a) +// to track allocations for their own instrumentation. Defining this file's +// own overloads under either sanitizer fails at *link* time with "multiple +// definition of `operator new(unsigned long)'" against that runtime archive +// -- ctest-level test exclusion (see .github/workflows/ci.yml) cannot help +// here, since the conflict happens before any test ever runs. +// +// Detected via nested #ifdef/#if blocks (not one combined boolean +// expression): MSVC's preprocessor does not define __has_feature at all, and +// some preprocessors do not short-circuit `defined(__has_feature) && +// __has_feature(...)` on a single line the way C++ code would -- they can +// still try to macro-expand `__has_feature` as a bare identifier and choke +// on the unmatched parenthesis that follows (`__has_feature(address_ +// sanitizer)`) once `defined(__has_feature)` alone is false. Nesting avoids +// ever writing `__has_feature` on a line MSVC actually preprocesses. +// +// GCC and Clang both define __SANITIZE_ADDRESS__/__SANITIZE_THREAD__ +// whenever the corresponding -fsanitize=address/thread flag is active, which +// covers this repo's own clang-asan/clang-tsan presets without needing +// __has_feature at all; the __has_feature branch below only matters for a +// Clang invocation that enables a sanitizer through some other means. +#if defined(__SANITIZE_ADDRESS__) || defined(__SANITIZE_THREAD__) +#define MORPH_TESTKIT_UNDER_ASAN_OR_TSAN +#elif defined(__has_feature) +#if __has_feature(address_sanitizer) +#define MORPH_TESTKIT_UNDER_ASAN_OR_TSAN +#elif __has_feature(thread_sanitizer) +#define MORPH_TESTKIT_UNDER_ASAN_OR_TSAN +#endif +#endif + +namespace { + +// injectorArmed off means no injector active on this thread. While armed, +// the next operator new call with size >= minSizeToFail throws and disarms +// (one-shot). Plain built-in types only -- these variables' own +// reads/writes must never themselves allocate, or arming the injector would +// recurse into itself the moment operator new next runs. thread_local, not +// a class member: operator new below is a free function with no `this` to +// hang state off, and every thread needs its own independent state (see the +// header's own @par Thread safety). +// NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) -- this +// state *is* the seam; there is no non-global way to reach into a bare +// operator new call from outside. +thread_local std::size_t minSizeToFail = 0; +thread_local bool injectorArmed = false; +// NOLINTEND(cppcoreguidelines-avoid-non-const-global-variables) + +} // namespace + +namespace morph::testkit { + +OomInjector::OomInjector(std::size_t minSize) { +#ifdef MORPH_TESTKIT_UNDER_ASAN_OR_TSAN + // The operator new/delete overrides below are compiled out under this + // build (see the guard around them). Constructing an OomInjector here + // would silently do nothing, so this throws instead of letting a test + // misread that silence as "the injected failure never happened to + // trigger". In practice this never fires: every test using OomInjector + // is excluded from the clang-asan/clang-tsan CI legs by tag (see + // .github/workflows/ci.yml) -- this is a correctness backstop, not the + // primary mechanism. + (void)minSize; + throw std::logic_error( + "OomInjector: unusable under ASan/TSan (operator new/delete overrides are compiled out -- " + "see oom_injector.cpp)"); +#else + if (injectorArmed) { + throw std::logic_error("OomInjector: another instance is already active on this thread"); + } + injectorArmed = true; + minSizeToFail = minSize; +#endif +} + +OomInjector::~OomInjector() { + injectorArmed = false; + minSizeToFail = 0; +} + +} // namespace morph::testkit + +#ifndef MORPH_TESTKIT_UNDER_ASAN_OR_TSAN + +namespace { + +// Every operator new overload below funnels through this so the trigger +// logic lives in one place. Recursion guard: reading/writing the +// thread_local state above touches only built-ins, never the heap, so this +// cannot re-enter itself. +void* allocateOrInject(std::size_t size) { + if (injectorArmed && size >= minSizeToFail) { + injectorArmed = false; // one-shot: disarm before throwing, so the + // catch block itself (and anything else on + // this thread afterward) allocates normally. + throw std::bad_alloc{}; + } + // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- + // this *is* the process-wide operator new/delete pair; std::malloc/free + // is what it has to be built from. + if (void* ptr = std::malloc(size == 0 ? 1 : size)) { + return ptr; + } + throw std::bad_alloc{}; +} + +} // namespace + +void* operator new(std::size_t size) { + return allocateOrInject(size); +} + +void* operator new[](std::size_t size) { + return allocateOrInject(size); +} + +void* operator new(std::size_t size, const std::nothrow_t& tag) noexcept { + (void)tag; + try { + return allocateOrInject(size); + } catch (...) { + return nullptr; + } +} + +void* operator new[](std::size_t size, const std::nothrow_t& tag) noexcept { + (void)tag; + try { + return allocateOrInject(size); + } catch (...) { + return nullptr; + } +} + +// NOLINTBEGIN(readability-inconsistent-declaration-parameter-name, +// cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) -- these +// replace the library's own global operator delete overloads; the "sized" +// forms' declarations in don't name their second parameter, and +// std::free is what a hand-written operator delete has to call to release +// what allocateOrInject's std::malloc above returned. +void operator delete(void* ptr) noexcept { + std::free(ptr); +} + +void operator delete[](void* ptr) noexcept { + std::free(ptr); +} + +void operator delete(void* ptr, std::size_t size) noexcept { + (void)size; + std::free(ptr); +} + +void operator delete[](void* ptr, std::size_t size) noexcept { + (void)size; + std::free(ptr); +} +// NOLINTEND(readability-inconsistent-declaration-parameter-name, +// cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) + +#endif // !MORPH_TESTKIT_UNDER_ASAN_OR_TSAN diff --git a/tests/oom_injector.hpp b/tests/oom_injector.hpp new file mode 100644 index 00000000..fc902e44 --- /dev/null +++ b/tests/oom_injector.hpp @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: Apache-2.0 + +#pragma once +#include + +namespace morph::testkit { + +/// @brief RAII scope that makes the next `operator new` call whose +/// requested size is at least @c minSize throw `std::bad_alloc` on +/// the current thread, so a test can drive a real allocation-failure +/// path (a `catch (...)` around a `std::string`/container assignment, +/// for example) without exhausting process memory or forking/mocking +/// the standard allocator. +/// +/// Test-only tooling, not part of the shipped library: `oom_injector.cpp` +/// defines `operator new`/`operator delete` for the whole binary it links +/// into, so this lives under `tests/` and is only ever linked into test +/// executables, never into `morph`'s own interface library or any example +/// app -- a real consumer's process must never get its allocator replaced +/// by test-only code. +/// +/// @par Incompatible with ASan, TSan, and Valgrind +/// ASan and TSan's own runtimes already interpose `operator new`/`operator +/// delete` themselves; linking this seam's own definitions alongside either +/// one fails at link time with "multiple definition of `operator +/// new(unsigned long)'" (confirmed in CI). Valgrind's memcheck intercepts +/// allocations at a layer this override does not reach, so the injector +/// silently never fires under it (confirmed: even its own self-tests fail +/// there). `.github/workflows/ci.yml` excludes every test tagged +/// `[oom-injector]`/`[issue108]` on the `clang-asan`/`clang-tsan` legs (by +/// `ctest -E`) and the Valgrind leg (by Catch2 tag filter) for exactly this +/// reason -- a test using this seam must carry one of those tags so it is +/// excluded consistently on every leg where the override cannot work. +/// +/// @par Why this exists +/// Several `catch (...)` blocks across the codebase (see +/// `LASTRADA-Software/morph#108`) only ever fire on `std::bad_alloc` from a +/// real allocation failure -- there is no other way into them. That is not +/// portably reachable from a unit test without a seam: this class overrides +/// the process-wide `operator new`/`operator new[]` (defined once in +/// `oom_injector.cpp`) to consult a thread-local size threshold before +/// delegating to the real allocator. +/// +/// @par Why a size predicate, not a raw allocation count +/// An earlier design counted "the Nth `operator new` call after arming" — +/// rejected before this shipped, because that count depends on exactly how +/// many heap allocations the standard library, the test's own setup code, +/// and even unrelated `std::string`/container growth perform first, which +/// varies across STL implementations and can shift with an unrelated +/// compiler/library upgrade. A test would silently start failing the wrong +/// allocation (or none at all) the moment that count changed, without +/// necessarily failing loudly. A size predicate instead targets what the +/// allocation actually *is*: e.g. "the next allocation of at least 64 +/// bytes" reliably matches a specific long string's heap buffer (a +/// short-string-optimized `std::string` never calls `operator new` at all), +/// so a test picks a source string long enough to defeat SSO on every +/// supported standard library and lets the predicate find that allocation +/// regardless of how many smaller, unrelated allocations happen around it. +/// +/// @par Why there is no occurrence-count parameter +/// A call path that copies the same long value more than once before +/// reaching the statement a test actually wants to fail (e.g. a setup copy +/// taken before a dispatch call, followed by another copy inside that same +/// call stack when a callback fires inline) cannot be disambiguated by size +/// alone. An earlier revision added a "skip the first K-1 matches" +/// occurrence count to handle exactly that case — and it was removed after +/// a value tuned against one STL's allocator (MSVC) failed to reproduce on +/// two others (libstdc++, libc++) in CI: the number of same-shaped copies +/// before a target line is itself an implementation detail, not something +/// this seam can portably parameterise around. A call path with that shape +/// is not a good fit for `OomInjector` — pick a different reproduction (a +/// path where the target copy is the *first* allocation of its size, as the +/// out-of-frame test in `test_async_registration.cpp` does) rather than +/// counting occurrences. +/// +/// @par Usage +/// ```cpp +/// { +/// // "expected" must be long enough that copying it heap-allocates on +/// // every supported STL (well past libstdc++/libc++/MSVC's ~15-23 +/// // byte SSO buffers). +/// std::string expected(128, 'x'); +/// morph::testkit::OomInjector inject{/*minSize=*/64}; +/// REQUIRE_THROWS_AS(codeThatCopiesExpectedSomewhere(expected), std::bad_alloc); +/// } // scope ends here: later allocations succeed normally again, even if +/// // the matching allocation never actually happened. +/// ``` +/// The first `operator new` call after construction whose `size >= minSize` +/// throws `std::bad_alloc` instead of allocating; every smaller allocation +/// before it (and every allocation, of any size, after it fires — including +/// inside the `catch` block itself) allocates normally: this is a one-shot +/// trigger, not a standing size filter. Only one `OomInjector` may be active +/// per thread at a time — nesting two throws `std::logic_error` from the +/// inner constructor, since both would otherwise race over the same +/// thread-local state. +/// +/// @par Thread safety +/// The injection state is `thread_local`: an `OomInjector` constructed on +/// one thread only affects `operator new` calls made from that same thread. +/// Other threads' allocations are never affected, so a test that spins up +/// worker threads around the code under test must construct the injector on +/// whichever thread actually performs the allocation it wants to fail. +/// +/// @par Why this isn't a `morph::core::FileIoOps`-shaped injectable parameter +/// `FileIoOps` works because `FileActionLog`/`FileOfflineQueue` call the raw +/// syscall directly and can take an injectable strategy object in their own +/// constructor. The `catch (...)` blocks this class targets guard a plain +/// `std::string` copy-assignment with no such seam to thread through -- the +/// allocation happens inside `std::string::operator=` itself, several layers +/// below any parameter `Bridge` could plausibly accept. Overriding the +/// global allocator is the standard technique for exactly this shape of gap. +class OomInjector { + public: + /// @brief Arms the injector so the first future `operator new` call on + /// this thread whose requested size is at least @p minSize + /// throws `std::bad_alloc` instead of allocating. Smaller + /// allocations before it succeed normally and do not count + /// against the trigger. + /// @param minSize Minimum allocation size, in bytes, that should fail. + /// @throws std::logic_error if another `OomInjector` is already active + /// on this thread. + explicit OomInjector(std::size_t minSize); + + /// @brief Disarms the injector. Allocations on this thread after this + /// point succeed normally again, whether or not the matching + /// allocation ever actually happened. + ~OomInjector(); + + OomInjector(const OomInjector&) = delete; + OomInjector& operator=(const OomInjector&) = delete; + OomInjector(OomInjector&&) = delete; + OomInjector& operator=(OomInjector&&) = delete; +}; + +} // namespace morph::testkit diff --git a/tests/test_async_registration.cpp b/tests/test_async_registration.cpp index 6741dd8b..95d25240 100644 --- a/tests/test_async_registration.cpp +++ b/tests/test_async_registration.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -31,6 +32,7 @@ #include #include +#include "oom_injector.hpp" #include "test_support.hpp" namespace { @@ -1822,3 +1824,111 @@ TEST_CASE("ensureBoundAsync's out-of-frame success callback is a genuine no-op o REQUIRE_NOTHROW(rawBackend->completeNext()); SUCCEED("completing a registerModelSharedAsync reply after the binding itself is gone did not crash"); } + +// --------------------------------------------------------------------------- +// Coverage for LASTRADA-Software/morph#108: attachHandlerAsync's two +// success-path `catch (...)` blocks (the out-of-frame callback below, and its +// in-frame claimHandoff counterpart) only ever fire on std::bad_alloc from a +// real allocation failure inside the strongBinding->contextKey/primary copy- +// assignment. morph::testkit::OomInjector (see oom_injector.hpp) makes that +// failure happen for real, on demand, instead of leaving both branches +// permanently undocumented-but-untested. +// +// AOmKeyModel below is std::string-keyed (PrimaryKeyOf == std::string) so its +// primary key is morph::model::keyToString's std::string pass-through, not a +// std::to_string of an integer -- the test picks a key long enough (256 +// bytes) to defeat every supported standard library's short-string +// optimization, so copying it genuinely allocates and OomInjector's size +// predicate has a real allocation to catch. +// NOLINTBEGIN(misc-use-internal-linkage) -- glaze's reflection needs these to +// be externally linked, not file-local, unlike the anonymous-namespace types +// above. +struct AOmTouch { + std::string key; + int amount = 0; +}; + +struct AOmKeyModel { + using PrimaryKey = std::string; + int value = 0; + int execute(const AOmTouch& act) { + value += act.amount; + return value; + } +}; + +BRIDGE_REGISTER_MODEL(AOmKeyModel, "AOm_KeyModel") +BRIDGE_REGISTER_ACTION(AOmKeyModel, AOmTouch, "AOm_Touch") +BRIDGE_KEY_FROM(AOmTouch, &AOmTouch::key); +// NOLINTEND(misc-use-internal-linkage) + +TEST_CASE( + "attachHandlerAsync's out-of-frame success callback surfaces a real allocation failure through onDone " + "(morph#108)", + "[bridge][registration][issue108]") { + SyncExec cbExec; + auto backend = std::make_shared(); + morph::bridge::Bridge bridge{std::make_unique(backend)}; + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + // 256 bytes defeats SSO on every supported standard library (libstdc++, + // libc++, and MSVC's implementations all inline at most ~23 bytes), so + // copying it into strongBinding->contextKey/primary genuinely allocates. + std::string const longKey(256, 'k'); + std::atomic result{-1}; + std::atomic failed{false}; + auto pending = handler.execute(AOmTouch{.key = longKey, .amount = 7}); + pending.then([&](int val) { result.store(val); }).onError([&](const std::exception_ptr&) { failed.store(true); }); + + REQUIRE(backend->pendingCount() == 1); + + // Arm right before the reply is completed: the attach's own dispatch + // machinery (registerModel/factory()) allocates plenty, but none of it + // copies `longKey` -- only the success callback's `strongBinding-> + // contextKey = primaryCopy` does, and it is >= 128 bytes, so the + // threshold only ever matches that copy, not the setup noise before it. + { + morph::testkit::OomInjector inject{/*minSize=*/128}; + backend->completeNext(); + } + + REQUIRE(morph::testing::waitUntil([&] { return result.load() != -1 || failed.load(); })); + CHECK(result.load() == -1); + CHECK(failed.load()); + // The failed publish left the handler unattached, exactly like any other + // attach failure -- not a half-published, corrupted binding. + CHECK_FALSE(handler.primary().has_value()); + + // The handler is still usable afterward: a fresh attach with a normal, + // short key succeeds, proving the injected failure left no corruption + // behind (the injector already disarmed itself after firing once). + std::atomic secondResult{-1}; + handler.execute(AOmTouch{.key = "short", .amount = 3}) + .then([&](int val) { secondResult.store(val); }) + .onError([&](const std::exception_ptr&) { failed.store(true); }); + REQUIRE(backend->pendingCount() == 1); + backend->completeNext(); + REQUIRE(morph::testing::waitUntil([&] { return secondResult.load() != -1; })); + CHECK(secondResult.load() == 3); + CHECK(handler.primary().value_or("") == "short"); +} + +// attachHandlerAsync's in-frame claimHandoff success path (binding-> +// contextKey = primaryCopy, reached when a backend's attachModelAsync +// completes synchronously -- see InlineCompletingBackend above) has the +// identical shape of catch (...) as the out-of-frame callback the test above +// targets, and is deliberately NOT given its own forced-OOM test: the whole +// call happens in one stack, so several of attachHandlerAsync's own earlier +// copies of the same primary key (the by-value `primary` parameter, `auto +// primaryCopy = primary;`, the {.contextKey=, .primary=} aggregate's two +// fields, the dispatch lambda's by-value capture) are the same >=SSO- +// defeating size as the target copy itself -- so a minSize-only OomInjector +// can't distinguish them, and the number of such copies before the target +// line is a real STL/compiler implementation detail (confirmed: an +// occurrence-count value tuned against MSVC's allocator did not reproduce on +// clang/libstdc++ or gcc/libstdc++ in CI, silently passing through instead +// of catching the target allocation). Forcing this specific occurrence +// portably would need either a structural change that gives the target copy +// a distinguishable allocation shape, or a seam finer-grained than a global +// allocator override can offer -- disproportionate machinery for one +// branch. Tracked by the same morph#108, not a second, separate ask. diff --git a/tests/test_oom_injector.cpp b/tests/test_oom_injector.cpp new file mode 100644 index 00000000..1a6704bc --- /dev/null +++ b/tests/test_oom_injector.cpp @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Self-test for morph::testkit::OomInjector (oom_injector.hpp), the same way +// scripts/test_check_deprecated_markers.sh and +// scripts/test_check_test_type_names.sh self-test their own checkers before +// anything else relies on them: a fault-injection seam nobody tests reports +// "it works" whether or not it actually fires the failure it claims to. + +#include +#include +#include +#include + +#include "oom_injector.hpp" + +TEST_CASE("OomInjector: an allocation below the threshold succeeds normally", "[testkit][oom-injector]") { + morph::testkit::OomInjector inject{/*minSize=*/1024}; + // A short string is well within SSO on every supported STL; even if it + // weren't, its heap buffer is far below the 1024-byte threshold above. + std::string small = "short"; + CHECK(small == "short"); +} + +TEST_CASE("OomInjector: the first allocation at or above the threshold throws std::bad_alloc", + "[testkit][oom-injector]") { + // 256 bytes defeats SSO on every supported standard library (libstdc++, + // libc++, and MSVC's implementations all inline at most ~23 bytes). + std::string source(256, 'x'); + morph::testkit::OomInjector inject{/*minSize=*/128}; + std::string target; + REQUIRE_THROWS_AS(target = source, std::bad_alloc); +} + +TEST_CASE("OomInjector: is one-shot -- only the first matching allocation fails, not every later one", + "[testkit][oom-injector]") { + std::string source(256, 'x'); + morph::testkit::OomInjector inject{/*minSize=*/128}; + std::string first; + REQUIRE_THROWS_AS(first = source, std::bad_alloc); + // The injector fired once and disarmed itself; a second copy of the same + // large string must now succeed normally, in the same scope. + std::string second; + REQUIRE_NOTHROW(second = source); + CHECK(second == source); +} + +TEST_CASE("OomInjector: disarms when its scope ends, even if the matching allocation never happened", + "[testkit][oom-injector]") { + { + morph::testkit::OomInjector inject{/*minSize=*/128}; + // Deliberately do not perform any allocation >= 128 bytes here. + } + // The armed-but-never-triggered injector must not leak into later code. + std::string source(256, 'x'); + std::string target; + REQUIRE_NOTHROW(target = source); + CHECK(target == source); +} + +TEST_CASE("OomInjector: two instances on the same thread cannot be active at once", "[testkit][oom-injector]") { + morph::testkit::OomInjector outer{/*minSize=*/128}; + REQUIRE_THROWS_AS((morph::testkit::OomInjector{/*minSize=*/128}), std::logic_error); +}