diff --git a/src/core/dsp/WdspChannel.cpp b/src/core/dsp/WdspChannel.cpp index fc0c7c4f71..f0eb33c5e8 100644 --- a/src/core/dsp/WdspChannel.cpp +++ b/src/core/dsp/WdspChannel.cpp @@ -1273,6 +1273,12 @@ void WdspChannel::synchronizeWorkerForTest() const noexcept SynchronizeChannelWorkerForTest(m_channelId); } +void WdspChannel::restorePureSignalCorrectionForTest(const char* path) const noexcept +{ + // PSRestoreCorr only copies the name; WDSP's signature is just not const. + PSRestoreCorr(m_channelId, const_cast(path)); +} + std::unique_lock WdspChannel::fftwSetupLock() { // Forwards, and keeps its name so Hl2Spectrum, AnanPanAnalyzer and diff --git a/src/core/dsp/WdspChannel.h b/src/core/dsp/WdspChannel.h index f2c1b55b9e..758ee516e2 100644 --- a/src/core/dsp/WdspChannel.h +++ b/src/core/dsp/WdspChannel.h @@ -515,6 +515,9 @@ class WdspChannel final // After output-count readiness, wait for the last hook and DSP body before // arming another hold. Test/control only; NEVER call while a hold is armed. void synchronizeWorkerForTest() const noexcept; + // Test only (#6179, WDSP patch 22): queue PSRestoreCorr(path) on this TX + // channel's PureSignal correction thread, on the WDSP the core drives. + void restorePureSignalCorrectionForTest(const char* path) const noexcept; // Test only: the next `count` control operations on THIS channel are // refused exactly as a racing processIq() callback would refuse them, so // a caller's refused-then-converges path can be driven deterministically. diff --git a/tests/tests.cmake b/tests/tests.cmake index c29fbf1acc..894375f583 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -1016,6 +1016,13 @@ target_link_libraries(wdsp_channel_reservation_test PRIVATE aethercore) add_test(NAME wdsp_channel_reservation_test COMMAND wdsp_channel_reservation_test) set_tests_properties(wdsp_channel_reservation_test PROPERTIES TIMEOUT 120) +# Tearing down a TX channel waits for the PureSignal correction thread before +# freeing what it uses (#6179, WDSP patch 22). POSIX FIFO; skips on Windows. +add_executable(wdsp_calcc_teardown_test tests/wdsp_calcc_teardown_test.cpp) +target_link_libraries(wdsp_calcc_teardown_test PRIVATE aethercore) +add_test(NAME wdsp_calcc_teardown_test COMMAND wdsp_calcc_teardown_test) +set_tests_properties(wdsp_calcc_teardown_test PROPERTIES SKIP_RETURN_CODE 77 TIMEOUT 120) + # Compiles src/core/NnrControls.h so its static_asserts are real, and pins the # default markers the NNR tab draws. Header-only: the WDSP cross-check needs the # facade that arrives with NnrFilter (RFC #5684 step 2). diff --git a/tests/wdsp_calcc_teardown_test.cpp b/tests/wdsp_calcc_teardown_test.cpp new file mode 100644 index 0000000000..1968f06f10 --- /dev/null +++ b/tests/wdsp_calcc_teardown_test.cpp @@ -0,0 +1,102 @@ +// Tearing down a TX channel must wait for WDSP's PureSignal correction thread +// (doPSCorrChange) before freeing what it uses (#6179, WDSP patch 22). +// +// The thread is held in PSRestoreCorr()'s fopen() on a FIFO that has no writer +// yet, which no fixed timeout can outlast. The channel is destroyed on another +// thread; the writer opens 1 s later. CloseChannel() must still be waiting +// then, and return once the thread is released. Upstream gave up after 500 ms +// and freed the CALCC under the blocked thread. + +#include "core/dsp/WdspChannel.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#ifndef _WIN32 +#include +#include +#include +#endif + +namespace { + +int failures = 0; + +void check(bool value, const char* message) +{ + std::cout << (value ? "[ OK ] " : "[FAIL] ") << message << '\n'; + if (!value) { + ++failures; + } +} + +} // namespace + +int main() +{ +#ifdef _WIN32 + std::cout << "[SKIP] needs a POSIX FIFO\n"; + return 77; +#else + WdspChannel::Config tx; + tx.direction = WdspChannel::Direction::Transmit; + tx.inputSampleRate = 48000; + tx.dspSampleRate = 48000; + tx.outputSampleRate = 48000; + std::string error; + std::unique_ptr channel = WdspChannel::create(tx, &error); + check(channel != nullptr, "a transmit channel opens"); + if (!channel) { + std::cerr << error << '\n'; + return 1; + } + + std::string fifo = std::string(std::getenv("TMPDIR") ? std::getenv("TMPDIR") : "/tmp") + + "/wdsp-calcc-teardown-" + std::to_string(::getpid()); + ::unlink(fifo.c_str()); + check(::mkfifo(fifo.c_str(), 0600) == 0, "premise: the FIFO is created"); + + // The correction thread now blocks in fopen(fifo, "r") until a writer opens. + channel->restorePureSignalCorrectionForTest(fifo.c_str()); + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + + std::atomic closed{false}; + std::thread closer([&] { + channel.reset(); + closed.store(true); + }); + + std::this_thread::sleep_for(std::chrono::milliseconds(1000)); + check(!closed.load(), + "teardown is still waiting 1 s later, past upstream's 500 ms limit"); + + // Release the thread: a line that is no correction file, so the restore + // fails and it moves on. Held open briefly so the reader sees a writer. + // Non-blocking, so a thread that never opened the FIFO fails here (ENXIO) + // instead of hanging until the ctest timeout. + const int writer = ::open(fifo.c_str(), O_WRONLY | O_NONBLOCK); + if (writer < 0 && errno == ENXIO) { + check(false, "premise: the correction thread has the FIFO open for reading"); + } + check(writer >= 0, "premise: the writer end opens"); + if (writer >= 0) { + const char junk[] = "not-a-correction-file x\n"; + check(::write(writer, junk, sizeof(junk) - 1) > 0, "premise: the junk line is written"); + std::this_thread::sleep_for(std::chrono::milliseconds(200)); + ::close(writer); + } + closer.join(); + check(closed.load(), "teardown completes once the correction thread is released"); + ::unlink(fifo.c_str()); + + std::cout << failures << " failure(s)\n"; + return failures == 0 ? 0 : 1; +#endif +} diff --git a/third_party/wdsp/AETHERSDR-PATCHES.md b/third_party/wdsp/AETHERSDR-PATCHES.md index 7d27a52f92..426b990f2a 100644 --- a/third_party/wdsp/AETHERSDR-PATCHES.md +++ b/third_party/wdsp/AETHERSDR-PATCHES.md @@ -573,7 +573,7 @@ number, so the grep above does not find them; it is findable from this file and from `ensure_minphase` in `upstream/firmin.c`. When refreshing WDSP, first check whether upstream has made each change -itself. The ten are different shapes, so grep for the shape, not for a free: +itself. They are different shapes, so grep for the shape, not for a free: - **patches 1-2** -- a trailing `_aligned_free()` of the object itself at the end of `destroy_notchdb()` / `destroy_nurbs()`. @@ -616,8 +616,14 @@ itself. The ten are different shapes, so grep for the shape, not for a free: `stitch(disp)`, not before it; and `sendbuf()`'s `IQO_idx` hand-off and `IQout_index` advance inside its existing `BufferControlSection`. - **patch 18** -- in `destroy_calcc()`, the `SemsPSCorr` and - `hCorrChangeExited` closes inside an `== WAIT_OBJECT_0` check on the - thread-exit wait. + `hCorrChangeExited` closes, still only after the thread's exit event was + seen (`corrThreadGone`, patch 22). A thread that was never started has + nothing to wait for, so its handles are closed directly. +- **patch 22** -- in `calcc.c`: the `corrThreadStarted` field set from + `_beginthread()`'s result in `create_calcc()`, and in `destroy_calcc()` the + `hCorrChangeExited` wait (clearing iqc's `busy` bit each pass, unbounded + once patch 4 saw the worker exit) with the six `util` spline frees moved + **after** it, not before. Drop any local patch upstream now carries. Otherwise reapply only these minimal changes and run the lifecycle test under AddressSanitizer on every supported @@ -1003,7 +1009,9 @@ wait, `destroy_calcc()` goes on to free `a` and its members, which a correction thread still running past the timeout would use. That use-after-free predates this patch (upstream frees `a` on the same path) and is tracked separately (#6179). Patch 18 only stops the normal, successful-exit path from -leaking and avoids adding a second hazard on the timeout path. +leaking and avoids adding a second hazard on the timeout path. Patch 22 leaves +the timeout path only behind a wedged DSP worker; there the handles stay open +as above, and `a` is still freed, as on `main`. **Upstream status.** Not reported. @@ -1238,3 +1246,60 @@ freshness and retired observations. These tests are added validation obligations their results must come from a subsequent exact-source Nobara run. On upstream refresh retain this extension unless equivalent typed, bounded, race-free semantics replace it, and do not label INDY as a PLL. + +## Patch 22 — `destroy_calcc()` frees nothing the correction thread still uses (#6179) + +`upstream/calcc.c`. `destroy_calcc()` waited at most 500 ms for the +`doPSCorrChange()` thread to signal `hCorrChangeExited`, then freed `a` and its +members anyway. The thread reads `a`, the six `util` splines, `txa[].iqc` and +`txa[].calcc.cs_update`, all freed by `destroy_calcc()` / `destroy_txa()` +right after. A thread in `calc()` or in correction-file I/O past the 500 ms +went on using freed memory. Upstream also freed the six `util` splines +**before** the wait, under a thread in `PSSaveCorr` / `PSRestoreCorr` work. + +Now: + +- `create_calcc()` records whether `_beginthread()` started the thread + (`corrThreadStarted`). A thread that never started is not waited for, which + is the reason patches 4 and 9 bound their waits. +- `destroy_calcc()` waits for `hCorrChangeExited`, clearing iqc's `busy` bit + on each 1 ms pass, because `SetTXAiqcStart()` / `SetTXAiqcSwap()` spin on + it for a DSP worker that `CloseChannel()` has already stopped. With the four + work semaphores drained first, the exit token is the next one the thread + takes once its current work ends. +- The wait has **no time limit only when patch 4's worker wait succeeded** + (`mainExited == mainGen`). Those iqc setters take `ch[].csDSP`, which is + free only once the worker has exited (patch 9). If patch 4's wait fell + through its cap, a wedged worker may still hold `csDSP`, an unbounded wait + could then never end, and a bound of 500 one-millisecond passes stands + instead: 500 ms on POSIX, up to ~8 s under Windows' default 15.6 ms timer + tick, since nothing in AetherSDR calls `timeBeginPeriod`. That path + is already undefined on `main` (`post_main_destroy` deletes `csDSP` under a + live holder); there the handles stay open as patch 18 had them, and `a` is + still freed as on `main`, so this patch does not make it worse. +- A `WAIT_FAILED` return (an exit event `CreateEvent` failed to make) ends + the wait rather than spinning. +- The `SemsPSCorr` / exit-event closes (patch 18) run only once the exit + event was seen, or when the thread never started. The six `util` spline + frees follow the wait. + +**What can still stall teardown.** The thread's own work, on the normal path: +`calc()` is bounded CPU work. Correction-file I/O can block as long as the file system does. +AetherSDR calls neither `PSSaveCorr` nor `PSRestoreCorr` today; a caller that +adds them owns that stall. + +**Reproduced before fixing**, locally: no `ci.yml` filter runs the test, and +it covers the wait, not the reordering of the frees after it (that half is the +sanitizer lanes'). `wdsp_calcc_teardown_test` holds the thread in +`PSRestoreCorr()` on a FIFO with no writer, closes the channel on another +thread, and opens the writer 1 s later. Before this patch `CloseChannel()` +returned after ~500 ms while the thread was still blocked. With it, +`CloseChannel()` returns only after the writer opens. POSIX only (`mkfifo`); +skipped on Windows. The test queues the restore through +`WdspChannel::restorePureSignalCorrectionForTest()`, so it links under +`AETHER_SHARED_CORE` and the sanitizer lanes build it. + +**Upstream status.** Not reported. + +When updating WDSP, keep this unless upstream's `destroy_calcc()` joins the +correction thread before freeing. diff --git a/third_party/wdsp/include/aether_wdsp.h b/third_party/wdsp/include/aether_wdsp.h index b187272682..89015a89bc 100644 --- a/third_party/wdsp/include/aether_wdsp.h +++ b/third_party/wdsp/include/aether_wdsp.h @@ -527,6 +527,9 @@ int GetChannelOutputSamplesForTest(int channel); // After count readiness, wait for the previous handoff hook/DSP to finish. // Test/control only; never call with either worker hold armed or entered. void SynchronizeChannelWorkerForTest(int channel); +// Queues a correction-file restore on a TX channel's PureSignal correction +// thread. AetherSDR calls it only from wdsp_calcc_teardown_test (patch 22). +void PSRestoreCorr(int channel, char* filename); #ifdef __cplusplus } diff --git a/third_party/wdsp/upstream/calcc.c b/third_party/wdsp/upstream/calcc.c index d087e3fe3c..fcf81bb310 100644 --- a/third_party/wdsp/upstream/calcc.c +++ b/third_party/wdsp/upstream/calcc.c @@ -199,6 +199,7 @@ typedef struct _calcc CurveEMA m_calavg_save, c_calavg_save, s_calavg_save; } util; HANDLE hCorrChangeExited; + int corrThreadStarted; // AetherSDR patch 22: destroy_calcc() waits only for a thread that exists } calcc, * CALCC; void __cdecl doPSCorrChange(void* arg); @@ -1079,7 +1080,7 @@ CALCC create_calcc (int channel, int runcal, int size, int rate, double hw_scale a->SemsPSCorr[i] = CreateSemaphoreW(0, 0, 1, 0); } a->hCorrChangeExited = CreateEvent(NULL, FALSE, FALSE, NULL); - _beginthread(doPSCorrChange, 0, (void*)a); + a->corrThreadStarted = _beginthread(doPSCorrChange, 0, (void*)a) != (uintptr_t)-1; return a; } @@ -1087,32 +1088,45 @@ void destroy_calcc (CALCC a) { IQC b = txa[a->channel].iqc.p; - ns_free(a->util.m_spline_restore); a->util.m_spline_restore = NULL; - ns_free(a->util.c_spline_restore); a->util.c_spline_restore = NULL; - ns_free(a->util.s_spline_restore); a->util.s_spline_restore = NULL; - ns_free(a->util.m_spline_save); a->util.m_spline_save = NULL; - ns_free(a->util.c_spline_save); a->util.c_spline_save = NULL; - ns_free(a->util.s_spline_save); a->util.s_spline_save = NULL; - for (int i = 0; i < 4; i++) while (WaitForSingleObject(a->SemsPSCorr[i], 0) == WAIT_OBJECT_0); InterlockedBitTestAndReset(&b->busy, 0); ReleaseSemaphore(a->SemsPSCorr[4], 1, 0); - // AetherSDR patch 18: close the five SemsPSCorr semaphores, which upstream - // never closed (leaked on every TXA channel destroy), and close them and the - // exit event ONLY once doPSCorrChange() has signalled it is gone. Its last - // act is SetEvent(hCorrChangeExited) then return, so after that nothing - // touches either. On the 500 ms timeout the thread may still be waiting on - // them, so they are not freed under it. That does NOT make the timeout path - // safe: the rest of this function still frees `a`, which a late thread - // would use. That hazard predates this patch and is tracked separately - // (#6179; see AETHERSDR-PATCHES.md, patch 18). - if (WaitForSingleObject(a->hCorrChangeExited, 500) == WAIT_OBJECT_0) + // AetherSDR patch 22 (#6179): nothing the correction thread touches (`a`, + // its util splines, the iqc stage, cs_update) is freed until it has exited. + // No time limit once patch 4 saw the DSP worker exit: csDSP, which the + // thread's iqc setters take, is then free, and iqc's busy bit they spin on + // is cleared each pass. If that wait fell through, 500 passes bound it + // (500 ms on POSIX, longer at Windows' default timer tick). + int corrThreadGone = !a->corrThreadStarted; + if (a->corrThreadStarted && a->hCorrChangeExited != NULL) + { + const int workerGone = _InterlockedAnd(&ch[a->channel].mainExited, ~0L) + == _InterlockedAnd(&ch[a->channel].mainGen, ~0L); + int waited = 0; + DWORD waitstat; + while ((waitstat = WaitForSingleObject(a->hCorrChangeExited, 1)) != WAIT_OBJECT_0) + { + if (waitstat == WAIT_FAILED || (!workerGone && ++waited >= 500)) + break; + InterlockedBitTestAndReset(&b->busy, 0); + } + corrThreadGone = waitstat == WAIT_OBJECT_0; + } + // AetherSDR patch 18: closed only once the thread is gone; past the + // bounded fallback it may still wait on them or set the event. + if (corrThreadGone) { for (int i = 0; i < 5; i++) CloseHandle(a->SemsPSCorr[i]); CloseHandle(a->hCorrChangeExited); } + ns_free(a->util.m_spline_restore); a->util.m_spline_restore = NULL; + ns_free(a->util.c_spline_restore); a->util.c_spline_restore = NULL; + ns_free(a->util.s_spline_restore); a->util.s_spline_restore = NULL; + ns_free(a->util.m_spline_save); a->util.m_spline_save = NULL; + ns_free(a->util.c_spline_save); a->util.c_spline_save = NULL; + ns_free(a->util.s_spline_save); a->util.s_spline_save = NULL; ns_free(a->m_spline); a->m_spline = NULL; ns_free(a->c_spline); a->c_spline = NULL;