fix(cat): abort rigctld clients before deleting QTcpServer#3414
Conversation
f965e66 to
56c56a4
Compare
|
For reviewer: I submitted a PR from my Linux system the first time (usually do it on my Mac). Didn't realize my code signing didn't make it to my Linux system. Corrected that and re-pushed with a signature. |
There was a problem hiding this comment.
Thanks for the careful root-cause writeup and the ASAN trace — the diagnosis is exactly right. QTcpServer::nextPendingConnection() parents each socket to the server (CatPort.cpp:142), so deleting m_tcpServer first frees the sockets and the subsequent abort() is on freed memory. The reorder is the correct fix.
One adjacent concern worth a look while you're in here (happy for it to be a follow-up, not blocking):
QAbstractSocket::abort() emits disconnected synchronously, and the slot you connect at CatPort.cpp:154 (onRigctlDisconnected) calls m_rigctlClients.removeAt(i) (CatPort.cpp:210). So calling c.socket->abort() inside for (auto& c : m_rigctlClients) mutates the list mid-iteration — fine with a single client (which is the ASAN repro), but with 2+ clients the range-for's iterator/reference can be invalidated and delete c.protocol ends up running on a stale entry (or the second delete runs in onRigctlDisconnected's removeAt path on memory we just freed below).
A small tweak would close that off — e.g. swap the container out first, or disconnect signals before aborting:
auto clients = std::exchange(m_rigctlClients, {});
for (auto& c : clients) {
QObject::disconnect(c.socket, nullptr, this, nullptr);
c.socket->abort();
delete c.protocol;
}That makes the loop safe regardless of how many clients are connected and avoids the double-cleanup path through onRigctlDisconnected during shutdown.
The PR as-is fixes the reported crash, which is the main thing — feel free to defer the multi-client hardening to a separate change if you'd prefer to keep this PR tight.
🤖 aethersdr-agent · cost: $10.6130 · model: claude-opus-4-7
QTcpServer parents the sockets returned by nextPendingConnection() to itself, so delete m_tcpServer frees all client sockets. The loop that followed called c.socket->abort() on those already-freed objects, causing a SEGV on shutdown. Fix: clear m_rigctlClients (abort + delete protocol) before deleting m_tcpServer so the sockets are still valid. Diagnosed via AddressSanitizer — crash address 0x0000000bd97d in QAbstractSocket::abort() during RadioConnection teardown. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
56c56a4 to
69036ff
Compare
|
Thanks for catching the multi-client iterator invalidation. Applied the |
ten9876
left a comment
There was a problem hiding this comment.
Clean memory-safety fix with sanitizer evidence. The std::exchange + signal-disconnect-before-abort pattern correctly handles both the original use-after-free and the bot's multi-client iterator-invalidation concern. Comment explains both issues for future readers. Approving as Tier 3 CODEOWNER for src/core/.
Principle XI.
## Summary Three leaks found and fixed during an ASAN session on Linux/RPi5. Total reduction: 137,879 bytes → 0 bytes leaked on exit. The leaks were not causing a crash, but were detected while investigating a use-after-free in `CatPort::stop()` (see PR #3414). - **`SpectrumWidget`**: `prepareForShutdown()` was guarded by `#ifndef Q_OS_LINUX`, preventing `releaseResources()` from running on Linux shutdown. Removing the guard eliminates 137 KB of GPU pipeline objects reported as leaked on exit. - **`MainWindow` (two fixes in one commit)**: A `pcMicLevelChanged` signal lambda captured two heap-allocated `float` values via raw pointer — replaced with `std::make_shared<float>` so they are freed when the connection is destroyed. Also removed an orphaned `m_radioInfoLabel = new QLabel("")` allocation that was immediately overwritten four lines later. - **`MainWindow`**: `m_connStatusLabel` was created with no parent and never added to a layout, so Qt never took ownership. Parenting to `this` also cleans up the `QAccessibleCache` entry created by the first `setText()` call. > **Note:** the branch also contains a cherry-pick of the `CatPort` fix from PR #3414, included here only to keep ASAN output clean during testing. That fix is already tracked in its own PR. ## Test plan - [ ] Debug/ASAN build on Linux (RPi5 aarch64): `LEAK_SANITIZER: detected memory leaks` absent on clean exit - [ ] ASAN baseline before fixes: 137,879 bytes in 245 allocations - [ ] ASAN after all fixes: 0 bytes leaked from application code 🤖 Generated with [Claude Code](https://claude.ai/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…#3414) ## Summary - `CatPort::stop()` called `c.socket->abort()` after `delete m_tcpServer`, which freed the sockets first (Qt parents them to the server via `nextPendingConnection()`) — use-after-free on every shutdown with a connected rigctld client ## Root cause `QTcpServer::nextPendingConnection()` creates each `QTcpSocket` with the Qt server as its parent. `delete m_tcpServer` therefore frees all client sockets before the rigctld loop ran `abort()` on them. ## Fix Move the rigctld client cleanup (abort + delete protocol) to before `delete m_tcpServer`. ## Repro / evidence Triggered reliably by closing AetherSDR via the window close button while an external client is connected via rigctld (e.g. Direwolf PTT). AddressSanitizer trace: ``` ERROR: AddressSanitizer: SEGV on unknown address 0x0000000bd97d #0 QAbstractSocket::abort() libQt6Network.so.6 aethersdr#1 AetherSDR::CatPort::stop() CatPort.cpp:98 aethersdr#2 AetherSDR::CatPort::~CatPort() CatPort.cpp:60 aethersdr#3 QObjectPrivate::deleteChildren() libQt6Core.so.6 aethersdr#4 QWidget::~QWidget() libQt6Widgets.so.6 aethersdr#5 AetherSDR::MainWindow::~MainWindow() MainWindow.cpp:5490 ``` ## Test - Debug build (ASAN): no SEGV on close with rigctld client connected ✓ - RelWithDebInfo build: clean exit on close ✓ - Platform: RPi5, Debian, Qt 6, rigctld client = Direwolf PTT Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary Three leaks found and fixed during an ASAN session on Linux/RPi5. Total reduction: 137,879 bytes → 0 bytes leaked on exit. The leaks were not causing a crash, but were detected while investigating a use-after-free in `CatPort::stop()` (see PR aethersdr#3414). - **`SpectrumWidget`**: `prepareForShutdown()` was guarded by `#ifndef Q_OS_LINUX`, preventing `releaseResources()` from running on Linux shutdown. Removing the guard eliminates 137 KB of GPU pipeline objects reported as leaked on exit. - **`MainWindow` (two fixes in one commit)**: A `pcMicLevelChanged` signal lambda captured two heap-allocated `float` values via raw pointer — replaced with `std::make_shared<float>` so they are freed when the connection is destroyed. Also removed an orphaned `m_radioInfoLabel = new QLabel("")` allocation that was immediately overwritten four lines later. - **`MainWindow`**: `m_connStatusLabel` was created with no parent and never added to a layout, so Qt never took ownership. Parenting to `this` also cleans up the `QAccessibleCache` entry created by the first `setText()` call. > **Note:** the branch also contains a cherry-pick of the `CatPort` fix from PR aethersdr#3414, included here only to keep ASAN output clean during testing. That fix is already tracked in its own PR. ## Test plan - [ ] Debug/ASAN build on Linux (RPi5 aarch64): `LEAK_SANITIZER: detected memory leaks` absent on clean exit - [ ] ASAN baseline before fixes: 137,879 bytes in 245 allocations - [ ] ASAN after all fixes: 0 bytes leaked from application code 🤖 Generated with [Claude Code](https://claude.ai/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
CatPort::stop()calledc.socket->abort()afterdelete m_tcpServer, which freed the sockets first (Qt parents them to the server vianextPendingConnection()) — use-after-free on every shutdown with a connected rigctld clientRoot cause
QTcpServer::nextPendingConnection()creates eachQTcpSocketwith the Qt server as its parent.delete m_tcpServertherefore frees all client sockets before the rigctld loop ranabort()on them.Fix
Move the rigctld client cleanup (abort + delete protocol) to before
delete m_tcpServer.Repro / evidence
Triggered reliably by closing AetherSDR via the window close button while an external client is connected via rigctld (e.g. Direwolf PTT). AddressSanitizer trace:
Test