Skip to content

Commit a96a99b

Browse files
nightjoker7nightjoker7
committed
Null-check clientNotificationPool / packetPool / mqtt pool allocations
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 (#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.
1 parent 4b49147 commit a96a99b

9 files changed

Lines changed: 86 additions & 56 deletions

File tree

‎src/main.cpp‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -909,11 +909,13 @@ void setup()
909909
if (nodeDB->keyIsLowEntropy && !nodeDB->hasWarned) {
910910
LOG_WARN(LOW_ENTROPY_WARNING);
911911
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
912-
cn->level = meshtastic_LogRecord_Level_WARNING;
913-
cn->time = getValidTime(RTCQualityFromNet);
914-
sprintf(cn->message, LOW_ENTROPY_WARNING);
915-
service->sendClientNotification(cn);
916-
nodeDB->hasWarned = true;
912+
if (cn) {
913+
cn->level = meshtastic_LogRecord_Level_WARNING;
914+
cn->time = getValidTime(RTCQualityFromNet);
915+
sprintf(cn->message, LOW_ENTROPY_WARNING);
916+
service->sendClientNotification(cn);
917+
nodeDB->hasWarned = true;
918+
}
917919
}
918920
#endif
919921
#if !MESHTASTIC_EXCLUDE_INPUTBROKER

‎src/mesh/NodeDB.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1881,10 +1881,12 @@ bool NodeDB::updateUser(uint32_t nodeId, meshtastic_User &p, uint8_t channelInde
18811881
"to regenerate your public keys.";
18821882
LOG_WARN(warning, p.long_name);
18831883
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
1884-
cn->level = meshtastic_LogRecord_Level_WARNING;
1885-
cn->time = getValidTime(RTCQualityFromNet);
1886-
sprintf(cn->message, warning, p.long_name);
1887-
service->sendClientNotification(cn);
1884+
if (cn) {
1885+
cn->level = meshtastic_LogRecord_Level_WARNING;
1886+
cn->time = getValidTime(RTCQualityFromNet);
1887+
sprintf(cn->message, warning, p.long_name);
1888+
service->sendClientNotification(cn);
1889+
}
18881890
}
18891891
return false;
18901892
}

‎src/mesh/PhoneAPI.cpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -765,6 +765,8 @@ bool PhoneAPI::available()
765765
void PhoneAPI::sendNotification(meshtastic_LogRecord_Level level, uint32_t replyId, const char *message)
766766
{
767767
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
768+
if (!cn)
769+
return;
768770
cn->has_reply_id = true;
769771
cn->reply_id = replyId;
770772
cn->level = meshtastic_LogRecord_Level_WARNING;

‎src/mesh/Router.cpp‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -325,12 +325,14 @@ ErrorCode Router::send(meshtastic_MeshPacket *p)
325325
LOG_WARN("Duty cycle limit exceeded. Aborting send for now, you can send again in %d mins", silentMinutes);
326326

327327
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
328-
cn->has_reply_id = true;
329-
cn->reply_id = p->id;
330-
cn->level = meshtastic_LogRecord_Level_WARNING;
331-
cn->time = getValidTime(RTCQualityFromNet);
332-
sprintf(cn->message, "Duty cycle limit exceeded. You can send again in %d mins", silentMinutes);
333-
service->sendClientNotification(cn);
328+
if (cn) {
329+
cn->has_reply_id = true;
330+
cn->reply_id = p->id;
331+
cn->level = meshtastic_LogRecord_Level_WARNING;
332+
cn->time = getValidTime(RTCQualityFromNet);
333+
sprintf(cn->message, "Duty cycle limit exceeded. You can send again in %d mins", silentMinutes);
334+
service->sendClientNotification(cn);
335+
}
334336

335337
meshtastic_Routing_Error err = meshtastic_Routing_Error_DUTY_CYCLE_LIMIT;
336338
if (isFromUs(p)) { // only send NAK to API, not to the mesh

‎src/modules/PositionModule.cpp‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -397,12 +397,14 @@ void PositionModule::sendOurPosition(NodeNum dest, bool wantReplies, uint8_t cha
397397
meshtastic_Config_DeviceConfig_Role_TAK_TRACKER) &&
398398
config.power.is_power_saving) {
399399
meshtastic_ClientNotification *notification = clientNotificationPool.allocZeroed();
400-
notification->level = meshtastic_LogRecord_Level_INFO;
401-
notification->time = getValidTime(RTCQualityFromNet);
402-
sprintf(notification->message, "Sending position and sleeping for %us interval in a moment",
403-
Default::getConfiguredOrDefaultMs(config.position.position_broadcast_secs, default_broadcast_interval_secs) /
404-
1000U);
405-
service->sendClientNotification(notification);
400+
if (notification) {
401+
notification->level = meshtastic_LogRecord_Level_INFO;
402+
notification->time = getValidTime(RTCQualityFromNet);
403+
sprintf(notification->message, "Sending position and sleeping for %us interval in a moment",
404+
Default::getConfiguredOrDefaultMs(config.position.position_broadcast_secs, default_broadcast_interval_secs) /
405+
1000U);
406+
service->sendClientNotification(notification);
407+
}
406408
sleepOnNextExecution = true;
407409
LOG_DEBUG("Start next execution in 5s, then sleep");
408410
setIntervalFromNow(FIVE_SECONDS_MS);

‎src/modules/SerialModule.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,12 @@ bool SerialModule::isValidConfig(const meshtastic_ModuleConfig_SerialConfig &con
9696
LOG_ERROR(warning);
9797
#if !IS_RUNNING_TESTS
9898
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
99-
cn->level = meshtastic_LogRecord_Level_ERROR;
100-
cn->time = getValidTime(RTCQualityFromNet);
101-
snprintf(cn->message, sizeof(cn->message), "%s", warning);
102-
service->sendClientNotification(cn);
99+
if (cn) {
100+
cn->level = meshtastic_LogRecord_Level_ERROR;
101+
cn->time = getValidTime(RTCQualityFromNet);
102+
snprintf(cn->message, sizeof(cn->message), "%s", warning);
103+
service->sendClientNotification(cn);
104+
}
103105
#endif
104106
return false;
105107
}

‎src/modules/Telemetry/AirQualityTelemetry.cpp‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -416,27 +416,31 @@ bool AirQualityTelemetryModule::sendTelemetry(NodeNum dest, bool phoneOnly)
416416

417417
if (config.device.role == meshtastic_Config_DeviceConfig_Role_SENSOR && config.power.is_power_saving) {
418418
meshtastic_ClientNotification *notification = clientNotificationPool.allocZeroed();
419-
notification->level = meshtastic_LogRecord_Level_INFO;
420-
notification->time = getValidTime(RTCQualityFromNet);
421-
sprintf(notification->message, "Sending telemetry and sleeping for %us interval in a moment",
422-
Default::getConfiguredOrDefaultMs(moduleConfig.telemetry.air_quality_interval,
423-
default_telemetry_broadcast_interval_secs) /
424-
1000U);
425-
service->sendClientNotification(notification);
419+
if (notification) {
420+
notification->level = meshtastic_LogRecord_Level_INFO;
421+
notification->time = getValidTime(RTCQualityFromNet);
422+
sprintf(notification->message, "Sending telemetry and sleeping for %us interval in a moment",
423+
Default::getConfiguredOrDefaultMs(moduleConfig.telemetry.air_quality_interval,
424+
default_telemetry_broadcast_interval_secs) /
425+
1000U);
426+
service->sendClientNotification(notification);
427+
}
426428
sleepOnNextExecution = true;
427429
LOG_DEBUG("Start next execution in 5s, then sleep");
428430
setIntervalFromNow(FIVE_SECONDS_MS);
429431
}
430432

431433
if (config.device.role == meshtastic_Config_DeviceConfig_Role_SENSOR && config.power.is_power_saving) {
432434
meshtastic_ClientNotification *notification = clientNotificationPool.allocZeroed();
433-
notification->level = meshtastic_LogRecord_Level_INFO;
434-
notification->time = getValidTime(RTCQualityFromNet);
435-
sprintf(notification->message, "Sending telemetry and sleeping for %us interval in a moment",
436-
Default::getConfiguredOrDefaultMs(moduleConfig.telemetry.air_quality_interval,
437-
default_telemetry_broadcast_interval_secs) /
438-
1000U);
439-
service->sendClientNotification(notification);
435+
if (notification) {
436+
notification->level = meshtastic_LogRecord_Level_INFO;
437+
notification->time = getValidTime(RTCQualityFromNet);
438+
sprintf(notification->message, "Sending telemetry and sleeping for %us interval in a moment",
439+
Default::getConfiguredOrDefaultMs(moduleConfig.telemetry.air_quality_interval,
440+
default_telemetry_broadcast_interval_secs) /
441+
1000U);
442+
service->sendClientNotification(notification);
443+
}
440444
sleepOnNextExecution = true;
441445
LOG_DEBUG("Start next execution in 5s, then sleep");
442446
setIntervalFromNow(FIVE_SECONDS_MS);

‎src/modules/Telemetry/EnvironmentTelemetry.cpp‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -644,13 +644,15 @@ bool EnvironmentTelemetryModule::sendTelemetry(NodeNum dest, bool phoneOnly)
644644

645645
if (config.device.role == meshtastic_Config_DeviceConfig_Role_SENSOR && config.power.is_power_saving) {
646646
meshtastic_ClientNotification *notification = clientNotificationPool.allocZeroed();
647-
notification->level = meshtastic_LogRecord_Level_INFO;
648-
notification->time = getValidTime(RTCQualityFromNet);
649-
sprintf(notification->message, "Sending telemetry and sleeping for %us interval in a moment",
650-
Default::getConfiguredOrDefaultMs(moduleConfig.telemetry.environment_update_interval,
651-
default_telemetry_broadcast_interval_secs) /
652-
1000U);
653-
service->sendClientNotification(notification);
647+
if (notification) {
648+
notification->level = meshtastic_LogRecord_Level_INFO;
649+
notification->time = getValidTime(RTCQualityFromNet);
650+
sprintf(notification->message, "Sending telemetry and sleeping for %us interval in a moment",
651+
Default::getConfiguredOrDefaultMs(moduleConfig.telemetry.environment_update_interval,
652+
default_telemetry_broadcast_interval_secs) /
653+
1000U);
654+
service->sendClientNotification(notification);
655+
}
654656
sleepOnNextExecution = true;
655657
LOG_DEBUG("Start next execution in 5s, then sleep");
656658
setIntervalFromNow(FIVE_SECONDS_MS);

‎src/mqtt/MQTT.cpp‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -482,6 +482,8 @@ bool MQTT::publish(const char *topic, const char *payload, bool retained)
482482
{
483483
if (moduleConfig.mqtt.proxy_to_client_enabled) {
484484
meshtastic_MqttClientProxyMessage *msg = mqttClientProxyMessagePool.allocZeroed();
485+
if (!msg)
486+
return false;
485487
msg->which_payload_variant = meshtastic_MqttClientProxyMessage_text_tag;
486488
strncpy(msg->topic, topic, sizeof(msg->topic));
487489
msg->topic[sizeof(msg->topic) - 1] = '\0';
@@ -503,6 +505,8 @@ bool MQTT::publish(const char *topic, const uint8_t *payload, size_t length, boo
503505
{
504506
if (moduleConfig.mqtt.proxy_to_client_enabled) {
505507
meshtastic_MqttClientProxyMessage *msg = mqttClientProxyMessagePool.allocZeroed();
508+
if (!msg)
509+
return false;
506510
msg->which_payload_variant = meshtastic_MqttClientProxyMessage_data_tag;
507511
strncpy(msg->topic, topic, sizeof(msg->topic));
508512
msg->topic[sizeof(msg->topic) - 1] = '\0'; // Ensure null termination
@@ -685,11 +689,13 @@ bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTC
685689
LOG_ERROR(warning);
686690
#if !IS_RUNNING_TESTS
687691
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
688-
cn->level = meshtastic_LogRecord_Level_ERROR;
689-
cn->time = getValidTime(RTCQualityFromNet);
690-
strncpy(cn->message, warning, sizeof(cn->message) - 1);
691-
cn->message[sizeof(cn->message) - 1] = '\0'; // Ensure null termination
692-
service->sendClientNotification(cn);
692+
if (cn) {
693+
cn->level = meshtastic_LogRecord_Level_ERROR;
694+
cn->time = getValidTime(RTCQualityFromNet);
695+
strncpy(cn->message, warning, sizeof(cn->message) - 1);
696+
cn->message[sizeof(cn->message) - 1] = '\0'; // Ensure null termination
697+
service->sendClientNotification(cn);
698+
}
693699
#endif
694700
return false;
695701
#endif
@@ -701,11 +707,13 @@ bool MQTT::isValidConfig(const meshtastic_ModuleConfig_MQTTConfig &config, MQTTC
701707
LOG_ERROR(warning);
702708
#if !IS_RUNNING_TESTS
703709
meshtastic_ClientNotification *cn = clientNotificationPool.allocZeroed();
704-
cn->level = meshtastic_LogRecord_Level_ERROR;
705-
cn->time = getValidTime(RTCQualityFromNet);
706-
strncpy(cn->message, warning, sizeof(cn->message) - 1);
707-
cn->message[sizeof(cn->message) - 1] = '\0'; // Ensure null termination
708-
service->sendClientNotification(cn);
710+
if (cn) {
711+
cn->level = meshtastic_LogRecord_Level_ERROR;
712+
cn->time = getValidTime(RTCQualityFromNet);
713+
strncpy(cn->message, warning, sizeof(cn->message) - 1);
714+
cn->message[sizeof(cn->message) - 1] = '\0'; // Ensure null termination
715+
service->sendClientNotification(cn);
716+
}
709717
#endif
710718
return false;
711719
}
@@ -878,6 +886,10 @@ void MQTT::perhapsReportToMap()
878886

879887
// Allocate MeshPacket and fill it
880888
meshtastic_MeshPacket *mp = packetPool.allocZeroed();
889+
if (!mp) {
890+
LOG_WARN("MQTT Map report: packet pool exhausted");
891+
return;
892+
}
881893
mp->which_payload_variant = meshtastic_MeshPacket_decoded_tag;
882894
mp->from = nodeDB->getNodeNum();
883895
mp->to = NODENUM_BROADCAST;

0 commit comments

Comments
 (0)