mirror of
https://github.com/got-feedBack/feedBack-desktop.git
synced 2026-09-10 22:14:10 +00:00
fix(audio): address PR #107 review — close serializer gaps, editor lifetime races, dispatch failures
All 8 CodeRabbit findings verified against the code and fixed: - ChainOps: macOS LoadVST routes its addProcessor through chainMutationMutex (macOS is a first-class platform; deadlock-safe — a worker holding the mutex never waits on the Node/main thread there). All four single-slot workers (LoadVST/NAM/IR/ReplaceIR) now bump chainGeneration so the executor's foreign-write detection sees direct loads, not just presets. - Rebuild barrier (beginChainRebuild/endChainRebuild): LoadPreset and ClearChain arm it before editor teardown; OpenPluginEditor refuses to open while a teardown+clear/rebuild is pending (#56 window between closeAllPluginEditorWindows returning and the worker taking the mutex). - EditorWindows: all slot/processor resolution in editor lambdas runs under a try_lock of chainMutationMutex (try_lock, never blocking — workers holding the mutex block-wait on the message thread). Sandbox promotion bumps chainGeneration. editorWindows map is now message-thread-only (duplicate-window check and close-erase moved into the queued lambdas). Null slot->processor recheck after a faulted promotion capture. - closeAllPluginEditorWindows returns false on refused post / 15s timeout; ClearChain skips the clear and LoadPreset resolves {success:false} instead of freeing processors under a live editor. - AddonContext: dispatchOnMessageThread reports refused-post/timeout; doShutdown leaves the message thread running when teardown didn't complete instead of unloading mid-destruction. - RendererBus::push rejects NaN/Inf/non-positive rates and a step that underflows to zero; new testRejectsUnusableRates unit case. Verified: addon builds clean, all 78 JS tests pass (storm, contracts, executor, N-API fuzz), all 5 engine_units native tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
d887c68014
commit
ea8c6a9ccd
@@ -81,7 +81,7 @@ static void stopJuceMessageThread()
|
||||
#endif
|
||||
}
|
||||
|
||||
void dispatchOnMessageThreadImpl(std::function<void()> func)
|
||||
bool dispatchOnMessageThreadImpl(std::function<void()> func)
|
||||
{
|
||||
#if JUCE_MAC
|
||||
// No background message thread on macOS — execute inline on caller thread.
|
||||
@@ -89,17 +89,38 @@ void dispatchOnMessageThreadImpl(std::function<void()> func)
|
||||
// instantiation (which genuinely requires a message thread on macOS) is
|
||||
// the one capability we give up until a proper libuv-based pump lands.
|
||||
func();
|
||||
return true;
|
||||
#else
|
||||
// Heap-allocate the WaitableEvent and capture by value so the queued
|
||||
// callAsync closure can outlive this stack frame. Without this, a 15 s
|
||||
// timeout (rare, but possible during shutdown when the message thread is
|
||||
// busy) leaves the lambda running on freed `done` storage — a real UAF.
|
||||
//
|
||||
// Both failure modes are reported to the caller: a refused post means
|
||||
// `func` will NEVER run (message queue already gone); a wait timeout
|
||||
// means it hasn't run YET (it may still run later while the dispatch
|
||||
// loop drains). Lifecycle callers must not proceed as if the work
|
||||
// completed — doShutdown in particular used to unload the addon while
|
||||
// editor teardown / stopAudio / engine destruction were still pending.
|
||||
auto done = std::make_shared<juce::WaitableEvent>();
|
||||
juce::MessageManager::callAsync([func = std::move(func), done]() mutable {
|
||||
func();
|
||||
done->signal();
|
||||
});
|
||||
done->wait(15000);
|
||||
const bool posted = juce::MessageManager::callAsync(
|
||||
[func = std::move(func), done]() mutable {
|
||||
func();
|
||||
done->signal();
|
||||
});
|
||||
if (!posted)
|
||||
{
|
||||
fprintf(stderr, "[audio-native] dispatchOnMessageThread: message queue "
|
||||
"refused the post; dispatched work will not run\n");
|
||||
return false;
|
||||
}
|
||||
if (!done->wait(15000))
|
||||
{
|
||||
fprintf(stderr, "[audio-native] dispatchOnMessageThread: dispatched work "
|
||||
"did not complete within 15s\n");
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
#endif
|
||||
}
|
||||
|
||||
@@ -152,7 +173,7 @@ void initialize(std::function<void()> uiTeardownHook)
|
||||
#endif
|
||||
|
||||
// Create engine on the JUCE message thread (or inline on macOS)
|
||||
dispatchOnMessageThread([]() {
|
||||
const bool initialized = dispatchOnMessageThread([]() {
|
||||
std::shared_ptr<AudioEngine> liveEngine;
|
||||
{
|
||||
std::lock_guard<std::mutex> lock(engineMutex);
|
||||
@@ -172,6 +193,9 @@ void initialize(std::function<void()> uiTeardownHook)
|
||||
types[i].inputDevices.size(),
|
||||
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");
|
||||
}
|
||||
|
||||
void doShutdown()
|
||||
@@ -201,7 +225,7 @@ void doShutdown()
|
||||
|
||||
if (juceRunning.load() || snapshotEngine() || snapshotVstHost())
|
||||
{
|
||||
dispatchOnMessageThread([]() {
|
||||
const bool toreDown = dispatchOnMessageThread([]() {
|
||||
// Editors reference their slot's processor; engine.reset() below
|
||||
// frees the whole chain, so destroy the editor windows first (#56).
|
||||
if (shutdownUiTeardown) shutdownUiTeardown();
|
||||
@@ -216,6 +240,20 @@ void doShutdown()
|
||||
vstHost.reset();
|
||||
}
|
||||
});
|
||||
if (!toreDown)
|
||||
{
|
||||
// Editor teardown / stopAudio / engine destruction have NOT
|
||||
// completed. Do not stop the message thread underneath them: a
|
||||
// timed-out teardown lambda is still queued and can only finish
|
||||
// if the pump keeps running. Leaking the pump thread at process
|
||||
// exit beats unloading the addon mid-destruction (the exact
|
||||
// shutdown UAF this path exists to prevent). The latch stays
|
||||
// set, so a re-entrant shutdown call no-ops.
|
||||
fprintf(stderr, "[audio-native] doShutdown: engine teardown did not "
|
||||
"complete; leaving message thread running\n");
|
||||
slopsmith::sandbox::uninstallVstCrashAttribution();
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
stopJuceMessageThread();
|
||||
|
||||
@@ -43,11 +43,15 @@ void doShutdown();
|
||||
// Dispatch `func` on the JUCE message thread and wait (bounded 15 s).
|
||||
// macOS: executes inline on the caller thread — no background pump exists
|
||||
// (AppKit owns the real main thread; see the fork note in the .cpp).
|
||||
void dispatchOnMessageThreadImpl(std::function<void()> func);
|
||||
// Returns false when the work did not complete: the post was refused
|
||||
// (message queue gone — `func` will never run) or the wait timed out
|
||||
// (`func` may still run later). Lifecycle callers must treat false as
|
||||
// "teardown/init did not happen" rather than continuing.
|
||||
bool dispatchOnMessageThreadImpl(std::function<void()> func);
|
||||
template <typename Func>
|
||||
inline void dispatchOnMessageThread(Func&& func)
|
||||
inline bool dispatchOnMessageThread(Func&& func)
|
||||
{
|
||||
dispatchOnMessageThreadImpl(std::function<void()>(std::forward<Func>(func)));
|
||||
return dispatchOnMessageThreadImpl(std::function<void()>(std::forward<Func>(func)));
|
||||
}
|
||||
|
||||
// Pending-async-load registry: LoadVSTWorker / LoadPresetWorker block on a
|
||||
|
||||
@@ -74,8 +74,25 @@ Napi::Value SetBypass(const Napi::CallbackInfo& info)
|
||||
|
||||
Napi::Value ClearChain(const Napi::CallbackInfo& info)
|
||||
{
|
||||
// Gate editor opens for the whole teardown+clear window (see the rebuild
|
||||
// barrier in ChainOps.h): without it, an editor opened between the
|
||||
// teardown below and the clear acquiring the mutex would point at a
|
||||
// processor the clear is about to free.
|
||||
slopsmith::addon::beginChainRebuild();
|
||||
struct BarrierRelease {
|
||||
~BarrierRelease() { slopsmith::addon::endChainRebuild(); }
|
||||
} barrierRelease;
|
||||
|
||||
// Tear editors down before their processors are freed just below (#56).
|
||||
closeAllPluginEditorWindows();
|
||||
if (!closeAllPluginEditorWindows())
|
||||
{
|
||||
// Teardown refused/timed out: an editor may still be bound to a chain
|
||||
// processor. Clearing now would free it under the live editor — the
|
||||
// documented UAF. Skip the clear; the caller can retry.
|
||||
fprintf(stderr, "[audio-native] clearChain: editor teardown did not complete; "
|
||||
"chain left untouched\n");
|
||||
return info.Env().Undefined();
|
||||
}
|
||||
if (auto liveEngine = snapshotEngine())
|
||||
{
|
||||
// Serialized with the async chain workers (deep-read 1). May block
|
||||
|
||||
@@ -44,6 +44,25 @@ uint64_t currentChainGeneration()
|
||||
return chainGeneration.load(std::memory_order_acquire);
|
||||
}
|
||||
|
||||
// ── Rebuild barrier (see ChainOps.h) ────────────────────────────────────────
|
||||
|
||||
static std::atomic<int> chainRebuildsPending{0};
|
||||
|
||||
void beginChainRebuild()
|
||||
{
|
||||
chainRebuildsPending.fetch_add(1, std::memory_order_acq_rel);
|
||||
}
|
||||
|
||||
void endChainRebuild()
|
||||
{
|
||||
chainRebuildsPending.fetch_sub(1, std::memory_order_acq_rel);
|
||||
}
|
||||
|
||||
bool isChainRebuildPending()
|
||||
{
|
||||
return chainRebuildsPending.load(std::memory_order_acquire) > 0;
|
||||
}
|
||||
|
||||
// ── decodeStateBlob (moved verbatim) ────────────────────
|
||||
|
||||
// Decode a state blob that may be in EITHER base64 flavour. JUCE's
|
||||
@@ -398,6 +417,8 @@ public:
|
||||
ProcessorSlot::Type::VST,
|
||||
name,
|
||||
path);
|
||||
if (slotId_ >= 0)
|
||||
slopsmith::addon::bumpChainGeneration(); // still under chainLock
|
||||
}
|
||||
|
||||
void OnOK() override
|
||||
@@ -476,11 +497,22 @@ Napi::Value LoadVST(const Napi::CallbackInfo& info)
|
||||
if (processor)
|
||||
{
|
||||
auto name = processor->getName();
|
||||
// Serialize with the async chain workers (deep-read 1): an unguarded
|
||||
// addProcessor here could land a slot inside a LoadPresetWorker's
|
||||
// clear()+rebuild running on a libuv thread. Deadlock-safe on macOS:
|
||||
// a worker holding this mutex never waits on THIS (Node/main) thread —
|
||||
// loadVstSandboxAware's JUCE_MAC branch is a synchronous load on the
|
||||
// worker itself, and dispatchOnMessageThread runs inline there. Only
|
||||
// the mutation is guarded; the slow plugin load above stays outside
|
||||
// the lock.
|
||||
std::lock_guard<std::mutex> chainLock(slopsmith::addon::chainMutationMutex());
|
||||
slotId = liveEngine->getSignalChain().addProcessor(
|
||||
std::move(processor),
|
||||
ProcessorSlot::Type::VST,
|
||||
name,
|
||||
juce::String(pluginPath));
|
||||
if (slotId >= 0)
|
||||
slopsmith::addon::bumpChainGeneration(); // still under chainLock
|
||||
}
|
||||
else
|
||||
{
|
||||
@@ -518,6 +550,8 @@ public:
|
||||
ProcessorSlot::Type::NAM,
|
||||
"NAM: " + name,
|
||||
juce::String(modelPath_));
|
||||
if (slotId_ >= 0)
|
||||
slopsmith::addon::bumpChainGeneration(); // still under chainLock
|
||||
}
|
||||
}
|
||||
|
||||
@@ -573,6 +607,8 @@ public:
|
||||
ProcessorSlot::Type::IR,
|
||||
"IR: " + name,
|
||||
juce::String(irPath_));
|
||||
if (slotId_ >= 0)
|
||||
slopsmith::addon::bumpChainGeneration(); // still under chainLock
|
||||
}
|
||||
}
|
||||
|
||||
@@ -635,6 +671,8 @@ public:
|
||||
"IR: " + name, juce::String(irPath_));
|
||||
if (ok_ && gain_ >= 0.0f)
|
||||
liveEngine->getSignalChain().setPostGain(slotId_, gain_);
|
||||
if (ok_)
|
||||
slopsmith::addon::bumpChainGeneration(); // still under chainLock
|
||||
}
|
||||
|
||||
void OnOK() override { deferred_.Resolve(Napi::Boolean::New(Env(), ok_)); }
|
||||
@@ -680,6 +718,13 @@ public:
|
||||
|
||||
void Execute() override
|
||||
{
|
||||
// Release the rebuild barrier LoadPreset() armed before editor
|
||||
// teardown, on every exit path — editors may open again once the
|
||||
// rebuild below has completed (or bailed).
|
||||
struct BarrierRelease {
|
||||
~BarrierRelease() { slopsmith::addon::endChainRebuild(); }
|
||||
} barrierRelease;
|
||||
|
||||
// Serialize the FULL mutation (TLC deep-read 1): overlapping chain
|
||||
// workers on the libuv pool must not interleave clear()/addProcessor().
|
||||
std::lock_guard<std::mutex> chainLock(slopsmith::addon::chainMutationMutex());
|
||||
@@ -841,6 +886,14 @@ Napi::Value LoadPreset(const Napi::CallbackInfo& info)
|
||||
return deferred.Promise();
|
||||
}
|
||||
|
||||
// Arm the rebuild barrier BEFORE editor teardown: between closeAll…()
|
||||
// returning and the queued worker acquiring chainMutationMutex, nothing
|
||||
// else stops OpenPluginEditor from opening a fresh editor whose processor
|
||||
// the worker is about to free (#56). The barrier gates editor opens for
|
||||
// the whole teardown+rebuild window; the worker releases it on every
|
||||
// Execute() exit path.
|
||||
slopsmith::addon::beginChainRebuild();
|
||||
|
||||
// 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
|
||||
@@ -848,7 +901,19 @@ Napi::Value LoadPreset(const Napi::CallbackInfo& info)
|
||||
// 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();
|
||||
if (!closeAllPluginEditorWindows())
|
||||
{
|
||||
// Teardown refused or timed out: an editor may still be alive and
|
||||
// bound to a chain processor. Clearing/rebuilding now would free that
|
||||
// processor under the live editor — the documented UAF. Abort the
|
||||
// load instead of proceeding.
|
||||
slopsmith::addon::endChainRebuild();
|
||||
auto obj = Napi::Object::New(env);
|
||||
obj.Set("success", false);
|
||||
obj.Set("error", "editor teardown did not complete; preset load aborted");
|
||||
deferred.Resolve(obj);
|
||||
return deferred.Promise();
|
||||
}
|
||||
|
||||
auto json = info[0].As<Napi::String>().Utf8Value();
|
||||
auto worker = new LoadPresetWorker(env, deferred, json);
|
||||
|
||||
@@ -49,6 +49,19 @@ uint64_t currentChainGeneration();
|
||||
// const uint64_t gen = bumpChainGeneration(); // still under the lock
|
||||
// (return gen in the result object)
|
||||
|
||||
// ── Rebuild barrier (editor-open gate) ────────────────────────────────────
|
||||
// A chain clear/rebuild is a two-step dance: editors are torn down on the
|
||||
// message thread FIRST, then the mutation runs (synchronously for ClearChain,
|
||||
// on a queued AsyncWorker for LoadPreset). Between those steps the mutation
|
||||
// mutex is NOT yet held, so an editor opened in that window would point at a
|
||||
// processor the imminent clear is about to free (#56). Callers bracket the
|
||||
// whole teardown+mutation with begin/end; OpenPluginEditor refuses to open
|
||||
// while any rebuild is pending. Counter (not bool): overlapping LoadPreset +
|
||||
// ClearChain must not un-gate each other early.
|
||||
void beginChainRebuild();
|
||||
void endChainRebuild();
|
||||
bool isChainRebuildPending();
|
||||
|
||||
// ── Shared load helpers (used by the workers here and SetSlotState) ──────
|
||||
// Decode a state blob in EITHER base64 flavour (JUCE-proprietary first,
|
||||
// standard RFC-4648 fallback when `allowStandard` — IR/NAM slots only).
|
||||
|
||||
@@ -8,12 +8,14 @@
|
||||
|
||||
#include "AddonContext.h"
|
||||
#include "NapiHelpers.h"
|
||||
#include "ChainOps.h"
|
||||
#include "../Sandbox/SandboxedProcessor.h"
|
||||
#include "../Sandbox/CrashAttribution.h"
|
||||
|
||||
#include <cstdio>
|
||||
#include <map>
|
||||
#include <memory>
|
||||
#include <mutex>
|
||||
|
||||
namespace slopsmith::addon {
|
||||
|
||||
@@ -82,13 +84,13 @@ void destroyAllPluginEditorWindowsOnMessageThread()
|
||||
// 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.
|
||||
void closeAllPluginEditorWindows()
|
||||
bool closeAllPluginEditorWindows()
|
||||
{
|
||||
auto* mm = juce::MessageManager::getInstanceWithoutCreating();
|
||||
if (mm != nullptr && mm->isThisTheMessageThread())
|
||||
{
|
||||
destroyAllPluginEditorWindowsOnMessageThread();
|
||||
return;
|
||||
return true;
|
||||
}
|
||||
|
||||
auto done = std::make_shared<juce::WaitableEvent>();
|
||||
@@ -100,12 +102,19 @@ void closeAllPluginEditorWindows()
|
||||
if (!posted)
|
||||
{
|
||||
fprintf(stderr, "[audio-native] closeAllPluginEditorWindows: message queue refused the post; "
|
||||
"editors may briefly outlive their processors\n");
|
||||
return;
|
||||
"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; proceeding\n");
|
||||
"within 15s; caller must not free chain processors\n");
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
|
||||
@@ -117,6 +126,22 @@ Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
|
||||
return Napi::Boolean::New(env, false);
|
||||
const int slotId = *slotIdOpt;
|
||||
|
||||
// Rebuild barrier (ChainOps.h): a chain clear/rebuild is between its
|
||||
// editor teardown and the mutation itself — the processor this editor
|
||||
// would bind to is about to be freed (#56). Refuse to open.
|
||||
if (slopsmith::addon::isChainRebuildPending())
|
||||
return Napi::Boolean::New(env, false);
|
||||
|
||||
// Resolve the slot under the chain-mutation mutex: getSlot returns a raw
|
||||
// pointer a concurrent worker's clear()/rebuild would free under us.
|
||||
// try_lock, never a blocking lock — a preset load can hold the mutex for
|
||||
// seconds (VST init) and this is V8's thread; if a mutation is in flight
|
||||
// the slot we'd open is about to be replaced anyway.
|
||||
std::unique_lock<std::mutex> chainLock(
|
||||
slopsmith::addon::chainMutationMutex(), std::try_to_lock);
|
||||
if (!chainLock.owns_lock())
|
||||
return Napi::Boolean::New(env, false);
|
||||
|
||||
auto slot = liveEngine->getSignalChain().getSlot(slotId);
|
||||
if (!slot || !slot->processor || !slot->processor->hasEditor())
|
||||
return Napi::Boolean::New(env, false);
|
||||
@@ -154,10 +179,22 @@ Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
|
||||
// crash between then and now is possible — re-check here.
|
||||
if (!sb->isAlive())
|
||||
return Napi::Boolean::New(env, false);
|
||||
// Validation is done — release before queueing, so the lambda's own
|
||||
// try_lock on the message thread can't collide with THIS thread still
|
||||
// holding the mutex and drop the open as a false conflict.
|
||||
chainLock.unlock();
|
||||
const bool queued = juce::MessageManager::callAsync([slotId]()
|
||||
{
|
||||
auto liveEngine = snapshotEngine();
|
||||
if (!liveEngine) return;
|
||||
// try_lock, NEVER a blocking lock on the message thread: chain
|
||||
// workers holding the mutex block-wait on this very thread
|
||||
// (loadVstSandboxAware's callAsync+wait) — blocking here would
|
||||
// deadlock. Contention means a mutation is rebuilding the slot;
|
||||
// skip the open.
|
||||
std::unique_lock<std::mutex> chainLock(
|
||||
slopsmith::addon::chainMutationMutex(), std::try_to_lock);
|
||||
if (!chainLock.owns_lock()) return;
|
||||
if (auto* slot = liveEngine->getSignalChain().getSlot(slotId))
|
||||
if (auto* sb = dynamic_cast<slopsmith::sandbox::SandboxedProcessor*>(slot->processor.get()))
|
||||
sb->requestOpenEditor();
|
||||
@@ -173,30 +210,46 @@ Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
|
||||
}
|
||||
#endif
|
||||
|
||||
// In-process plugin — host-side PluginEditorWindow flow. If a window
|
||||
// already exists for this slot, bring it to front rather than creating
|
||||
// a duplicate.
|
||||
auto it = editorWindows.find(slotId);
|
||||
if (it != editorWindows.end() && it->second)
|
||||
{
|
||||
if (it->second->isVisible())
|
||||
{
|
||||
it->second->toFront(true);
|
||||
return Napi::Boolean::New(env, true);
|
||||
}
|
||||
// Window was hidden/closed, remove stale entry
|
||||
editorWindows.erase(it);
|
||||
}
|
||||
|
||||
// Create editor on the message thread. Capture slotId only — re-resolve
|
||||
// the slot via snapshotEngine() + getSlot(slotId) inside the lambda so a
|
||||
// SignalChain::removeProcessor() between this call returning and the
|
||||
// async firing can't leave us calling createEditorAndMakeActive() on a
|
||||
// dangling juce::AudioProcessor*. Mirrors the sandbox branch's pattern.
|
||||
// In-process plugin — host-side PluginEditorWindow flow. Everything —
|
||||
// including the duplicate-window check — runs on the message thread:
|
||||
// editorWindows is a plain std::map owned by that thread, and reading or
|
||||
// erasing it from this (N-API) thread raced the message-thread inserts/
|
||||
// erases.
|
||||
//
|
||||
// Capture slotId only — re-resolve the slot via snapshotEngine() +
|
||||
// getSlot(slotId) inside the lambda so a SignalChain::removeProcessor()
|
||||
// between this call returning and the async firing can't leave us calling
|
||||
// createEditorAndMakeActive() on a dangling juce::AudioProcessor*.
|
||||
//
|
||||
// Validation is done — release before queueing, so the lambda's own
|
||||
// try_lock on the message thread can't collide with THIS thread still
|
||||
// holding the mutex and drop the open as a false conflict.
|
||||
chainLock.unlock();
|
||||
const bool queued = juce::MessageManager::callAsync([slotId]()
|
||||
{
|
||||
// If a window already exists for this slot, bring it to front rather
|
||||
// than creating a duplicate.
|
||||
auto it = editorWindows.find(slotId);
|
||||
if (it != editorWindows.end() && it->second)
|
||||
{
|
||||
if (it->second->isVisible())
|
||||
{
|
||||
it->second->toFront(true);
|
||||
return;
|
||||
}
|
||||
// Window was hidden/closed, remove stale entry
|
||||
editorWindows.erase(it);
|
||||
}
|
||||
|
||||
auto liveEngine = snapshotEngine();
|
||||
if (!liveEngine) return;
|
||||
// try_lock, NEVER a blocking lock on the message thread: chain
|
||||
// workers holding the mutex block-wait on this very thread
|
||||
// (loadVstSandboxAware's callAsync+wait) — blocking here would
|
||||
// deadlock. Contention means the slot is being rebuilt; skip.
|
||||
std::unique_lock<std::mutex> chainLock(
|
||||
slopsmith::addon::chainMutationMutex(), std::try_to_lock);
|
||||
if (!chainLock.owns_lock()) return;
|
||||
auto& chain = liveEngine->getSignalChain();
|
||||
auto* slot = chain.getSlot(slotId);
|
||||
if (!slot || !slot->processor) return;
|
||||
@@ -259,6 +312,11 @@ Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
|
||||
if (chain.replaceProcessor(slotId, std::move(sandboxed)))
|
||||
{
|
||||
promoted = true;
|
||||
// A promotion swaps the slot's processor: bump the
|
||||
// generation (we hold chainMutationMutex via the
|
||||
// try_lock above) so JS-side chain owners re-sync
|
||||
// instead of driving the replaced slot blind.
|
||||
slopsmith::addon::bumpChainGeneration();
|
||||
bool editorOpened = false;
|
||||
if (auto* slot2 = chain.getSlot(slotId))
|
||||
if (auto* sb = dynamic_cast<slopsmith::sandbox::SandboxedProcessor*>(slot2->processor.get()))
|
||||
@@ -296,7 +354,14 @@ Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info)
|
||||
|
||||
// In-process editor: non-VST3, editor-less, already-sandboxed, or POSIX
|
||||
// (where the in-process editor is safe).
|
||||
//
|
||||
// Re-check the processor: the promotion branch above documents that a
|
||||
// faulted captureVstStateForPromotion() can RELEASE the slot's
|
||||
// processor before returning false — falling through here with a null
|
||||
// processor would crash on createEditorAndMakeActive().
|
||||
auto* processor = slot->processor.get();
|
||||
if (processor == nullptr)
|
||||
return;
|
||||
auto name = slot->name;
|
||||
juce::AudioProcessorEditor* editor = nullptr;
|
||||
try {
|
||||
@@ -324,53 +389,38 @@ Napi::Value ClosePluginEditor(const Napi::CallbackInfo& info)
|
||||
if (!slotIdOpt) return Napi::Boolean::New(env, false);
|
||||
const int slotId = *slotIdOpt;
|
||||
|
||||
// Sandboxed plugins: route the close to the sandbox child via IPC.
|
||||
// No host-side PluginEditorWindow exists for these.
|
||||
//
|
||||
// Same shape as the open path: dispatch off the N-API thread and
|
||||
// re-resolve the slot inside the lambda. requestCloseEditor()
|
||||
// ultimately writes to the control pipe (writeFrame can block up
|
||||
// to ~5s on a stalled reader), so running it synchronously here
|
||||
// would freeze JS / the renderer UI on a slow sandbox; the
|
||||
// re-resolve guards against slot-removal UAF between the napi call
|
||||
// and the async firing.
|
||||
//
|
||||
// All desktop platforms: route the close to the sandbox child via IPC
|
||||
// (SandboxedProcessor is compiled everywhere now). In-process plugins fall
|
||||
// through to the host-side editor-window teardown below.
|
||||
#if defined(SLOPSMITH_AUDIO_ADDON)
|
||||
if (auto liveEngine = snapshotEngine())
|
||||
// One queued lambda handles both the sandbox and in-process paths, for
|
||||
// two reasons:
|
||||
// - editorWindows is message-thread-owned; the old synchronous
|
||||
// find() here raced the message-thread inserts/erases.
|
||||
// - getSlot() from this (N-API) thread dereferenced a slot a chain
|
||||
// worker could free mid-call; the slot is now resolved inside the
|
||||
// lambda under a try_lock on the chain-mutation mutex.
|
||||
// requestCloseEditor() ultimately writes to the control pipe (writeFrame
|
||||
// can block up to ~5s on a stalled reader), so dispatching also keeps a
|
||||
// slow sandbox from freezing JS / the renderer UI.
|
||||
const bool queued = juce::MessageManager::callAsync([slotId]()
|
||||
{
|
||||
if (auto* slot = liveEngine->getSignalChain().getSlot(slotId))
|
||||
{
|
||||
if (slot->processor
|
||||
&& dynamic_cast<slopsmith::sandbox::SandboxedProcessor*>(slot->processor.get()))
|
||||
{
|
||||
const bool queued = juce::MessageManager::callAsync([slotId]()
|
||||
{
|
||||
auto liveEngine = snapshotEngine();
|
||||
if (!liveEngine) return;
|
||||
if (auto* slot = liveEngine->getSignalChain().getSlot(slotId))
|
||||
if (auto* sb = dynamic_cast<slopsmith::sandbox::SandboxedProcessor*>(slot->processor.get()))
|
||||
sb->requestCloseEditor();
|
||||
});
|
||||
return Napi::Boolean::New(env, queued);
|
||||
}
|
||||
}
|
||||
}
|
||||
#endif
|
||||
// Host-side window (in-process plugins). Erasing a missing key is a
|
||||
// no-op; sandbox slots never have an entry here.
|
||||
editorWindows.erase(slotId);
|
||||
|
||||
// In-process plugin — tear down the host-side editor window.
|
||||
auto it = editorWindows.find(slotId);
|
||||
if (it != editorWindows.end())
|
||||
{
|
||||
juce::MessageManager::callAsync([slotId]()
|
||||
{
|
||||
editorWindows.erase(slotId);
|
||||
});
|
||||
return Napi::Boolean::New(env, true);
|
||||
}
|
||||
return Napi::Boolean::New(env, false);
|
||||
#if defined(SLOPSMITH_AUDIO_ADDON)
|
||||
auto liveEngine = snapshotEngine();
|
||||
if (!liveEngine) return;
|
||||
// try_lock, NEVER a blocking lock on the message thread: chain
|
||||
// workers holding the mutex block-wait on this very thread —
|
||||
// blocking here would deadlock. Contention means the chain is being
|
||||
// rebuilt, which tears editors down anyway.
|
||||
std::unique_lock<std::mutex> chainLock(
|
||||
slopsmith::addon::chainMutationMutex(), std::try_to_lock);
|
||||
if (!chainLock.owns_lock()) return;
|
||||
if (auto* slot = liveEngine->getSignalChain().getSlot(slotId))
|
||||
if (auto* sb = dynamic_cast<slopsmith::sandbox::SandboxedProcessor*>(slot->processor.get()))
|
||||
sb->requestCloseEditor();
|
||||
#endif
|
||||
});
|
||||
return Napi::Boolean::New(env, queued);
|
||||
}
|
||||
|
||||
|
||||
|
||||
@@ -19,7 +19,10 @@ void destroyAllPluginEditorWindowsOnMessageThread();
|
||||
// caller frees the processors those editors point at. Safe from the Node
|
||||
// thread (posts to the message thread and blocks, bounded) or the message
|
||||
// thread itself (inline). Clearing an empty map is cheap.
|
||||
void closeAllPluginEditorWindows();
|
||||
// Returns false when teardown did NOT complete (post refused or the bounded
|
||||
// wait timed out) — the caller must not free chain processors in that case
|
||||
// (#56 use-after-free).
|
||||
bool closeAllPluginEditorWindows();
|
||||
|
||||
// N-API bindings (registered by NodeAddon's export table).
|
||||
Napi::Value OpenPluginEditor(const Napi::CallbackInfo& info);
|
||||
|
||||
@@ -62,8 +62,12 @@ public:
|
||||
{
|
||||
if (!busEnabled.load(std::memory_order_acquire)) return false;
|
||||
if (interleavedLR == nullptr || frames <= 0) return false;
|
||||
if (deviceRate <= 0.0) return false;
|
||||
if (!(sourceRate > 0.0)) sourceRate = deviceRate;
|
||||
// Both rates cross the JS/IPC boundary: reject NaN/Inf (a NaN
|
||||
// deviceRate passes a plain `<= 0.0` check) and a step that
|
||||
// underflowed to zero (subnormal source rate), either of which would
|
||||
// make the resample loop index garbage or never advance.
|
||||
if (!std::isfinite(deviceRate) || deviceRate <= 0.0) return false;
|
||||
if (!std::isfinite(sourceRate) || sourceRate <= 0.0) sourceRate = deviceRate;
|
||||
|
||||
uint64_t w = ring.beginWrite();
|
||||
|
||||
@@ -73,6 +77,7 @@ public:
|
||||
// interpolation is continuous across pushes. Equal rates degenerate
|
||||
// to step == 1.0 (still exact: pos stays integral, frac == 0).
|
||||
const double step = sourceRate / deviceRate;
|
||||
if (!std::isfinite(step) || step <= 0.0) return false;
|
||||
double pos = srcPos;
|
||||
uint64_t written = 0;
|
||||
while (true)
|
||||
|
||||
@@ -8,6 +8,7 @@
|
||||
#include <cassert>
|
||||
#include <cmath>
|
||||
#include <cstdio>
|
||||
#include <limits>
|
||||
#include <vector>
|
||||
|
||||
using slopsmith::RendererBus;
|
||||
@@ -165,9 +166,35 @@ static void testFlushOnDisable()
|
||||
assert(dl[1] == 7.0f && "post-re-enable audio must be the fresh push");
|
||||
}
|
||||
|
||||
// Rate validation (PR #107 review): non-finite rates cross the JS/IPC
|
||||
// boundary; NaN passes a plain `<= 0` check, and a subnormal source rate can
|
||||
// underflow step to 0 — both must be rejected before the resample loop.
|
||||
// A bad sourceRate falls back to deviceRate (documented behaviour).
|
||||
static void testRejectsUnusableRates()
|
||||
{
|
||||
RendererBus bus;
|
||||
bus.setEnabled(true, 1.0f);
|
||||
const auto chunk = rampChunk(128, 1.0f, 0.0f);
|
||||
const double nan = std::nan("");
|
||||
const double inf = std::numeric_limits<double>::infinity();
|
||||
assert(!bus.push(chunk.data(), 128, 48000.0, nan));
|
||||
assert(!bus.push(chunk.data(), 128, 48000.0, inf));
|
||||
assert(!bus.push(chunk.data(), 128, 48000.0, -48000.0));
|
||||
assert(!bus.push(chunk.data(), 128, 48000.0, 0.0));
|
||||
// step underflow: denormal source over huge device rate → step == 0.
|
||||
assert(!bus.push(chunk.data(), 128, 5e-324, 1e308));
|
||||
assert(bus.metrics().pushedFrames == 0 && "rejected pushes must stage nothing");
|
||||
// NaN/Inf/negative SOURCE rate falls back to deviceRate (step == 1).
|
||||
assert(bus.push(chunk.data(), 128, nan, 48000.0));
|
||||
assert(bus.push(chunk.data(), 128, inf, 48000.0));
|
||||
assert(bus.push(chunk.data(), 128, -1.0, 48000.0));
|
||||
assert(bus.metrics().pushedFrames > 0);
|
||||
}
|
||||
|
||||
int main()
|
||||
{
|
||||
testEqualRateBitExact();
|
||||
testRejectsUnusableRates();
|
||||
testResampleContinuityAcrossPushes();
|
||||
testPrimeGate();
|
||||
testUnderflowReprimes();
|
||||
|
||||
Reference in New Issue
Block a user