From 08ff63f9947cf8a308f488bdf0c52975784d5831 Mon Sep 17 00:00:00 2001 From: jcmerg <56387246+jcmerg@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:08:39 +0200 Subject: [PATCH 1/4] fix(wdsp): tearing down a TX channel waits for the PureSignal correction thread. Principle VIII. destroy_calcc() waited at most 500 ms for doPSCorrChange() to exit, then freed the CALCC, its util splines, and (in destroy_txa) the iqc stage and cs_update the thread still uses. It also freed the six util splines before the wait. A thread in calc() or correction-file I/O past 500 ms used freed memory (#6179). WDSP patch 22: create_calcc() records whether the thread started; destroy_calcc() waits for its exit with no time limit, clearing iqc's busy bit on each pass so SetTXAiqcStart/Swap cannot spin on a stopped DSP worker, and frees the util splines and closes the handles only after it. wdsp_calcc_teardown_test holds the thread in PSRestoreCorr() on a FIFO: red on the unpatched calcc.c (teardown returned after ~500 ms), green with the patch. Closes #6179 Co-authored-by: Claude Opus 5.5 --- tests/tests.cmake | 7 ++ tests/wdsp_calcc_teardown_test.cpp | 98 +++++++++++++++++++++++++++ third_party/wdsp/AETHERSDR-PATCHES.md | 52 +++++++++++++- third_party/wdsp/upstream/calcc.c | 40 ++++++----- 4 files changed, 173 insertions(+), 24 deletions(-) create mode 100644 tests/wdsp_calcc_teardown_test.cpp diff --git a/tests/tests.cmake b/tests/tests.cmake index c29fbf1acc..f65950bade 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -1011,6 +1011,13 @@ set_tests_properties(audio_thread_planner_join_test PROPERTIES TIMEOUT 60) # Socket-free shared-pool admission and injected receiver lifetime tests. These # foundations are compiled/tested even when the optional RTL USB driver is off. +# 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 TIMEOUT 120) + add_executable(wdsp_channel_reservation_test tests/wdsp_channel_reservation_test.cpp) target_link_libraries(wdsp_channel_reservation_test PRIVATE aethercore) add_test(NAME wdsp_channel_reservation_test COMMAND wdsp_channel_reservation_test) diff --git a/tests/wdsp_calcc_teardown_test.cpp b/tests/wdsp_calcc_teardown_test.cpp new file mode 100644 index 0000000000..6570a353ee --- /dev/null +++ b/tests/wdsp_calcc_teardown_test.cpp @@ -0,0 +1,98 @@ +// 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 + +#ifndef _WIN32 +#include +#include +#include +#endif + +extern "C" void PSRestoreCorr(int channel, char* filename); + +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 0; +#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, "rb") until a writer opens. + PSRestoreCorr(channel->channelId(), fifo.data()); + 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. + const int writer = ::open(fifo.c_str(), O_WRONLY); + 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..a501335963 100644 --- a/third_party/wdsp/AETHERSDR-PATCHES.md +++ b/third_party/wdsp/AETHERSDR-PATCHES.md @@ -616,8 +616,12 @@ 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. Patch 22 replaced its `== WAIT_OBJECT_0` check: + the closes now follow the unbounded exit wait. +- **patch 22** -- in `calcc.c`: the `corrThreadStarted` field set from + `_beginthread()`'s result in `create_calcc()`, and in `destroy_calcc()` the + unbounded `hCorrChangeExited` wait (clearing iqc's `busy` bit each pass) + 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 +1007,8 @@ 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 closes +the timeout path by removing it. **Upstream status.** Not reported. @@ -1238,3 +1243,44 @@ 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` with **no time limit**. The + four work semaphores are drained first, so the exit token is the next one the + thread takes once its current work ends. Each 1 ms pass clears iqc's `busy` + bit, because `SetTXAiqcStart()` / `SetTXAiqcSwap()` spin on it for a DSP + worker that `CloseChannel()` has already stopped. `ch[].csDSP`, which those + setters take, is free at this point (patch 9). +- The `SemsPSCorr` / exit-event closes (patch 18) and the six `util` spline + frees follow the exit. + +**What can still stall teardown.** The thread's own work: `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.** `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. + +**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/upstream/calcc.c b/third_party/wdsp/upstream/calcc.c index d087e3fe3c..1f0fb7e5ce 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,6 +1088,23 @@ void destroy_calcc (CALCC a) { IQC b = txa[a->channel].iqc.p; + 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 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: with the work semaphores drained the exit token is its + // next one. iqc's busy bit is cleared on each pass, since SetTXAiqcStart/ + // Swap spin on it for a DSP worker that is already gone. + if (a->corrThreadStarted) + { + while (WaitForSingleObject(a->hCorrChangeExited, 1) != WAIT_OBJECT_0) + InterlockedBitTestAndReset(&b->busy, 0); + } + 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; @@ -1094,26 +1112,6 @@ void destroy_calcc (CALCC a) 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) - { - for (int i = 0; i < 5; i++) - CloseHandle(a->SemsPSCorr[i]); - CloseHandle(a->hCorrChangeExited); - } - ns_free(a->m_spline); a->m_spline = NULL; ns_free(a->c_spline); a->c_spline = NULL; ns_free(a->s_spline); a->s_spline = NULL; From a6084f8e86cebb63a048dcd6bd733cd168c31548 Mon Sep 17 00:00:00 2001 From: jcmerg <56387246+jcmerg@users.noreply.github.com> Date: Wed, 7 Oct 2026 22:51:11 +0200 Subject: [PATCH 2/4] fix(wdsp): wait without a limit only once the DSP worker is gone; end on WAIT_FAILED. Principle VIII. Review of #6261: the correction thread's iqc setters take ch[].csDSP, which is free only when patch 4's bounded worker wait succeeded. If it fell through, a wedged worker may hold csDSP and an unbounded wait could never end. destroy_calcc() now waits without a limit only when mainExited == mainGen, and keeps upstream's 500 ms otherwise (already undefined on main). A WAIT_FAILED return ends the wait instead of spinning, and a NULL exit event is not waited on. Also: the test comment's fopen mode, the test block placed after the reservation block it had split from its comment, and the patch-ledger wording. Co-authored-by: Claude Opus 5.5 --- tests/tests.cmake | 10 ++++---- tests/wdsp_calcc_teardown_test.cpp | 2 +- third_party/wdsp/AETHERSDR-PATCHES.md | 35 ++++++++++++++++++--------- third_party/wdsp/upstream/calcc.c | 20 ++++++++++----- 4 files changed, 43 insertions(+), 24 deletions(-) diff --git a/tests/tests.cmake b/tests/tests.cmake index f65950bade..fdecc4dac3 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -1011,6 +1011,11 @@ set_tests_properties(audio_thread_planner_join_test PROPERTIES TIMEOUT 60) # Socket-free shared-pool admission and injected receiver lifetime tests. These # foundations are compiled/tested even when the optional RTL USB driver is off. +add_executable(wdsp_channel_reservation_test tests/wdsp_channel_reservation_test.cpp) +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) @@ -1018,11 +1023,6 @@ 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 TIMEOUT 120) -add_executable(wdsp_channel_reservation_test tests/wdsp_channel_reservation_test.cpp) -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) - # 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 index 6570a353ee..b0f9727f40 100644 --- a/tests/wdsp_calcc_teardown_test.cpp +++ b/tests/wdsp_calcc_teardown_test.cpp @@ -64,7 +64,7 @@ int main() ::unlink(fifo.c_str()); check(::mkfifo(fifo.c_str(), 0600) == 0, "premise: the FIFO is created"); - // The correction thread now blocks in fopen(fifo, "rb") until a writer opens. + // The correction thread now blocks in fopen(fifo, "r") until a writer opens. PSRestoreCorr(channel->channelId(), fifo.data()); std::this_thread::sleep_for(std::chrono::milliseconds(100)); diff --git a/third_party/wdsp/AETHERSDR-PATCHES.md b/third_party/wdsp/AETHERSDR-PATCHES.md index a501335963..4664a76b13 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()`. @@ -620,8 +620,9 @@ itself. The ten are different shapes, so grep for the shape, not for a free: the closes now follow the unbounded exit wait. - **patch 22** -- in `calcc.c`: the `corrThreadStarted` field set from `_beginthread()`'s result in `create_calcc()`, and in `destroy_calcc()` the - unbounded `hCorrChangeExited` wait (clearing iqc's `busy` bit each pass) - with the six `util` spline frees moved **after** it, not before. + `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 @@ -1259,21 +1260,31 @@ 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` with **no time limit**. The - four work semaphores are drained first, so the exit token is the next one the - thread takes once its current work ends. Each 1 ms pass clears iqc's `busy` - bit, because `SetTXAiqcStart()` / `SetTXAiqcSwap()` spin on it for a DSP - worker that `CloseChannel()` has already stopped. `ch[].csDSP`, which those - setters take, is free at this point (patch 9). +- `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 upstream's 500 ms bound stands instead. That path + is already undefined on `main` (`post_main_destroy` deletes `csDSP` under a + live holder); 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) and the six `util` spline frees follow the exit. -**What can still stall teardown.** The thread's own work: `calc()` is bounded -CPU work. Correction-file I/O can block as long as the file system does. +**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.** `wdsp_calcc_teardown_test` holds the thread in +**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, diff --git a/third_party/wdsp/upstream/calcc.c b/third_party/wdsp/upstream/calcc.c index 1f0fb7e5ce..e455dbd15e 100644 --- a/third_party/wdsp/upstream/calcc.c +++ b/third_party/wdsp/upstream/calcc.c @@ -1094,13 +1094,21 @@ void destroy_calcc (CALCC a) ReleaseSemaphore(a->SemsPSCorr[4], 1, 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: with the work semaphores drained the exit token is its - // next one. iqc's busy bit is cleared on each pass, since SetTXAiqcStart/ - // Swap spin on it for a DSP worker that is already gone. - if (a->corrThreadStarted) - { - while (WaitForSingleObject(a->hCorrChangeExited, 1) != WAIT_OBJECT_0) + // 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, upstream's 500 ms stands. + 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); + } } for (int i = 0; i < 5; i++) CloseHandle(a->SemsPSCorr[i]); From aa80a5fb52c45cf6a5f51109d60a077e1751b25f Mon Sep 17 00:00:00 2001 From: jcmerg <56387246+jcmerg@users.noreply.github.com> Date: Sat, 10 Oct 2026 08:30:52 +0200 Subject: [PATCH 3/4] fix(wdsp): the calcc teardown test reaches WDSP through WdspChannel, so it links under the shared core. Principle VIII. Review of #6261 (K5PTB): the test declared PSRestoreCorr itself and linked only aethercore. Under AETHER_SHARED_CORE=ON libaethercore hides WDSP's symbols, so the link failed and both weekly sanitizer lanes would stop at Build, the regression #6309 fixed. PSRestoreCorr is now declared in aether_wdsp.h and reached through WdspChannel::restorePureSignalCorrectionForTest(). Also: the writer opens the FIFO non-blocking, so a correction thread that never opened it fails on ENXIO instead of hanging until the ctest timeout; and the wedged-worker fallback is described as 500 one-millisecond passes, which is 500 ms on POSIX but up to ~8 s at Windows' default timer tick. Rebased onto main (ca5e0634) so CI covers #6309. Co-authored-by: Claude Opus 5.5 --- src/core/dsp/WdspChannel.cpp | 6 ++++++ src/core/dsp/WdspChannel.h | 3 +++ tests/wdsp_calcc_teardown_test.cpp | 12 ++++++++---- third_party/wdsp/AETHERSDR-PATCHES.md | 8 ++++++-- third_party/wdsp/include/aether_wdsp.h | 3 +++ third_party/wdsp/upstream/calcc.c | 3 ++- 6 files changed, 28 insertions(+), 7 deletions(-) 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/wdsp_calcc_teardown_test.cpp b/tests/wdsp_calcc_teardown_test.cpp index b0f9727f40..8e375c2a4f 100644 --- a/tests/wdsp_calcc_teardown_test.cpp +++ b/tests/wdsp_calcc_teardown_test.cpp @@ -11,6 +11,7 @@ #include #include +#include #include #include #include @@ -24,8 +25,6 @@ #include #endif -extern "C" void PSRestoreCorr(int channel, char* filename); - namespace { int failures = 0; @@ -65,7 +64,7 @@ int main() 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. - PSRestoreCorr(channel->channelId(), fifo.data()); + channel->restorePureSignalCorrectionForTest(fifo.c_str()); std::this_thread::sleep_for(std::chrono::milliseconds(100)); std::atomic closed{false}; @@ -80,7 +79,12 @@ int main() // 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. - const int writer = ::open(fifo.c_str(), O_WRONLY); + // 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"; diff --git a/third_party/wdsp/AETHERSDR-PATCHES.md b/third_party/wdsp/AETHERSDR-PATCHES.md index 4664a76b13..96f73f4859 100644 --- a/third_party/wdsp/AETHERSDR-PATCHES.md +++ b/third_party/wdsp/AETHERSDR-PATCHES.md @@ -1269,7 +1269,9 @@ Now: (`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 upstream's 500 ms bound stands instead. That path + 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); this patch does not make it worse. - A `WAIT_FAILED` return (an exit event `CreateEvent` failed to make) ends @@ -1289,7 +1291,9 @@ sanitizer lanes'). `wdsp_calcc_teardown_test` holds the thread in 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. +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. 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 e455dbd15e..16a1997f2a 100644 --- a/third_party/wdsp/upstream/calcc.c +++ b/third_party/wdsp/upstream/calcc.c @@ -1096,7 +1096,8 @@ void destroy_calcc (CALCC 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, upstream's 500 ms stands. + // is cleared each pass. If that wait fell through, 500 passes bound it + // (500 ms on POSIX, longer at Windows' default timer tick). if (a->corrThreadStarted && a->hCorrChangeExited != NULL) { const int workerGone = _InterlockedAnd(&ch[a->channel].mainExited, ~0L) From 9ffa9ace728bb9f9c646cd9a6ac6594d5c47c963 Mon Sep 17 00:00:00 2001 From: jcmerg <56387246+jcmerg@users.noreply.github.com> Date: Sat, 10 Oct 2026 11:29:50 +0200 Subject: [PATCH 4/4] fix(wdsp): the correction thread's handles close only once it has exited, as patch 18 had them. Principle VIII. Review of #6261 (K6OZY): a6084f8e made the SemsPSCorr / hCorrChangeExited closes unconditional. On the bounded fallback behind a wedged DSP worker, and on WAIT_FAILED, the loop breaks with the thread alive, and under the POSIX shim CloseHandle frees the handle the thread still waits on or will set. The closes now run only when the exit event was seen, or when the thread never started; that path keeps main's behaviour for `a`. The patch 18 and 22 notes no longer say the timeout path is gone. Also: on Windows the test returns 77 and is registered with SKIP_RETURN_CODE 77, so ctest reports it skipped rather than passed. Co-authored-by: Claude Opus 5.5 --- tests/tests.cmake | 2 +- tests/wdsp_calcc_teardown_test.cpp | 2 +- third_party/wdsp/AETHERSDR-PATCHES.md | 18 +++++++++++------- third_party/wdsp/upstream/calcc.c | 13 ++++++++++--- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/tests/tests.cmake b/tests/tests.cmake index fdecc4dac3..894375f583 100644 --- a/tests/tests.cmake +++ b/tests/tests.cmake @@ -1021,7 +1021,7 @@ set_tests_properties(wdsp_channel_reservation_test PROPERTIES TIMEOUT 120) 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 TIMEOUT 120) +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 diff --git a/tests/wdsp_calcc_teardown_test.cpp b/tests/wdsp_calcc_teardown_test.cpp index 8e375c2a4f..1968f06f10 100644 --- a/tests/wdsp_calcc_teardown_test.cpp +++ b/tests/wdsp_calcc_teardown_test.cpp @@ -43,7 +43,7 @@ int main() { #ifdef _WIN32 std::cout << "[SKIP] needs a POSIX FIFO\n"; - return 0; + return 77; #else WdspChannel::Config tx; tx.direction = WdspChannel::Direction::Transmit; diff --git a/third_party/wdsp/AETHERSDR-PATCHES.md b/third_party/wdsp/AETHERSDR-PATCHES.md index 96f73f4859..426b990f2a 100644 --- a/third_party/wdsp/AETHERSDR-PATCHES.md +++ b/third_party/wdsp/AETHERSDR-PATCHES.md @@ -616,8 +616,9 @@ itself. They 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. Patch 22 replaced its `== WAIT_OBJECT_0` check: - the closes now follow the unbounded 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 @@ -1008,8 +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. Patch 22 closes -the timeout path by removing it. +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. @@ -1273,11 +1275,13 @@ Now: 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); this patch does not make it worse. + 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) and the six `util` spline - frees follow the exit. +- 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. diff --git a/third_party/wdsp/upstream/calcc.c b/third_party/wdsp/upstream/calcc.c index 16a1997f2a..fcf81bb310 100644 --- a/third_party/wdsp/upstream/calcc.c +++ b/third_party/wdsp/upstream/calcc.c @@ -1098,6 +1098,7 @@ void destroy_calcc (CALCC a) // 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) @@ -1110,10 +1111,16 @@ void destroy_calcc (CALCC a) 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); } - 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;