Skip to content

QtExecutor::post() heap-use-after-free when a nested Completion chain's post outlives the executor's teardown #127

Description

@Yaraslaut

Summary

QtExecutor::post() (include/morph/qt/qt_executor.hpp) can be called on
an already-destroyed QtExecutor, reading freed memory. This is a genuine
heap-use-after-free, not a false positive: it reproduces as a plain
SIGSEGV even without a sanitizer
, and ThreadSanitizer additionally
diagnoses it precisely.

First observed on ladder-kanban-impl's PR #121, in
test_kanban_stress.cpp's "Concurrent MoveTaskPosition calls (N=4) never desync positions -- run under ThreadSanitizer" test, once that branch wired
kanban's Qt-linked concurrent tests into a real ThreadSanitizer CI leg for
the first time (the Kanban / ThreadSanitizer job). The same underlying bug
also crashed two uninstrumented CI legs outright:
Linux / clang-coverage (in AttachmentServer/BoardBridge's own
concurrent-move test) and Linux / all optional features (clang) (in
BoardBridge's EventPoller test) — three different tests, same root
cause, none of them kanban-specific in nature.

This is not application code's fault: QtExecutor, Completion, and the
worker-pool/executor teardown ordering it depends on are all shared
framework code (include/morph/core/completion.hpp,
include/morph/core/executor.hpp, include/morph/qt/qt_executor.hpp).
Anything using morph::async::Completion chained across an executor
boundary — which morph::bridge::Bridge does on every call — is exposed to
this whenever the executor is torn down promptly after the caller observes
its own top-level completion as "done."

Root cause

  • QtExecutor::post() uses Qt::QueuedConnection
    (QMetaObject::invokeMethod(_context, std::move(fn), Qt::QueuedConnection),
    qt_executor.hpp:40) — it enqueues fn and returns immediately. It does
    not run fn, and the queued event carries no reference back to the
    QtExecutor instance itself once it's been queued.
  • ThreadPoolExecutor's destructor (executor.hpp:66-82) blocks until every
    worker thread exits, and workers drain their queue before exiting — so by
    the time it returns, every post() call a pool task made is guaranteed to
    have happened. It says nothing about whether the resulting event was
    ever delivered
    .
  • morph::bridge::Bridge::executeVia (include/morph/core/bridge.hpp)
    chains three Completion<T> objects per dispatched action, each
    settled from inside the previous one's delivered callback:
    1. LocalBackend's own internal completion (anyCompletion,
      bridge.hpp:1509), settled by a pool thread.
    2. executeVia's per-call completion (typedState, bridge.hpp:1361),
      settled at bridge.hpp:1613 from inside anyCompletion's own
      .onError continuation — itself delivered by the first post().
    3. The caller's own completion — whatever .then()/.onError() the
      application code attached — settled from inside whatever typedState
      delivers via the second post().
  • A caller that only waits on its own top-level completion (e.g.
    test_kanban_stress.cpp's pumpUntil([&]{ return outstanding.load() == 0; }), decremented from inside step 3's own handler) has no visibility into
    steps 1/2's intermediate posts. It can observe "done" and let the
    QtExecutor be torn down while an intermediate post is still queued,
    undelivered, on the Qt event loop.
  • When that stale event is finally pumped, its callback body itself calls
    cbExec->post(...) again (the next link in the chain) — and if the
    QtExecutor it's calling into has already been freed, that post() call
    reads this->_context off freed memory: exactly the crash TSan reports
    (heap-use-after-free ... in morph::qt::QtExecutor::post,
    qt_executor.hpp:40:35) and what an uninstrumented build simply segfaults
    on.

Minimal repro

Framework-only — no kanban, no Bridge, just ThreadPoolExecutor +
QtExecutor + Completion/Promise::makeSettleable, reproducing the exact
three-level nesting executeVia produces. Segfaults 5/5 runs on Linux
and Windows (MSVC and Clang) without a sanitizer; add
target_compile_definitions(repro PRIVATE APPLY_FIX) (applying the same
drain the fix below uses) and it exits cleanly 10/10 instead.

repro.cpp:

#include <QCoreApplication>
#include <QEventLoop>
#include <atomic>
#include <cstdio>
#include <memory>
#include <morph/core/completion.hpp>
#include <morph/core/executor.hpp>
#include <morph/qt/qt_executor.hpp>

using morph::async::Completion;

template <typename T>
struct Pending {
    Completion<T> completion;
    typename Completion<T>::Promise promise;

    explicit Pending(std::pair<Completion<T>, typename Completion<T>::Promise> pair)
        : completion{std::move(pair.first)}, promise{std::move(pair.second)} {}
};

int main(int argc, char** argv) {
    QCoreApplication app(argc, argv);

    auto workerPool = std::make_unique<morph::exec::ThreadPoolExecutor>(4);
    auto qtExecutor = std::make_unique<morph::qt::QtExecutor>();

    std::atomic<int> outstanding{1};

    // Three Completion<T> objects sharing one QtExecutor, each settled from
    // *inside* the previous one's delivered callback -- mirrors
    // Bridge::executeVia's real shape: LocalBackend's own completion ->
    // executeVia's typedState -> the caller's own attached handler.
    auto outer = std::make_shared<Pending<int>>(Completion<int>::makeSettleable(qtExecutor.get()));
    auto inner = std::make_shared<Pending<int>>(Completion<int>::makeSettleable(qtExecutor.get()));
    auto outermostCaller = std::make_shared<Pending<int>>(Completion<int>::makeSettleable(qtExecutor.get()));

    outermostCaller->completion.onError([](const std::exception_ptr&) {
        std::fprintf(stderr, "[repro] outermostCaller onError handler ran\n");
    });

    inner->completion.onError([inner, outermostCaller](const std::exception_ptr&) {
        outermostCaller->promise.reject(std::make_exception_ptr(std::runtime_error{"boom again"}));
    });

    outer->completion.then([&outstanding, inner](int) {
        inner->promise.reject(std::make_exception_ptr(std::runtime_error{"boom"}));
        --outstanding;  // caller-visible "done" -- but inner's own post is
                        // still queued, undelivered, at this exact moment.
    });

    // A pool thread resolves `outer` -- the first post(). ThreadPoolExecutor's
    // destructor guarantees this call *happens*, not that it's *delivered*.
    workerPool->post([outer]() { outer->promise.resolve(42); });

    while (outstanding.load() != 0) {
        QCoreApplication::processEvents();
    }

    workerPool.reset();       // joins -- every post() so far was made
#ifdef APPLY_FIX
    for (int slice = 0; slice < 5; ++slice) {
        QCoreApplication::processEvents(QEventLoop::AllEvents, 10);
    }
#endif
    qtExecutor.reset();       // <-- freed while an undelivered post is pending

    // Delivers inner's callback, which -- pre-fix -- calls cbExec->post()
    // again against the now-freed QtExecutor: qt_executor.hpp:40:35.
    QCoreApplication::processEvents();

    std::fprintf(stderr, "[repro] exited cleanly\n");
    return 0;
}

CMakeLists.txt:

cmake_minimum_required(VERSION 3.25)
project(qt_executor_uaf_repro CXX)
set(CMAKE_CXX_STANDARD 23)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_AUTOMOC ON)
find_package(Qt6 REQUIRED COMPONENTS Core)
add_executable(repro repro.cpp)
target_include_directories(repro PRIVATE "<path to morph>/include")
target_link_libraries(repro PRIVATE Qt6::Core)
# target_compile_definitions(repro PRIVATE APPLY_FIX)   # uncomment to see the fix hold

Run:

$ cmake -S . -B build -DCMAKE_PREFIX_PATH=<Qt6 lib/cmake dir> && cmake --build build
$ for i in 1 2 3 4 5; do ./build/repro; echo "exit: $?"; done
Segmentation fault
exit: 139
Segmentation fault
exit: 139
Segmentation fault
exit: 139
Segmentation fault
exit: 139
Segmentation fault
exit: 139

Actual CI evidence (PR #121, run 32177977984, job 95844175149)

WARNING: ThreadSanitizer: heap-use-after-free (virtual call vs free) (pid=8907)
  Read of size 8 at 0x720400001060 by main thread:
    #0 morph::async::detail::CompletionState<kanban::GetBoardResult>::setException(...)
       include/morph/core/completion.hpp:112:21
    ...
SUMMARY: ThreadSanitizer: heap-use-after-free (virtual call vs free)
  include/morph/core/completion.hpp:112:21 in
  morph::async::detail::CompletionState<kanban::GetBoardResult>::setException(...)
==================
WARNING: ThreadSanitizer: heap-use-after-free (pid=8907)
  Read of size 8 at 0x720400001068 by main thread:
    #0 morph::qt::QtExecutor::post(std::function<void ()>)
       include/morph/qt/qt_executor.hpp:40:35
    ...
SUMMARY: ThreadSanitizer: heap-use-after-free
  include/morph/qt/qt_executor.hpp:40:35 in morph::qt::QtExecutor::post(std::function<void ()>)

Both the read (use) and the earlier write (the ~QtExecutor() free) are
attributed to the main thread — this is not a cross-thread data race in
the usual sense, it's a same-thread use-after-free purely from event
delivery being deferred past object teardown.

Suggested fix

Applied locally as a workaround in the one place that hit it
(examples/common/testkit/backend_rig.hpp's ~BackendRig(), PR #121 commit
917ea54): explicitly join the worker pool early, then drain the Qt event
loop for a few bounded slices (long enough for a chained post to arrive, run,
and issue its own nested post, and for that to arrive and run too) before
freeing the executor:

~BackendRig() {
    if (_wsServer) {
        _wsServer->closeGracefully(std::chrono::milliseconds{2000});
    }
    _workerPool.reset();  // join early: every post() so far has now happened
    for (int slice = 0; slice < 5; ++slice) {
        QCoreApplication::processEvents(QEventLoop::AllEvents, 10);
    }
    // ... normal member destruction (including _qtExecutor) proceeds
}

This is a workaround at one call site, not a framework fix — every other
QtExecutor owner (main.cpp in each ladder rung, any long-lived
AppContext) has the same latent exposure whenever it tears down a
ThreadPoolExecutor + QtExecutor pair while a Bridge-mediated call chain
might still have an in-flight nested completion. Candidate framework-level
fixes worth considering:

  • Give QtExecutor a way to invalidate/cancel its own not-yet-delivered
    posted events at destruction time (e.g. a shared "alive" flag the queued
    closure checks before running, similar to the liveness()/weak_ptr
    pattern morph::bridge::Bridge already uses for its own callbacks).
  • Have Completion/CompletionState track "in-flight nested posts" so a
    caller can drain deterministically instead of a fixed number of
    processEvents() slices (which is inherently a "probably enough" bound,
    not a proof).
  • Document (in docs/spec/core/completion.md and/or a Qt-executor spec, if
    one exists) that any QtExecutor owner must drain pending events before
    destruction when nested Completion chains are possible, since this is
    easy to get wrong silently — as this bug demonstrates, native/
    uninstrumented builds can pass for a long time before the timing window is
    hit.

Environment

  • Found on: ubuntu-latest GitHub Actions runners, GCC 14 / Clang, Qt
    6.8.1, ThreadSanitizer.
  • Also reproduces on Windows (MSVC 19.51, Qt 6.11.1) with the standalone
    repro above — not sanitizer- or platform-specific.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions