From 1359c7c24ded13539442e3a02098dadc125404a3 Mon Sep 17 00:00:00 2001 From: OmikronApex Date: Fri, 17 Jul 2026 18:28:06 +0200 Subject: [PATCH] =?UTF-8?q?fix(audio):=20P0=20lifecycle=20executor=20?= =?UTF-8?q?=E2=80=94=20serialized=20ops,=20generation=20guard,=20module=20?= =?UTF-8?q?pinning?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three identical field crash dumps (2026-07-17, CFG fail-fast 0xc0000409 param 0xa = GUARD_ICALL_CHECK_FAILURE on the JUCE message thread, plugin WndProc frame on the stack) traced to the dispatch timeout desync: every lifecycle call site could give up after 15 s while its op was still queued, leaving two owners mutating engine/plugin lifecycle state with no ordering authority. - New LifecycleExecutor (guide §12 P0): named lifecycle ops, strictly FIFO on the message thread, engine-generation stamped at submit and no-oped when stale. The wait is unbounded with a 15 s watchdog that reports and keeps waiting; false now always means "verifiably did not run" — the "false but it may still run later" state is gone. - initialize/doShutdown bump the generation and run as executor ops; shutdown is the one bounded caller (60 s, then leak the pump — beats hanging process exit behind a wedged driver call). - runDeviceLifecycleOp and closeAllPluginEditorWindows route through the executor; the editor-teardown 15 s give-up (delayed #56 UAF) is removed. - pinPluginModuleForever: plugin modules are pinned on load so JUCE's refcount can never unload them mid-session — a queued window/timer message into an unloaded module is the CFG fail-fast in the dumps. Co-Authored-By: Claude Fable 5 --- src/audio/CMakeLists.txt | 1 + src/audio/VSTHost.cpp | 13 +- src/audio/addon/AddonContext.cpp | 31 ++++- src/audio/addon/AddonContext.h | 39 +++--- src/audio/addon/EditorWindows.cpp | 39 ++---- src/audio/addon/LifecycleExecutor.cpp | 167 ++++++++++++++++++++++++++ src/audio/addon/LifecycleExecutor.h | 83 +++++++++++++ src/vst-host/CMakeLists.txt | 1 + 8 files changed, 317 insertions(+), 57 deletions(-) create mode 100644 src/audio/addon/LifecycleExecutor.cpp create mode 100644 src/audio/addon/LifecycleExecutor.h diff --git a/src/audio/CMakeLists.txt b/src/audio/CMakeLists.txt index 41489d9..bcc3917 100644 --- a/src/audio/CMakeLists.txt +++ b/src/audio/CMakeLists.txt @@ -12,6 +12,7 @@ set(AUDIO_SOURCES engine/SourcePool.cpp engine/ExtraInputs.cpp addon/AddonContext.cpp + addon/LifecycleExecutor.cpp addon/ChainOps.cpp addon/EditorWindows.cpp addon/DeviceBindings.cpp diff --git a/src/audio/VSTHost.cpp b/src/audio/VSTHost.cpp index c82d7ae..933cb57 100644 --- a/src/audio/VSTHost.cpp +++ b/src/audio/VSTHost.cpp @@ -1,5 +1,6 @@ #include "VSTHost.h" #include "VSTTrace.h" +#include "addon/LifecycleExecutor.h" // The out-of-process scan path is compiled only into the audio addon // (SLOPSMITH_AUDIO_ADDON, set in src/audio/CMakeLists.txt). slopsmith-vst-host @@ -513,6 +514,13 @@ std::unique_ptr VSTHost::loadPlugin( return nullptr; } + // P0 (guide §12): never unload a plugin module mid-session. JUCE frees + // the module when its last instance dies; a message already queued to a + // window/timer in that module then indirect-calls into unmapped code — + // the CFG fail-fast in the 2026-07-17 field dumps. + slopsmith::addon::pinPluginModuleForever( + matchedDesc.fileOrIdentifier.toRawUTF8()); + return instance; } @@ -568,7 +576,8 @@ void VSTHost::loadPluginAsync( // copy threads through both lambda hops. formatManager.createPluginInstanceAsync( matchedDesc, sampleRate, blockSize, - [cb = std::move(callback), name = matchedDesc.name] + [cb = std::move(callback), name = matchedDesc.name, + fileOrId = matchedDesc.fileOrIdentifier] (std::unique_ptr instance, const juce::String& error) { VST_TRACE("VSTHost.loadPluginAsync: createPluginInstanceAsync END " @@ -584,6 +593,8 @@ void VSTHost::loadPluginAsync( : juce::String("Failed to create plugin instance")); return; } + // P0: pin — see the sync loadPlugin path for rationale. + slopsmith::addon::pinPluginModuleForever(fileOrId.toRawUTF8()); cb(std::move(instance), {}); }); } diff --git a/src/audio/addon/AddonContext.cpp b/src/audio/addon/AddonContext.cpp index 79ee2a9..14a89bc 100644 --- a/src/audio/addon/AddonContext.cpp +++ b/src/audio/addon/AddonContext.cpp @@ -164,6 +164,10 @@ void initialize(std::function uiTeardownHook) // instead of treating it as already-done. alreadyShutDown.store(false, std::memory_order_release); + // New engine lifetime: ops still queued against the previous engine + // (or a previous failed init) are stale from here on. + bumpEngineGeneration(); + // Start JUCE message thread first (no-op on macOS) startJuceMessageThread(); @@ -172,8 +176,13 @@ void initialize(std::function uiTeardownHook) std::this_thread::sleep_for(std::chrono::milliseconds(200)); #endif - // Create engine on the JUCE message thread (or inline on macOS) - const bool initialized = dispatchOnMessageThread([]() { + // Create engine on the JUCE message thread (or inline on macOS). + // Unbounded wait (P0): on machines where the first device enumeration + // blocks the message thread for minutes (Focusrite ASIO, 2026-07-17 + // dumps), init completes late instead of "failing" while the engine + // appears anyway — the desync that left JS driving a half-initialized + // addon. + const bool initialized = runLifecycleOpOk("engine-init", []() { std::shared_ptr liveEngine; { std::lock_guard lock(engineMutex); @@ -194,8 +203,9 @@ void initialize(std::function uiTeardownHook) types[i].outputDevices.size()); }); if (!initialized) - fprintf(stderr, "[audio-native] initialize: engine creation did not complete " - "on the message thread; audio bindings will no-op until re-init\n"); + fprintf(stderr, "[audio-native] initialize: engine creation did not run " + "(pump gone or generation moved); audio bindings will " + "no-op until re-init\n"); } void doShutdown() @@ -217,6 +227,12 @@ void doShutdown() bool expected = false; if (!alreadyShutDown.compare_exchange_strong(expected, true)) return; + // Invalidate every lifecycle op still queued against the live engine: + // they run before our teardown op (FIFO) but must not touch a dying + // engine's devices. The teardown op below is stamped AFTER this bump, + // so it alone survives the generation check. + bumpEngineGeneration(); + // Release any LoadVSTWorker / LoadPresetWorker currently blocked on a // pending async load. Without this they'd wait forever on the // WaitableEvent — the createPluginInstanceAsync callback can't fire @@ -225,7 +241,10 @@ void doShutdown() if (juceRunning.load() || snapshotEngine() || snapshotVstHost()) { - const bool toreDown = dispatchOnMessageThread([]() { + // Bounded wait — the ONE bounded caller (LifecycleExecutor.h): at + // process exit, leaking the pump beats hanging exit forever behind + // a wedged driver call. + const bool toreDown = runLifecycleOpOk("engine-shutdown", []() { // Editors reference their slot's processor; engine.reset() below // frees the whole chain, so destroy the editor windows first (#56). if (shutdownUiTeardown) shutdownUiTeardown(); @@ -239,7 +258,7 @@ void doShutdown() std::lock_guard lock(vstHostMutex); vstHost.reset(); } - }); + }, /*maxWaitMs=*/60000); if (!toreDown) { // Editor teardown / stopAudio / engine destruction have NOT diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index 5f0ca2d..ff254ce 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -20,6 +20,7 @@ #include "../AudioEngine.h" #include "../VSTHost.h" +#include "LifecycleExecutor.h" #include @@ -74,36 +75,26 @@ inline bool dispatchOnMessageThread(Func&& func) // caller already IS the message thread (engine init runs there), because // dispatch-and-wait from the message thread would deadlock. // -// Returns false when the dispatched work did not verifiably complete (post -// refused or 15 s timeout) — same contract as dispatchOnMessageThread. -// CAPTURE RULE: on timeout the queued closure may still run later, so the -// closure must own everything it touches — capture by value (engine -// snapshot, args) and write results through a shared_ptr, never through -// references to the caller's stack. -// -// KNOWN LIMITATION (timeout desync): when the 15 s wait expires the binding -// reports failure to JS, but the closure may still complete afterwards — -// e.g. addSource returns -1 yet the source gets created, holding its input -// device until the next reconfigure. Memory-safe by the capture rule above, -// just not logically reconciled; only reachable with the message thread -// jammed > 15 s. Callers that care can re-query engine state (listSources, -// getCurrentDevice) after a reported failure. +// P0 (guide §12): routed through the LifecycleExecutor. False now means the +// op verifiably did NOT run (pump gone, or the engine generation moved while +// it was queued) — the old "false but it may still run later" desync state +// is gone. The wait is unbounded; a watchdog logs every 15 s while a driver +// call blocks the message thread. +// CAPTURE RULE unchanged: the closure must own everything it touches — +// capture by value (engine snapshot, args) and write results through a +// shared_ptr, never through references to the caller's stack. template inline bool runDeviceLifecycleOp(Func&& func) { #if JUCE_WINDOWS - // No MessageManager means the pump is gone (pre-init or mid-shutdown): - // report failure instead of running the mutation unserialised on the - // caller's thread — that would reintroduce the race this helper exists - // to prevent (CodeRabbit #113 review). - auto* mm = juce::MessageManager::getInstanceWithoutCreating(); - if (mm == nullptr) - return false; - if (!mm->isThisTheMessageThread()) - return dispatchOnMessageThread(std::forward(func)); -#endif + return runLifecycleOpOk("device-lifecycle", + std::function(std::forward(func))); +#else + // macOS: dispatch runs inline (no separate pump). Linux/ALSA keeps its + // long-standing "called from the Node main thread" contract untouched. func(); return true; +#endif } // Pending-async-load registry: LoadVSTWorker / LoadPresetWorker block on a diff --git a/src/audio/addon/EditorWindows.cpp b/src/audio/addon/EditorWindows.cpp index 1beb886..c55e6fc 100644 --- a/src/audio/addon/EditorWindows.cpp +++ b/src/audio/addon/EditorWindows.cpp @@ -86,35 +86,22 @@ void destroyAllPluginEditorWindowsOnMessageThread() // timeout so a lingering-editor UAF stays diagnosable. bool closeAllPluginEditorWindows() { - auto* mm = juce::MessageManager::getInstanceWithoutCreating(); - if (mm != nullptr && mm->isThisTheMessageThread()) + // P0 (guide §12): serialized behind every other lifecycle op with an + // unbounded wait — the old 15 s give-up let a caller proceed to free + // slot processors while this teardown was still queued (#56, delayed). + // A false return now means the teardown verifiably did NOT run (pump + // gone / generation moved) — callers still must not free processors on + // false, but there is no longer a "maybe it runs later" state. + // Note: inline execution when already on the message thread is handled + // inside runLifecycleOp. + const bool toreDown = runLifecycleOpOk("editor-teardown", []() { destroyAllPluginEditorWindowsOnMessageThread(); - return true; - } - - auto done = std::make_shared(); - const bool posted = juce::MessageManager::callAsync([done]() - { - destroyAllPluginEditorWindowsOnMessageThread(); - done->signal(); }); - if (!posted) - { - fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: message queue refused the post; " - "editors may still be alive\n"); - return false; - } - if (!done->wait(15000)) - { - // The queued teardown hasn't run: editors may still hold pointers into - // the chain. Callers must NOT free slot processors on a false return — - // proceeding here is exactly the #56 use-after-free, just delayed. - fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: editor teardown did not complete " - "within 15s; caller must not free chain processors\n"); - return false; - } - return true; + if (!toreDown) + fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: teardown did " + "not run; caller must not free chain processors\n"); + return toreDown; } Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info) diff --git a/src/audio/addon/LifecycleExecutor.cpp b/src/audio/addon/LifecycleExecutor.cpp new file mode 100644 index 0000000..8a1f6af --- /dev/null +++ b/src/audio/addon/LifecycleExecutor.cpp @@ -0,0 +1,167 @@ +// LifecycleExecutor implementation — see LifecycleExecutor.h for the +// contract and the field evidence that motivated it (guide §12 P0). + +#include "LifecycleExecutor.h" + +#include + +#include +#include +#include +#include + +#if JUCE_WINDOWS + #define WIN32_LEAN_AND_MEAN + #include +#endif + +namespace slopsmith::addon { + +static std::atomic engineGeneration{1}; + +std::uint64_t currentEngineGeneration() +{ + return engineGeneration.load(std::memory_order_acquire); +} + +std::uint64_t bumpEngineGeneration() +{ + return engineGeneration.fetch_add(1, std::memory_order_acq_rel) + 1; +} + +// Submission lock: MessageManager::callAsync is itself FIFO, but two +// threads racing between "decide to submit" and "post" could otherwise +// observe an ordering different from the one their callers reasoned about. +// All lifecycle submissions serialize here so queue order == submit order. +static std::mutex submitMutex; + +static constexpr int kWatchdogIntervalMs = 15000; + +LifecycleOpResult runLifecycleOp(const char* name, + std::function func, + int maxWaitMs) +{ + const auto stamped = currentEngineGeneration(); + + auto* mm = juce::MessageManager::getInstanceWithoutCreating(); + if (mm == nullptr) + { + fprintf(stderr, "[lifecycle] op '%s': no message pump; refusing\n", name); + return LifecycleOpResult::neverRan; + } + + // Already on the message thread (engine init path, macOS inline mode): + // dispatch-and-wait on ourselves would deadlock. Run inline — we are by + // definition serialized with every queued op. + if (mm->isThisTheMessageThread()) + { + if (stamped != currentEngineGeneration()) + { + fprintf(stderr, "[lifecycle] op '%s': stale generation (inline); skipped\n", name); + return LifecycleOpResult::stale; + } + func(); + return LifecycleOpResult::completed; + } + + struct Shared { + juce::WaitableEvent done; + std::atomic wasStale{false}; + }; + auto shared = std::make_shared(); + + bool posted = false; + { + std::lock_guard lock(submitMutex); + posted = juce::MessageManager::callAsync( + [func = std::move(func), shared, stamped, name]() mutable { + if (stamped != currentEngineGeneration()) + { + fprintf(stderr, "[lifecycle] op '%s': engine generation moved " + "%llu -> %llu while queued; skipped\n", + name, + (unsigned long long) stamped, + (unsigned long long) currentEngineGeneration()); + shared->wasStale.store(true, std::memory_order_release); + } + else + { + func(); + } + shared->done.signal(); + }); + } + if (!posted) + { + fprintf(stderr, "[lifecycle] op '%s': message queue refused the post; " + "op will not run\n", name); + return LifecycleOpResult::neverRan; + } + + int waitedMs = 0; + while (!shared->done.wait(kWatchdogIntervalMs)) + { + waitedMs += kWatchdogIntervalMs; + + // Pump death while we wait means the op can never run. Distinguish + // from a merely blocked pump: the MessageManager singleton is + // destroyed by stopJuceMessageThread. + if (juce::MessageManager::getInstanceWithoutCreating() == nullptr) + { + fprintf(stderr, "[lifecycle] op '%s': message pump died after %d ms " + "wait; op will not run\n", name, waitedMs); + return LifecycleOpResult::neverRan; + } + + if (maxWaitMs > 0 && waitedMs >= maxWaitMs) + { + fprintf(stderr, "[lifecycle] op '%s': abandoning wait after %d ms " + "(bounded caller); op may still run\n", name, waitedMs); + return LifecycleOpResult::abandonedWait; + } + + // Watchdog: report, never decide (guide §12 P0 item 5). The usual + // culprit for a multi-second stall is an audio driver call (ASIO + // open/reset) blocking the message thread. + fprintf(stderr, "[lifecycle] watchdog: op '%s' still waiting after %d ms — " + "message thread blocked (audio driver call?); continuing " + "to wait\n", name, waitedMs); + } + + return shared->wasStale.load(std::memory_order_acquire) + ? LifecycleOpResult::stale + : LifecycleOpResult::completed; +} + +void pinPluginModuleForever(const char* fileOrIdentifierUtf8) +{ +#if JUCE_WINDOWS + // JUCE loads the inner /Contents/x86_64-win/.vst3 (or a + // flat .vst3/.dll); either way the loaded module's base name is the + // path's final component. Pin by that name so the loader never unloads + // it, even when JUCE's module refcount hits zero. + const juce::String path = juce::String::fromUTF8(fileOrIdentifierUtf8); + const juce::String base = path.replaceCharacter('\\', '/') + .fromLastOccurrenceOf("/", false, false); + if (base.isEmpty()) + return; + + HMODULE h = nullptr; + if (GetModuleHandleExW(GET_MODULE_HANDLE_EX_FLAG_PIN, + base.toWideCharPointer(), &h)) + { + fprintf(stderr, "[lifecycle] pinned plugin module '%s' for process " + "lifetime\n", base.toRawUTF8()); + } + // Not currently loaded under that name (sandboxed plugin, or the inner + // file is named differently): nothing to pin — the sandbox child owns + // its own modules, and an in-process module we can't resolve here was + // loaded under some other base name and will be pinned on a later load + // if it ever resolves. Silent by design; this is a belt on JUCE's + // refcount braces, not a correctness gate. +#else + (void) fileOrIdentifierUtf8; +#endif +} + +} // namespace slopsmith::addon diff --git a/src/audio/addon/LifecycleExecutor.h b/src/audio/addon/LifecycleExecutor.h new file mode 100644 index 0000000..df9921b --- /dev/null +++ b/src/audio/addon/LifecycleExecutor.h @@ -0,0 +1,83 @@ +#pragma once + +// LifecycleExecutor — P0 of the audio architecture guide (§12 P0). +// +// One serialized queue for native lifecycle mutations (engine init/shutdown, +// device create/destroy, editor teardown). Ops are named, stamped with the +// engine generation at submit time, and executed strictly in submission +// order on the JUCE message thread. +// +// What this replaces: the "dispatch and give up after 15 s" contract +// (AddonContext.h, KNOWN LIMITATION). Field evidence 2026-07-17 (three +// identical CFG fail-fast dumps): a caller that abandons its wait proceeds +// to mutate lifecycle state while the abandoned op is still queued — two +// owners, no ordering authority. The executor removes the abandon path: +// +// - The wait is not bounded by a give-up. A watchdog logs every +// kWatchdogIntervalMs while an op is stalled (message thread blocked by +// a driver call) and keeps waiting. The op result the caller sees is +// always the truth of what ran. +// - The only false returns are "the op will never run" (message pump gone +// or post refused) and "the op was stale" (engine generation changed +// between submit and execution — see below). Callers can treat false as +// final; there is no "maybe it still runs later" state. +// - `maxWaitMs` exists for the one caller that must not block forever: +// process shutdown, where leaking the pump beats hanging exit. Everyone +// else passes 0 (unbounded). +// +// Generation guard: initialize/doShutdown bump the engine generation. An op +// stamped under an older generation no-ops when it finally executes, so +// work that was queued against a torn-down engine can never touch its +// replacement. This is the lifecycle-level generalization of +// chainGeneration. +// +// The executor serializes; it never authorizes. Policy (who may mutate +// what) belongs to the JS-side lease registry (guide §7.1). +// +// macOS: no background pump exists (AppKit owns the real main thread — +// AddonContext.cpp fork note), so ops execute inline on the caller thread, +// still generation-checked. Same contract, degenerate queue. + +#include +#include + +namespace slopsmith::addon { + +// Monotonic engine generation. Bumped by initialize()/doShutdown(). +std::uint64_t currentEngineGeneration(); +std::uint64_t bumpEngineGeneration(); + +enum class LifecycleOpResult { + completed, // op ran to completion under its submit-time generation + stale, // generation changed before the op ran; op no-oped + neverRan, // pump gone / post refused — op will never run + abandonedWait, // maxWaitMs expired; op may still run (shutdown-only path) +}; + +// Run `func` on the JUCE message thread, serialized behind every previously +// submitted lifecycle op. Blocks until the op completes (watchdog logs +// while stalled). `name` appears in every log line and in the watchdog. +// `maxWaitMs` = 0 means wait indefinitely (bail only if the pump dies). +LifecycleOpResult runLifecycleOp(const char* name, + std::function func, + int maxWaitMs = 0); + +// Convenience: true only for LifecycleOpResult::completed. +inline bool runLifecycleOpOk(const char* name, std::function func, + int maxWaitMs = 0) +{ + return runLifecycleOp(name, std::move(func), maxWaitMs) + == LifecycleOpResult::completed; +} + +// Pin a plugin module so the OS never unloads it for the life of the +// process (guide §12 P0 item 4). JUCE refcounts VST3 modules and unloads +// them when the last instance dies; a window/timer message already queued +// to that module's code then fires into unmapped or reused pages — the CFG +// fail-fast (0xc0000409 / FAST_FAIL_GUARD_ICALL_CHECK_FAILURE) in the +// 2026-07-17 dumps. The pin is by module base name (the inner +// x86_64-win/*.vst3 file JUCE actually loads shares the bundle's base +// name). No-op off Windows and for modules that are not currently loaded. +void pinPluginModuleForever(const char* fileOrIdentifierUtf8); + +} // namespace slopsmith::addon diff --git a/src/vst-host/CMakeLists.txt b/src/vst-host/CMakeLists.txt index a6462f0..a516a64 100644 --- a/src/vst-host/CMakeLists.txt +++ b/src/vst-host/CMakeLists.txt @@ -63,6 +63,7 @@ endfunction() set(VST_HOST_SOURCES main.cpp ../audio/VSTHost.cpp + ../audio/addon/LifecycleExecutor.cpp ../audio/Sandbox/Protocol.cpp ../audio/Sandbox/ControlChannel_shared.cpp ../audio/Sandbox/AudioChannel_shared.cpp