diff --git a/src/audio/addon/AddonContext.cpp b/src/audio/addon/AddonContext.cpp index b2050e5..79ee2a9 100644 --- a/src/audio/addon/AddonContext.cpp +++ b/src/audio/addon/AddonContext.cpp @@ -81,7 +81,7 @@ static void stopJuceMessageThread() #endif } -void dispatchOnMessageThreadImpl(std::function func) +bool dispatchOnMessageThreadImpl(std::function func) { #if JUCE_MAC // No background message thread on macOS — execute inline on caller thread. @@ -89,17 +89,38 @@ void dispatchOnMessageThreadImpl(std::function 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::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 uiTeardownHook) #endif // Create engine on the JUCE message thread (or inline on macOS) - dispatchOnMessageThread([]() { + const bool initialized = dispatchOnMessageThread([]() { std::shared_ptr liveEngine; { std::lock_guard lock(engineMutex); @@ -172,6 +193,9 @@ void initialize(std::function 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(); diff --git a/src/audio/addon/AddonContext.h b/src/audio/addon/AddonContext.h index 879da2a..2c2a1b9 100644 --- a/src/audio/addon/AddonContext.h +++ b/src/audio/addon/AddonContext.h @@ -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 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 func); template -inline void dispatchOnMessageThread(Func&& func) +inline bool dispatchOnMessageThread(Func&& func) { - dispatchOnMessageThreadImpl(std::function(std::forward(func))); + return dispatchOnMessageThreadImpl(std::function(std::forward(func))); } // Pending-async-load registry: LoadVSTWorker / LoadPresetWorker block on a diff --git a/src/audio/addon/ChainBindings.cpp b/src/audio/addon/ChainBindings.cpp index 07ec06b..f98df93 100644 --- a/src/audio/addon/ChainBindings.cpp +++ b/src/audio/addon/ChainBindings.cpp @@ -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 diff --git a/src/audio/addon/ChainOps.cpp b/src/audio/addon/ChainOps.cpp index 82ab181..36e8f1b 100644 --- a/src/audio/addon/ChainOps.cpp +++ b/src/audio/addon/ChainOps.cpp @@ -44,6 +44,25 @@ uint64_t currentChainGeneration() return chainGeneration.load(std::memory_order_acquire); } +// ── Rebuild barrier (see ChainOps.h) ──────────────────────────────────────── + +static std::atomic 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 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 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().Utf8Value(); auto worker = new LoadPresetWorker(env, deferred, json); diff --git a/src/audio/addon/ChainOps.h b/src/audio/addon/ChainOps.h index 757f7e0..4a7d240 100644 --- a/src/audio/addon/ChainOps.h +++ b/src/audio/addon/ChainOps.h @@ -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). diff --git a/src/audio/addon/EditorWindows.cpp b/src/audio/addon/EditorWindows.cpp index d74ac59..1beb886 100644 --- a/src/audio/addon/EditorWindows.cpp +++ b/src/audio/addon/EditorWindows.cpp @@ -8,12 +8,14 @@ #include "AddonContext.h" #include "NapiHelpers.h" +#include "ChainOps.h" #include "../Sandbox/SandboxedProcessor.h" #include "../Sandbox/CrashAttribution.h" #include #include #include +#include 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(); @@ -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 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 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(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 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(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(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(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 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(slot->processor.get())) + sb->requestCloseEditor(); +#endif + }); + return Napi::Boolean::New(env, queued); } diff --git a/src/audio/addon/EditorWindows.h b/src/audio/addon/EditorWindows.h index 3afe80b..45304a7 100644 --- a/src/audio/addon/EditorWindows.h +++ b/src/audio/addon/EditorWindows.h @@ -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); diff --git a/src/audio/engine/RendererBus.h b/src/audio/engine/RendererBus.h index 765b2e3..36f247e 100644 --- a/src/audio/engine/RendererBus.h +++ b/src/audio/engine/RendererBus.h @@ -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) diff --git a/tests/engine_units/renderer_bus_test.cpp b/tests/engine_units/renderer_bus_test.cpp index 6dcffe0..8e5fb8c 100644 --- a/tests/engine_units/renderer_bus_test.cpp +++ b/tests/engine_units/renderer_bus_test.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include 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::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();