Null-check packet allocations in allocForSending and its callers - #11086
Conversation
📝 WalkthroughWalkthroughChangesThe PR adds null checks across packet allocation primitives, shared packet builders, administrative reply handlers, telemetry paths, and mesh module send paths. Failed allocations now return safely, with detection-message paths also releasing temporary buffers before returning. Packet allocation safety
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mesh/MeshModule.cpp`:
- Around line 59-61: Update every caller of allocAckNak() to handle a nullptr
result before accessing or passing the packet onward. In
RoutingModule::sendAckNak(), check p before dereferencing p->want_ack; in
allocErrorResponse(), check the allocation result before calling setReplyTo(),
preserving the existing failure behavior where appropriate.
In `@src/mesh/ProtobufModule.h`:
- Around line 47-48: Update StoreForwardModule::sendMessage() to check whether
allocDataProtobuf(payload) returned nullptr before dereferencing p or accessing
packet fields, and return early on allocation failure while preserving the
existing send flow for valid packets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a9b6c05d-5301-4d73-9b1e-c6f0f833ec2d
📒 Files selected for processing (18)
src/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/mesh/MeshModule.cppsrc/mesh/ProtobufModule.hsrc/mesh/Router.cppsrc/mesh/SinglePortModule.hsrc/modules/CannedMessageModule.cppsrc/modules/DetectionSensorModule.cppsrc/modules/DropzoneModule.cppsrc/modules/PositionModule.cppsrc/modules/RangeTestModule.cppsrc/modules/ReplyBotModule.cppsrc/modules/ReplyModule.cppsrc/modules/SerialModule.cppsrc/modules/SerialModule.hsrc/modules/StatusMessageModule.cppsrc/modules/StoreForwardModule.cppsrc/modules/esp32/AudioModule.cppsrc/modules/games/GamesModule.cpp
| meshtastic_MeshPacket *p = router->allocForSending(); | ||
| if (!p) | ||
| return nullptr; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Propagate the nullable result to every allocAckNak() caller.
This guard makes allocAckNak() return nullptr, but RoutingModule::sendAckNak() immediately dereferences p->want_ack, and allocErrorResponse() passes the result to setReplyTo() without checking it. Add failure handling at both callers before they touch the packet; otherwise the allocation-failure fix still leaves a crash path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mesh/MeshModule.cpp` around lines 59 - 61, Update every caller of
allocAckNak() to handle a nullptr result before accessing or passing the packet
onward. In RoutingModule::sendAckNak(), check p before dereferencing
p->want_ack; in allocErrorResponse(), check the allocation result before calling
setReplyTo(), preserving the existing failure behavior where appropriate.
Second tier of the same nullable contract.
|
Both findings were valid and are fixed. Rather than patch the two reported sites, I computed the transitive closure of functions that can now return That surfaced 23 unguarded dereference sites at the second tier, including the two reported. All are now guarded: AdminModule x8, the seven Telemetry senders, PaxcounterModule, KeyVerificationModule x2, NeighborInfoModule, RoutingModule, StoreForwardModule, MQTT. One reported site needed no change. At AdminModule.cpp:719 the only dereference is already guarded by Totals across both commits: 55 call sites guarded, 0 unguarded dereferences remaining. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/Telemetry/AirQualityTelemetry.cpp`:
- Around line 466-468: Preserve the false result on packet allocation failure
without returning before the power-saving sleep-scheduling blocks: in
src/modules/Telemetry/AirQualityTelemetry.cpp lines 466-468,
src/modules/Telemetry/EnvironmentTelemetry.cpp lines 663-665,
src/modules/Telemetry/HealthTelemetry.cpp lines 250-252, and
src/modules/Telemetry/PowerTelemetry.cpp lines 277-279, track telemetry-send
failure and skip only packet setup/sending so each function still reaches its
existing five-second wake/sleep scheduling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dc69ee1-62e7-430d-9ec1-a90d25c465c1
📒 Files selected for processing (13)
src/modules/AdminModule.cppsrc/modules/KeyVerificationModule.cppsrc/modules/NeighborInfoModule.cppsrc/modules/RoutingModule.cppsrc/modules/StoreForwardModule.cppsrc/modules/Telemetry/AirQualityTelemetry.cppsrc/modules/Telemetry/DeviceTelemetry.cppsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/HealthTelemetry.cppsrc/modules/Telemetry/HostMetrics.cppsrc/modules/Telemetry/PowerTelemetry.cppsrc/modules/esp32/PaxcounterModule.cppsrc/mqtt/MQTT.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/StoreForwardModule.cpp
| meshtastic_MeshPacket *p = allocDataProtobuf(m); | ||
| if (!p) | ||
| return false; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not return before power-saving sleep is scheduled.
When allocation fails, these early returns skip sleepOnNextExecution and the five-second wake/sleep scheduling for power-saving SENSOR nodes, leaving them awake until the next telemetry interval. Preserve the false result while continuing through the sleep-scheduling block—for example, mark the telemetry send as failed and skip only packet setup/sending.
src/modules/Telemetry/AirQualityTelemetry.cpp#L466-L468: avoid returning before lines 506-513 execute.src/modules/Telemetry/EnvironmentTelemetry.cpp#L663-L665: avoid returning before lines 702-708 execute.src/modules/Telemetry/HealthTelemetry.cpp#L250-L252: avoid returning before lines 276-282 execute.src/modules/Telemetry/PowerTelemetry.cpp#L277-L279: avoid returning before lines 303-309 execute.
📍 Affects 4 files
src/modules/Telemetry/AirQualityTelemetry.cpp#L466-L468(this comment)src/modules/Telemetry/EnvironmentTelemetry.cpp#L663-L665src/modules/Telemetry/HealthTelemetry.cpp#L250-L252src/modules/Telemetry/PowerTelemetry.cpp#L277-L279
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/Telemetry/AirQualityTelemetry.cpp` around lines 466 - 468,
Preserve the false result on packet allocation failure without returning before
the power-saving sleep-scheduling blocks: in
src/modules/Telemetry/AirQualityTelemetry.cpp lines 466-468,
src/modules/Telemetry/EnvironmentTelemetry.cpp lines 663-665,
src/modules/Telemetry/HealthTelemetry.cpp lines 250-252, and
src/modules/Telemetry/PowerTelemetry.cpp lines 277-279, track telemetry-send
failure and skip only packet setup/sending so each function still reaches its
existing five-second wake/sleep scheduling logic.
Allocation failure now marks the telemetry invalid instead of returning early, so power-saving SENSOR nodes still arm deep sleep.
|
Valid, and fixed in 5076999. The early Scope checked: only these four senders have the sleep mechanism. DeviceTelemetry, HostMetrics and PaxcounterModule have no such block, so their early returns are left as they are.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/Telemetry/AirQualityTelemetry.cpp (1)
494-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
snprintfinstead ofsprintfto prevent potential buffer overflows.The code uses
sprintfto format the notification message, which writes to a fixed-size buffer without bounds checking. Even though the format string is relatively short, it is safer and standard practice to usesnprintfwithsizeofto ensure the buffer bounds are strictly respected.
src/modules/Telemetry/AirQualityTelemetry.cpp#L494-L497: Replacesprintf(notification->message, ...)withsnprintf(notification->message, sizeof(notification->message), ...).src/modules/Telemetry/EnvironmentTelemetry.cpp#L690-L693: Replacesprintf(notification->message, ...)withsnprintf(notification->message, sizeof(notification->message), ...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/Telemetry/AirQualityTelemetry.cpp` around lines 494 - 497, Replace sprintf with bounded snprintf for notification->message in AirQualityTelemetry.cpp lines 494-497 and EnvironmentTelemetry.cpp lines 690-693, passing sizeof(notification->message) as the buffer size while preserving the existing format and arguments.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/modules/Telemetry/AirQualityTelemetry.cpp`:
- Around line 494-497: Replace sprintf with bounded snprintf for
notification->message in AirQualityTelemetry.cpp lines 494-497 and
EnvironmentTelemetry.cpp lines 690-693, passing sizeof(notification->message) as
the buffer size while preserving the existing format and arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f4ce811a-068b-41cb-9676-cecb3bcb35a0
📒 Files selected for processing (4)
src/modules/Telemetry/AirQualityTelemetry.cppsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/HealthTelemetry.cppsrc/modules/Telemetry/PowerTelemetry.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/Telemetry/HealthTelemetry.cpp
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (30)
Build artifacts expire on 2026-08-19. Updated for |
2 participants
Router::allocForSending()dereferencedpacketPool.allocZeroed()without a null check. Sibling of 0ae44d7.The pool is heap-backed on ARCH_STM32WL and BOARD_HAS_PSRAM, so allocation can fail. Guarding
allocForSending()alone moves the deref to its callers, so theallocDataPacket()wrappers and all unguarded call sites are guarded too. 31 call sites: 29 guarded, 2 return the pointer directly.Summary by CodeRabbit