fix(audio): close plugin editor windows before freeing their processors (pause UAF #56) (#57)

* fix(audio): close in-process plugin editor windows before freeing their processors (#56) A `PluginEditorWindow` owns an `AudioProcessorEditor` bound to its slot's processor, but nothing tore those windows down when the chain was freed. On pause the renderer clears/reloads the chain (`clearChain` / `loadPreset`), which destroys every slot processor — leaving any open editor pointing at freed memory. Its next timer/paint callback then jumps through a dangling pointer: the reported ACCESS_VIOLATION / DEP-execute at an unmapped address, seconds after pausing (thread stack thick with `RB Final Leveler.vst3` editor-window frames calling back into slopsmith_audio.node). Fix: destroy the in-process editor windows BEFORE the processors they reference, in all three teardown paths: - ClearChain (JS thread) — close editors, then clear(). - LoadPresetWorker::Execute (libuv worker) — close editors, then clear() before rebuilding the chain. - doShutdown — destroy editors first inside the existing message-thread lambda, before engine.reset(). editorWindows holds JUCE GUI objects, so teardown must happen on the message thread. `closeAllPluginEditorWindows()` marshals via `dispatchOnMessageThread` (post-and-wait) so the caller blocks until every editor is gone — guaranteeing editors die before their processors. Callers already on the message thread (doShutdown) use the inline `destroyAllPluginEditorWindowsOnMessageThread()` to avoid a post-and-wait-on-self deadlock. On Linux/Windows the JUCE message thread is a dedicated std::thread, so ClearChain (Node) and the worker never deadlock; on macOS dispatch runs inline and in-process editors don't exist (sandboxed). Native addon builds clean (Release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review: fix macOS worker-thread editor teardown; assert precondition; report post/timeout Codex [P2]: closeAllPluginEditorWindows() delegated to dispatchOnMessageThread(), which runs inline under JUCE_MAC — so LoadPresetWorker::Execute() (libuv worker) could destroy JUCE DocumentWindow/AudioProcessorEditor objects off the message thread on macOS. Now branch on the caller's actual thread: run inline only when already on the message thread (else deadlock), otherwise post via MessageManager::callAsync (drained by the JUCE thread on Linux/Windows and the Node-main libuv timer on macOS) and wait. Correct on all platforms. Copilot: report a refused post / 15s wait timeout via stderr instead of silently assuming teardown completed (the previous "guarantee" wording overstated it); add JUCE_ASSERT_MESSAGE_THREAD to the inline variant as a debug tripwire. Native addon builds clean (Release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review(codex): don't callAsync+wait on an unpumped macOS MessageManager Codex round 2 [P2]: my round-1 fix routed the off-message-thread teardown through MessageManager::callAsync + WaitableEvent::wait on ALL platforms. On macOS there is no separate message-thread pump (startJuceMessageThread's JUCE_MAC branch only creates the manager; there is no dispatch loop), so a callAsync+wait from LoadPresetWorker's libuv worker would stall the full 15s timeout and then proceed with the editor still alive — the very UAF this targets. Platform-split the off-thread path, matching loadVstSandboxAware()'s existing JUCE_MAC handling: - Already on the message thread → inline (doShutdown; ClearChain on macOS). - Linux/Windows off-thread → post to the dedicated JUCE message thread + wait (with refused-post / timeout reporting). - macOS off-thread → clear inline (the pre-existing macOS worker-thread limitation). editorWindows is empty on macOS in practice (in-process editors route to the sandbox child), and the editor/processor UAF this targets is Windows-specific, so no message-thread hop is needed there. Native addon builds clean (Release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * review(codex): tear down editors from LoadPreset (main thread), not the worker Codex flagged a genuine dilemma in the previous approach: closing editor windows from LoadPresetWorker::Execute (a libuv worker) is unsafe either way on macOS — callAsync+wait stalls (no message-thread pump; the "libuv timer" the comment promises was never implemented) AND clearing inline destroys JUCE GUI objects off the message thread. Resolve it by not tearing down from the worker at all: LoadPreset() (the N-API entry, on the Node/main thread) now closes editors before queuing the AsyncWorker. That is safe on every platform — macOS: main thread IS the message thread (inline); Linux/Windows: post to the dedicated JUCE message thread and wait — and still guarantees editors die before Execute() frees the chain's processors. closeAllPluginEditorWindows() is consequently never called off a worker thread, so its macOS special-case is gone and it reduces to the uniform on-message-thread / post-and-wait form. Native addon builds clean (Release). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-08-11 03:09:56 +00:00 · 2026-07-01 15:05:54 +02:00
parent 3eefa3646a
commit 850d0926c7
1 changed files with 96 additions and 0 deletions
@@ -170,6 +170,13 @@ static void dispatchOnMessageThread(Func&& func)
 #endif
 }

+// Destroys every in-process plugin editor window. MUST be called on the message
+// thread (editorWindows holds JUCE GUI objects). Defined far below, after the
+// editorWindows map; forward-declared here so doShutdown — which already runs on
+// the message thread — can tear editors down before engine.reset() frees the
+// processors those editors point at (use-after-free; feedBack-desktop#56).
+static void destroyAllPluginEditorWindowsOnMessageThread();
+
 // ── Lifecycle ─────────────────────────────────────────────────────────────────

 static Napi::Value Init(const Napi::CallbackInfo& info)
@@ -243,6 +250,12 @@ static void doShutdown()
    if (juceRunning.load() || snapshotEngine() || snapshotVstHost())
    {
        dispatchOnMessageThread([]() {
+            // Editors reference their slot's processor; engine.reset() below
+            // frees the whole chain, so destroy the editor windows first (#56).
+            // Already on the message thread here — call the inline variant
+            // directly (closeAllPluginEditorWindows() would reach the same code
+            // via its message-thread branch; this just skips the thread check).
+            destroyAllPluginEditorWindowsOnMessageThread();
            if (auto liveEngine = snapshotEngine())
                liveEngine->stopAudio();
            {
@@ -2496,8 +2509,19 @@ static Napi::Value SetBypass(const Napi::CallbackInfo& info)
    return info.Env().Undefined();
 }

+// Destroy every open in-process plugin editor window on the message thread and
+// block until done. MUST run before any path that frees slot processors
+// (ClearChain, LoadPreset's chain rebuild, engine teardown): an editor window
+// owns an AudioProcessorEditor bound to its slot's processor, so if the
+// processor is freed first the editor's next timer/paint callback dereferences
+// freed memory (use-after-free → DEP-execute crash seconds after pause;
+// feedBack-desktop#56). Defined below, after the editorWindows map.
+static void closeAllPluginEditorWindows();
+
 static Napi::Value ClearChain(const Napi::CallbackInfo& info)
 {
+    // Tear editors down before their processors are freed just below (#56).
+    closeAllPluginEditorWindows();
    if (auto liveEngine = snapshotEngine()) liveEngine->getSignalChain().clear();
    return info.Env().Undefined();
 }
@@ -2607,6 +2631,62 @@ public:
    }
 };

+// Inline teardown: destroys every editor window. Caller MUST already be on the
+// message thread (editorWindows holds JUCE GUI objects). Forward-declared near
+// Init for doShutdown's use.
+static void destroyAllPluginEditorWindowsOnMessageThread()
+{
+    // Fails fast in assertion-enabled builds if a caller violates the
+    // precondition. Compiled out here under -DJUCE_DISABLE_ASSERTIONS, so it is
+    // documentation + a debug-build tripwire, never runtime cost.
+    JUCE_ASSERT_MESSAGE_THREAD
+    editorWindows.clear();
+}
+
+// See the forward declaration above ClearChain for why this exists. Tears down
+// the in-process editor windows so they are destroyed before the caller frees
+// the processors those editors point at. Clearing an empty map is cheap, so
+// calling this on every teardown is fine even when no editor is open.
+//
+// IMPORTANT: every caller runs on a MAIN-thread / message-thread context —
+// ClearChain and LoadPreset are N-API calls on the Node thread, doShutdown uses
+// the inline variant directly. This is NOT called from a libuv worker (that is
+// why LoadPreset closes editors before queuing LoadPresetWorker, rather than
+// letting the worker do it). Given that:
+//   - Already on the message thread (doShutdown; ClearChain / LoadPreset on
+//     macOS, where Node's main thread IS the JUCE message thread) → tear down
+//     inline; posting-and-waiting on ourselves would deadlock.
+//   - Otherwise (ClearChain / LoadPreset on Linux/Windows, where the JUCE
+//     message thread is a dedicated std::thread) → post to that thread and block
+//     until the editors are gone. Its 50ms dispatch loop drains this promptly,
+//     so there is no macOS-style stall here. Report a refused post / wait
+//     timeout so a lingering-editor UAF stays diagnosable.
+static void closeAllPluginEditorWindows()
+{
+    auto* mm = juce::MessageManager::getInstanceWithoutCreating();
+    if (mm != nullptr && mm->isThisTheMessageThread())
+    {
+        destroyAllPluginEditorWindowsOnMessageThread();
+        return;
+    }
+
+    auto done = std::make_shared<juce::WaitableEvent>();
+    const bool posted = juce::MessageManager::callAsync([done]()
+    {
+        destroyAllPluginEditorWindowsOnMessageThread();
+        done->signal();
+    });
+    if (!posted)
+    {
+        fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: message queue refused the post; "
+                        "editors may briefly outlive their processors\n");
+        return;
+    }
+    if (!done->wait(15000))
+        fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: editor teardown did not complete "
+                        "within 15s; proceeding\n");
+}
+
 static Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
 {
    auto env = info.Env();
@@ -2963,6 +3043,13 @@ public:
        auto* chainArray = chainVar.getArray();
        if (!chainArray) { success_ = false; error_ = "No chain array"; return; }

+        // NB: any open in-process editor windows were already torn down on the
+        // message thread by LoadPreset() before this AsyncWorker was queued (see
+        // there) — so clearing the chain here can't leave an editor pointing at
+        // a freed processor (use-after-free; #56). We deliberately do NOT tear
+        // editors down from this worker thread: JUCE GUI objects must only be
+        // destroyed on the message thread, and macOS has no pump to marshal to
+        // from here.
        // Clear existing chain
        liveEngine->getSignalChain().clear();

@@ -3090,6 +3177,15 @@ static Napi::Value LoadPreset(const Napi::CallbackInfo& info)
        return deferred.Promise();
    }

+    // Tear down any open in-process editor windows NOW, on the N-API/main
+    // thread, before the AsyncWorker frees the chain's processors on a libuv
+    // worker (#56). Doing it here — not inside LoadPresetWorker::Execute — keeps
+    // JUCE GUI teardown off the worker thread: on macOS this thread IS the
+    // message thread (inline teardown); on Linux/Windows closeAllPluginEditor-
+    // Windows() posts to the dedicated JUCE message thread and blocks. Either
+    // way editors are destroyed before Execute() clears the chain.
+    closeAllPluginEditorWindows();
+
    auto json = info[0].As<Napi::String>().Utf8Value();
    auto worker = new LoadPresetWorker(env, deferred, json);
    worker->Queue();