Null-check pool.allocZeroed() at 13 sites across 9 files by nightjoker7 · Pull Request #10262 · meshtastic/firmware

nightjoker7 · 2026-04-23T03:06:24Z

Summary

13 sites across 9 files called `pool.allocZeroed()` and immediately dereferenced the result without null-checking. When the static MemoryPool is exhausted, `allocZeroed()` returns nullptr (with a LOG_WARN in Allocator::allocZeroed) — every dereference that follows is a crash.

From MemoryPool.h:149-160: static pools return nullptr when no free slot is available. That's a real failure mode under burst load.

Sites fixed

File	Line	Pool	Trigger
`main.cpp`	911	clientNotification	Low-entropy key warning on boot
`mesh/NodeDB.cpp`	1883	clientNotification	Duplicate public key warning
`mesh/PhoneAPI.cpp`	767	clientNotification	`sendNotification()` helper
`mesh/Router.cpp`	327	clientNotification	Duty-cycle-exceeded notification
`modules/PositionModule.cpp`	399	clientNotification	Tracker role power-save notify
`modules/SerialModule.cpp`	98	clientNotification	Invalid serial config
`modules/Telemetry/AirQualityTelemetry.cpp`	418, 432	clientNotification	Sensor role power-save notify (2 sites)
`modules/Telemetry/EnvironmentTelemetry.cpp`	646	clientNotification	Sensor role power-save notify
`mqtt/MQTT.cpp`	484, 505	mqttClientProxyMessage	MQTT publish proxy (2 sites)
`mqtt/MQTT.cpp`	687, 703	clientNotification	Invalid-config notifications (2 sites)
`mqtt/MQTT.cpp`	888	packet	Map report MeshPacket alloc

Style

Matches the existing pattern used in already-null-checked sites:

AdminModule::sendWarning: `if (!cn) return;`
RadioInterface::sendErrorNotification: `if (!cn) return;`
MQTT.cpp:671-678: `if (cn) { ... }`

Used `if (!cn) return;` for single-exit contexts and `if (cn) { ... }` where the surrounding code needs to run regardless (eg. `sleepOnNextExecution = true;` still happens in the telemetry modules even when the notification can't be sent).

Fifteen sites across nine files called pool.allocZeroed() and immediately dereferenced the result without null-checking. On static MemoryPool-backed pools, allocZeroed() returns nullptr when slots are exhausted (with a LOG_WARN) — every dereference crashes. Sites fixed: - main.cpp:911 — low-entropy key warning on boot - mesh/NodeDB.cpp:1883 — duplicate-public-key warning - mesh/PhoneAPI.cpp:767 — sendNotification helper - mesh/Router.cpp:327 — duty-cycle-exceeded notification - modules/PositionModule.cpp:399 — tracker power-save notify - modules/SerialModule.cpp:98 — invalid serial config notify - modules/Telemetry/AirQualityTelemetry.cpp:418,432 — sensor sleep notify (2 sites) - modules/Telemetry/EnvironmentTelemetry.cpp:646 — sensor sleep notify - mqtt/MQTT.cpp:484,505 — MQTT publish proxy message (2 sites) - mqtt/MQTT.cpp:687,703 — invalid-config client notifications (2 sites) - mqtt/MQTT.cpp:888 — map report MeshPacket alloc Sister PRs in flight (meshtastic#10261 covers Router::allocForSending) harden the same allocation class on different call sites. Matching the existing style: sites that already null-check (AdminModule::sendWarning, RadioInterface::sendErrorNotification, MQTT.cpp:671) use the same `if (cn) { ... }` or `if (!cn) return;` pattern now applied uniformly.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds defensive null-checks around pool.allocZeroed() call sites to prevent null dereferences when static pools are exhausted under load.

Changes:

Guard allocZeroed() results before dereferencing across MQTT, telemetry, routing, and notification paths
In some paths, early-return or skip sending a notification when allocation fails
Add a warning log when packetPool is exhausted during MQTT map reporting

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/mqtt/MQTT.cpp	Adds null checks for proxy messages, config notifications, and map-report packet allocation
src/modules/Telemetry/EnvironmentTelemetry.cpp	Skips client notification send when allocation fails while still scheduling sleep behavior
src/modules/Telemetry/AirQualityTelemetry.cpp	Skips client notification send when allocation fails (two similar sites)
src/modules/SerialModule.cpp	Guards client notification allocation in invalid-config path
src/modules/PositionModule.cpp	Guards client notification allocation in power-save tracker notify path
src/mesh/Router.cpp	Guards notification allocation on duty-cycle exceeded path
src/mesh/PhoneAPI.cpp	Adds early return if notification allocation fails
src/mesh/NodeDB.cpp	Guards notification allocation for duplicate public key warning
src/main.cpp	Guards notification allocation for low-entropy key warning

…ation-null-checks

@thebentern

Five issues raised by @thebentern and @Copilot: 1. **MQTT map report alloc failure (thebentern, MQTT.cpp:890)**: changed LOG_WARN to LOG_ERROR — map reports aren't retried, so losing one on alloc failure is worth error-level visibility. 2. **MQTT proxy publish silently drops (Copilot, MQTT.cpp:486+509)**: added LOG_WARN on pool exhaustion so operators can see why a publish failed instead of just getting a silent false return. 3. **Low-entropy warning spin (Copilot, main.cpp:918)**: nodeDB->hasWarned was only set on allocZeroed() success, so if the pool was full we'd retry every tick and re-LOG_WARN forever. Set unconditionally — the LOG_WARN is what matters for visibility, the notification is best-effort. 4. **Level parameter ignored (Copilot, PhoneAPI.cpp:772)**: sendNotification took a level parameter but hardcoded cn->level = WARNING. Honor the caller's level. Also switched strncpy to NUL-terminating form because meshtastic_ClientNotification.message may not be NUL-terminated if the source string fills the buffer. 5. **sprintf overflow (Copilot, NodeDB.cpp:1887)**: the public-key-duplicate warning used sprintf with a remote-controlled p.long_name (up to 40 bytes) formatted into a ~200-byte warning template. Switched to snprintf against sizeof(cn->message).

nightjoker7 · 2026-04-23T21:17:24Z

Addressed all 5 review points in 055bf7b52: map-report LOG_ERROR, proxy-publish LOG_WARN, hasWarned unconditional, level-parameter honored, sprintf→snprintf for public-key warning. See commit message for per-issue detail.

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Copilot · 2026-04-23T21:54:37Z

    // Allocate MeshPacket and fill it
    meshtastic_MeshPacket *mp = packetPool.allocZeroed();
+    if (!mp) {
+        LOG_ERROR("MQTT Map report: packet pool exhausted");


On packetPool exhaustion this returns immediately without updating last_report_to_map, so perhapsReportToMap() will attempt again on every runOnce() iteration (20–200ms) and can spam logs while the pool remains exhausted. Consider throttling this error path (e.g., set last_report_to_map or a separate last_map_report_alloc_fail timestamp) so failures back off to the configured publish interval.

Suggested change

LOG_ERROR("MQTT Map report: packet pool exhausted");

LOG_ERROR("MQTT Map report: packet pool exhausted");

last_report_to_map = millis();

Copilot · 2026-04-23T21:54:37Z

+        // notice; if the ClientNotification pool is exhausted we'd otherwise spin every tick
+        // retrying + spamming LOG_WARN.


This comment says we’d otherwise “spin every tick retrying”, but this check runs in setup() (single-shot). Consider rewording to reflect the actual behavior (e.g., avoiding repeated warnings across restarts / multiple setup invocations) or remove the “every tick” phrasing to prevent confusion.

Suggested change

// notice; if the ClientNotification pool is exhausted we'd otherwise spin every tick

// retrying + spamming LOG_WARN.

// notice; if the ClientNotification pool is exhausted we should not retry on a later

// setup invocation and emit the same LOG_WARN again.

Copilot · 2026-04-23T21:54:37Z

+        if (!msg) {
+            LOG_WARN("MQTT proxy publish skipped: message pool exhausted");
+            return false;
+        }


allocZeroed()/MemoryPool already emits warnings when the pool is exhausted, so this additional LOG_WARN can double/triple-log and flood output when publishes are frequent. Prefer relying on the allocator log, lowering severity, or adding throttling.

…+ reword setup comment Three line-item nits from Copilot's post-fixup pass on meshtastic#10262: 1. perhapsReportToMap() pool-exhaustion path: didn't update last_report_to_map, so the caller in runOnce() would retry every 20-200 ms while the pool stayed exhausted, turning sustained pool pressure into a log-flood. Back off to the configured publish interval by stamping last_report_to_map = millis() on allocation failure. 2. MQTT::publish() proxy overloads: MemoryPool::alloc already LOG_WARNs on exhaustion, so the caller-side LOG_WARN("MQTT proxy publish skipped: message pool exhausted") was double-logging under the same stress that triggers the allocator's own warn. Drop both (char* and uint8_t* overloads) and rely on the allocator log. 3. main.cpp low-entropy-key warn block: the comment said the unconditional hasWarned assignment prevented spinning 'every tick', but setup() is single-shot. Reword to reflect the actual behavior: hasWarned persists in NodeDB so the warning is suppressed across subsequent boots.

github-actions Bot added needs-review Needs human review bugfix Pull request that fixes bugs labels Apr 23, 2026

Merge branch 'develop' into fix/client-notification-null-checks

bb899f4

thebentern requested a review from Copilot April 23, 2026 11:16

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread src/mqtt/MQTT.cpp

Comment thread src/main.cpp

Comment thread src/mesh/PhoneAPI.cpp Outdated

Comment thread src/mesh/NodeDB.cpp

thebentern reviewed Apr 23, 2026

View reviewed changes

Comment thread src/mqtt/MQTT.cpp Outdated

Copilot started reviewing on behalf of thebentern April 23, 2026 11:32 View session

nightjoker7 added 2 commits April 23, 2026 15:56

Merge remote-tracking branch 'origin/develop' into fix/client-notific…

1d1713a

…ation-null-checks

thebentern requested a review from Copilot April 23, 2026 21:46

Copilot started reviewing on behalf of thebentern April 23, 2026 21:51 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

nightjoker7 mentioned this pull request Apr 27, 2026

fix: null-check packetPool / queueStatusPool allocCopy across 11 router/module callers #10316

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Null-check pool.allocZeroed() at 13 sites across 9 files#10262

Null-check pool.allocZeroed() at 13 sites across 9 files#10262
nightjoker7 wants to merge 5 commits intomeshtastic:developfrom
nightjoker7:fix/client-notification-null-checks

nightjoker7 commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nightjoker7 commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Copilot AI Apr 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	LOG_ERROR("MQTT Map report: packet pool exhausted");
	LOG_ERROR("MQTT Map report: packet pool exhausted");
	last_report_to_map = millis();

		// notice; if the ClientNotification pool is exhausted we'd otherwise spin every tick
		// retrying + spamming LOG_WARN.

Uh oh!

Conversation

nightjoker7 commented Apr 23, 2026

Summary

Sites fixed

Style

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nightjoker7 commented Apr 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants