fix(gui): fix memory leaks found via AddressSanitizer#3417
Conversation
There was a problem hiding this comment.
Thanks for the careful ASAN session, @K5PTB — the writeup is excellent and the root causes are all well-identified. The three fixes look correct and minimal.
Verified:
MainWindow.cpp:10137— confirmed orphaned: line 10142 immediately re-assignsm_radioInfoLabel, so removing the first allocation is clean. ✓MainWindow.cpp:4421–4422—std::make_shared<float>captured by value into the lambda is the right C++20 idiom here; the floats are freed when the connection is destroyed. ✓MainWindow.cpp:10041— parentingm_connStatusLabeltothisgives Qt ownership; correct fix for an unparented widget never added to a layout. ✓CatPort.cpp— cherry-picked from #3414 and noted as such. No concerns; will collapse when #3414 lands.
One thing worth flagging for the maintainer:
The #ifndef Q_OS_LINUX guard in SpectrumWidget::prepareForShutdown() was introduced in PR #2786 alongside the macOS GPU lifecycle work — it looks like a precautionary exclusion rather than a fix for a known Linux problem. Your testing on RPi5 shows releaseResources() is safe on Linux and reclaims the 137 KB, so removing the guard is reasonable. Worth confirming with someone running an x86 Linux desktop build + a discrete GPU before merge, since RPi5 aarch64 uses a different Qt RHI backend (likely OpenGL ES via V3D) than a Mesa Vulkan/GLX setup.
Otherwise this looks good to land — clean, focused, and the test plan ties leak bytes to specific allocations.
🤖 aethersdr-agent · cost: $14.8670 · model: claude-opus-4-7
prepareForShutdown() skipped releaseResources() on Linux behind a #ifndef Q_OS_LINUX guard. The guard was appropriate for the reparenting path (where Linux/OpenGL handles surface changes differently) but not for shutdown — on Linux the pipeline objects were never freed, leaking 137 KB (245 allocations) on every exit. Remove the guard from the shutdown path only; the reparenting guard at prepareForTopLevelChange() is unchanged. Diagnosed via AddressSanitizer on RPi5 (Debug build). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace raw `new float` captures in pcMicLevelChanged lambda with
std::make_shared<float> so they are freed when the connection is destroyed
- Remove orphaned `m_radioInfoLabel = new QLabel("")` allocation that was
immediately overwritten by a second allocation four lines later
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
QLabel 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. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
36ad7bb to
776431a
Compare
ten9876
left a comment
There was a problem hiding this comment.
Rebased the branch locally to drop the now-merged CatPort commit from #3414 — left three clean leak fixes:
SpectrumWidget::prepareForShutdown—#ifndef Q_OS_LINUXguard removed (137 KB GPU pipeline leak)MainWindowpcMicLevelChanged lambda —std::make_shared<float>+ orphan QLabel removedMainWindow::m_connStatusLabel— parented tothis
Smoke-tested the SpectrumWidget guard removal locally on x86 Linux (CachyOS, Wayland, Intel iGPU + NVIDIA RTX 4090 dGPU, QRhi OpenGL backend). App built clean, all three GPU pipelines initialized (waterfall, overlay, spectrum), sustained run without crashes. SIGTERM exit clean. The full Qt close-event path wasn't exercised under SIGTERM, but the bot's specific concern — that x86 Mesa might behave differently than RPi5's V3D — is addressed by the successful x86 GPU init and stable run.
Approving as Tier 3 CODEOWNER.
Principle XI.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. Principle XI.
Two related fixes to TitleBar's new PanLock accessors so all four platforms compile again: 1. signals: vs. public: — MOC parses anything inside a `signals:` block as a signal declaration, so the inline `bool isPanFollowChecked() const` and `void setPanFollowChecked(bool)` were rejected at moc time with "Not a signal declaration" (TitleBar.h:56). Both methods are accessors, not signals — they belong in `public:`. 2. Out-of-line over inline. Even after the section move, the inline bodies referenced `m_panFollowBtn->isChecked()` and `QSignalBlocker`, which need the full QPushButton type. The header only forward-declares QPushButton, so inline bodies couldn't compile in callers either (the moc error masked this until now). Moving the definitions to TitleBar.cpp keeps the header light and matches the style of neighbours like `isSystemMoveAreaAt`. Build verified locally on Arch Linux x86 — 632/632 clean (only the pre-existing unrelated macDaxDriverInstalled warning). Same maintainer fix-up pattern as aethersdr#3279/aethersdr#3286/aethersdr#3289/aethersdr#3381/aethersdr#3398/aethersdr#3417/aethersdr#3439/aethersdr#3441. 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 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
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, preventingreleaseResources()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): ApcMicLevelChangedsignal lambda captured two heap-allocatedfloatvalues via raw pointer — replaced withstd::make_shared<float>so they are freed when the connection is destroyed. Also removed an orphanedm_radioInfoLabel = new QLabel("")allocation that was immediately overwritten four lines later.MainWindow:m_connStatusLabelwas created with no parent and never added to a layout, so Qt never took ownership. Parenting tothisalso cleans up theQAccessibleCacheentry created by the firstsetText()call.Test plan
LEAK_SANITIZER: detected memory leaksabsent on clean exit🤖 Generated with Claude Code