Skip to content

fix(gui): fix memory leaks found via AddressSanitizer#3417

Merged
ten9876 merged 3 commits into
aethersdr:mainfrom
K5PTB:fix/asan-leak-fixes
Jun 6, 2026
Merged

fix(gui): fix memory leaks found via AddressSanitizer#3417
ten9876 merged 3 commits into
aethersdr:mainfrom
K5PTB:fix/asan-leak-fixes

Conversation

@K5PTB

@K5PTB K5PTB commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

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

@K5PTB K5PTB marked this pull request as ready for review June 6, 2026 00:04
@K5PTB K5PTB requested a review from a team as a code owner June 6, 2026 00:04

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-assigns m_radioInfoLabel, so removing the first allocation is clean. ✓
  • MainWindow.cpp:4421–4422std::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 — parenting m_connStatusLabel to this gives 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

@ten9876 ten9876 self-assigned this Jun 6, 2026
K5PTB and others added 3 commits June 5, 2026 22:31
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>
@ten9876 ten9876 force-pushed the fix/asan-leak-fixes branch from 36ad7bb to 776431a Compare June 6, 2026 05:37

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased the branch locally to drop the now-merged CatPort commit from #3414 — left three clean leak fixes:

  1. SpectrumWidget::prepareForShutdown#ifndef Q_OS_LINUX guard removed (137 KB GPU pipeline leak)
  2. MainWindow pcMicLevelChanged lambda — std::make_shared<float> + orphan QLabel removed
  3. MainWindow::m_connStatusLabel — parented to this

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.

@ten9876 ten9876 enabled auto-merge (squash) June 6, 2026 05:38
@ten9876 ten9876 merged commit ea282be into aethersdr:main Jun 6, 2026
4 checks passed
@K5PTB K5PTB deleted the fix/asan-leak-fixes branch June 6, 2026 08:44
ten9876 added a commit to ea5wa/AetherSDR that referenced this pull request Jun 6, 2026
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.
ten9876 added a commit to ea5wa/AetherSDR that referenced this pull request Jun 7, 2026
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.
G6PWY-Chris pushed a commit to G6PWY-Chris/AetherSDR that referenced this pull request Jun 22, 2026
## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants