Skip to content

Fix packet-pool slot leak in canned message destination picker - #11017

Merged
thebentern merged 1 commit into
developfrom
claude/funny-panini-4cbeb3
Jul 16, 2026
Merged

Fix packet-pool slot leak in canned message destination picker#11017
thebentern merged 1 commit into
developfrom
claude/funny-panini-4cbeb3

Conversation

@thebentern

@thebentern thebentern commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

CannedMessageModule::updateDestinationSelectionList() allocated a meshtastic_MeshPacket via allocDataPacket() that was never sent nor released, permanently consuming one slot from packetPool on every rebuild of the destination-selection picker.

meshtastic_MeshPacket *p = allocDataPacket();
p->pki_encrypted = true;
p->channel = 0;
// ...p is never used again, never sent, never released

Impact

  • Non-PSRAM targets: packetPool is a static 70-slot BSS pool (Router.cpp:57-66). Repeated use of the destination picker steadily drains it and eventually blocks all packet allocation, causing TX/RX failures.
  • PSRAM / portduino targets: packetPool uses MemoryDynamic, so this is a true heap leak of ~424 B per rebuild.

The picker is rebuilt on entry to destination selection, on every search keystroke, and whenever the node list changes, so the leak accumulates quickly in normal use.

Root cause

The three lines were a copy/paste artifact of the legitimate PKI setup in sendText() (introduced in #8182, "Multi message storage"). In sendText() the allocated packet is populated and handed to service->sendToMesh(); here it was pasted into a function that only builds a UI list and has nothing to send.

Fix

Remove the dead allocation. No other code in the function references p, so there is no behavior change beyond eliminating the leak. allocDataPacket()'s only side effect is bumping the monotonic packet-ID counter, which is harmless to skip since nothing was ever transmitted.

Testing

  • Native test suite: 602/602 passing (./bin/test-native-docker.sh)
  • trunk fmt: clean

Summary by CodeRabbit

  • Refactor
    • Simplified internal destination-selection processing without changing observable behavior.

updateDestinationSelectionList() allocated a MeshPacket via
allocDataPacket() that was never sent or released, permanently
consuming one packetPool slot every time the destination-selection
picker was rebuilt.

On non-PSRAM targets packetPool is a static 70-slot BSS pool, so
repeated picker use exhausts it and eventually blocks all packet
allocation (TX/RX failures). On PSRAM/portduino (MemoryDynamic)
targets it is a true heap leak of ~424B per rebuild.

The allocation and its two field writes (pki_encrypted, channel) were
a copy/paste artifact of the PKI setup in sendText() and had no effect
in this function. Remove the dead allocation.
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 89384332-5483-49a1-85e4-28734ab1e903

📥 Commits

Reviewing files that changed from the base of the PR and between deea7be and 64d23a8.

📒 Files selected for processing (1)
  • src/modules/CannedMessageModule.cpp
💤 Files with no reviewable changes (1)
  • src/modules/CannedMessageModule.cpp

📝 Walkthrough

Walkthrough

updateDestinationSelectionList() no longer allocates a meshtastic_MeshPacket or assigns its encryption and channel fields before populating active channels and adjusting selection indices.

Changes

Destination selection cleanup

Layer / File(s) Summary
Remove unused packet setup
src/modules/CannedMessageModule.cpp
Removes the unused mesh packet allocation and pki_encrypted/channel assignments from destination selection updates.

Estimated code review effort: 1 (Trivial) | ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly states the core change: removing a packet-pool leak in the canned message destination picker.
Description check ✅ Passed The description covers summary, impact, root cause, fix, and testing; only the template attestation section is omitted.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/funny-panini-4cbeb3

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thebentern
thebentern requested a review from HarukiToreda July 16, 2026 12:22
@github-actions

Copy link
Copy Markdown
Contributor

⚡ Try this PR in the Web Flasher

Note

Building this pull request… the flash button, badges and supported-board
list will appear here automatically once CI finishes.

@thebentern thebentern added the bugfix Pull request that fixes bugs label Jul 16, 2026
@thebentern
thebentern merged commit f7fd058 into develop Jul 16, 2026
87 of 90 checks passed
@thebentern
thebentern deleted the claude/funny-panini-4cbeb3 branch July 16, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Pull request that fixes bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants