From 220bb4d1865ffc7536d387f794e420060317adf2 Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Wed, 6 May 2026 15:33:59 -0500 Subject: [PATCH] Smart pointers and memory management cleanup (#10400) * Refactor memory management in Syslog and StoreForwardModule * Implement destructor for Lock * Refactor RotaryEncoder and PacketHistory to use smart pointers for better memory management * CH341 should use unique_ptr for improved memory management * Fix checks in PH * Improve Syslog::vlogf to handle variable argument lists more safely * Fix initOk method to use nullptr for null pointer check --- src/DebugConfiguration.cpp | 33 +++++++------- src/concurrency/Lock.cpp | 7 +++ src/concurrency/Lock.h | 1 + src/input/RotaryEncoderImpl.cpp | 11 ++--- src/input/RotaryEncoderImpl.h | 3 +- src/mesh/PacketHistory.cpp | 55 ++++++++++-------------- src/mesh/PacketHistory.h | 8 ++-- src/modules/StoreForwardModule.cpp | 4 +- src/modules/StoreForwardModule.h | 9 +++- src/platform/portduino/PortduinoGlue.cpp | 8 ++-- 10 files changed, 68 insertions(+), 71 deletions(-) diff --git a/src/DebugConfiguration.cpp b/src/DebugConfiguration.cpp index 207feb8c0..f83624bbf 100644 --- a/src/DebugConfiguration.cpp +++ b/src/DebugConfiguration.cpp @@ -26,6 +26,8 @@ SOFTWARE.*/ #include "DebugConfiguration.h" +#include + #ifdef ARCH_PORTDUINO #include "platform/portduino/PortduinoGlue.h" #endif @@ -119,27 +121,22 @@ bool Syslog::vlogf(uint16_t pri, const char *fmt, va_list args) bool Syslog::vlogf(uint16_t pri, const char *appName, const char *fmt, va_list args) { - char *message; - size_t initialLen; - size_t len; - bool result; + // First measure the formatted length using a copy of args; passing args directly + // to vsnprintf consumes it, and reusing a consumed va_list is undefined behavior. + va_list args_measure; + va_copy(args_measure, args); + int needed = vsnprintf(nullptr, 0, fmt, args_measure); + va_end(args_measure); - initialLen = strlen(fmt); + if (needed < 0) + return false; // encoding error - message = new char[initialLen + 1]; + auto message = std::unique_ptr(new char[static_cast(needed) + 1]); + int written = vsnprintf(message.get(), static_cast(needed) + 1, fmt, args); + if (written < 0) + return false; - len = vsnprintf(message, initialLen + 1, fmt, args); - if (len > initialLen) { - delete[] message; - message = new char[len + 1]; - - vsnprintf(message, len + 1, fmt, args); - } - - result = this->_sendLog(pri, appName, message); - - delete[] message; - return result; + return this->_sendLog(pri, appName, message.get()); } inline bool Syslog::_sendLog(uint16_t pri, const char *appName, const char *message) diff --git a/src/concurrency/Lock.cpp b/src/concurrency/Lock.cpp index 0fe80e455..4596e0edf 100644 --- a/src/concurrency/Lock.cpp +++ b/src/concurrency/Lock.cpp @@ -14,6 +14,11 @@ Lock::Lock() : handle(xSemaphoreCreateBinary()) } } +Lock::~Lock() +{ + vSemaphoreDelete(handle); +} + void Lock::lock() { if (xSemaphoreTake(handle, portMAX_DELAY) == false) { @@ -30,6 +35,8 @@ void Lock::unlock() #else Lock::Lock() {} +Lock::~Lock() {} + void Lock::lock() {} void Lock::unlock() {} diff --git a/src/concurrency/Lock.h b/src/concurrency/Lock.h index 1b9ea20d5..a51248117 100644 --- a/src/concurrency/Lock.h +++ b/src/concurrency/Lock.h @@ -12,6 +12,7 @@ class Lock { public: Lock(); + ~Lock(); Lock(const Lock &) = delete; Lock &operator=(const Lock &) = delete; diff --git a/src/input/RotaryEncoderImpl.cpp b/src/input/RotaryEncoderImpl.cpp index cc1222595..dcdbf0d36 100644 --- a/src/input/RotaryEncoderImpl.cpp +++ b/src/input/RotaryEncoderImpl.cpp @@ -13,7 +13,6 @@ RotaryEncoderImpl *rotaryEncoderImpl; RotaryEncoderImpl::RotaryEncoderImpl() { - rotary = nullptr; #ifdef ARCH_ESP32 isFirstInit = true; #endif @@ -23,11 +22,6 @@ RotaryEncoderImpl::~RotaryEncoderImpl() { LOG_DEBUG("RotaryEncoderImpl destructor"); detachRotaryEncoderInterrupts(); - - if (rotary != nullptr) { - delete rotary; - rotary = nullptr; - } } bool RotaryEncoderImpl::init() @@ -43,8 +37,9 @@ bool RotaryEncoderImpl::init() eventPressed = static_cast(moduleConfig.canned_message.inputbroker_event_press); if (rotary == nullptr) { - rotary = new RotaryEncoder(moduleConfig.canned_message.inputbroker_pin_a, moduleConfig.canned_message.inputbroker_pin_b, - moduleConfig.canned_message.inputbroker_pin_press); + rotary.reset(new RotaryEncoder(moduleConfig.canned_message.inputbroker_pin_a, + moduleConfig.canned_message.inputbroker_pin_b, + moduleConfig.canned_message.inputbroker_pin_press)); } attachRotaryEncoderInterrupts(); diff --git a/src/input/RotaryEncoderImpl.h b/src/input/RotaryEncoderImpl.h index ec8a064bd..58b4e3450 100644 --- a/src/input/RotaryEncoderImpl.h +++ b/src/input/RotaryEncoderImpl.h @@ -5,6 +5,7 @@ #include "InputBroker.h" #include "concurrency/OSThread.h" #include "mesh/NodeDB.h" +#include class RotaryEncoder; @@ -28,7 +29,7 @@ class RotaryEncoderImpl final : public InputPollable input_broker_event eventCcw = INPUT_BROKER_NONE; input_broker_event eventPressed = INPUT_BROKER_NONE; - RotaryEncoder *rotary; + std::unique_ptr rotary; private: #ifdef ARCH_ESP32 diff --git a/src/mesh/PacketHistory.cpp b/src/mesh/PacketHistory.cpp index 845a936d4..861b5fdfb 100644 --- a/src/mesh/PacketHistory.cpp +++ b/src/mesh/PacketHistory.cpp @@ -16,7 +16,7 @@ #define VERBOSE_PACKET_HISTORY 0 // Set to 1 for verbose logging, 2 for heavy debugging #define PACKET_HISTORY_TRACE_AGING 1 // Set to 1 to enable logging of the age of re/used history slots -PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0), recentPackets(NULL) // Initialize members +PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0) // Initialize members { if (size < 4 || size > PACKETHISTORY_MAX) { // Copilot suggested - makes sense LOG_WARN("Packet History - Invalid size %d, using default %d", size, PACKETHISTORY_MAX); @@ -25,7 +25,7 @@ PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0), recentPa // Allocate memory for the recent packets array recentPacketsCapacity = size; - recentPackets = new PacketRecord[recentPacketsCapacity]; + recentPackets.reset(new PacketRecord[recentPacketsCapacity]); if (!recentPackets) { // No logging here, console/log probably uninitialized yet. LOG_ERROR("Packet History - Memory allocation failed for size=%d entries / %d Bytes", size, sizeof(PacketRecord) * recentPacketsCapacity); @@ -34,14 +34,7 @@ PacketHistory::PacketHistory(uint32_t size) : recentPacketsCapacity(0), recentPa } // Initialize the recent packets array to zero - memset(recentPackets, 0, sizeof(PacketRecord) * recentPacketsCapacity); -} - -PacketHistory::~PacketHistory() -{ - recentPacketsCapacity = 0; - delete[] recentPackets; - recentPackets = NULL; + memset(recentPackets.get(), 0, sizeof(PacketRecord) * recentPacketsCapacity); } /** Update recentPackets and return true if we have already seen this packet */ @@ -205,13 +198,14 @@ PacketHistory::PacketRecord *PacketHistory::find(NodeNum sender, PacketId id) return NULL; } + PacketRecord *base = recentPackets.get(); PacketRecord *it = NULL; - for (it = recentPackets; it < (recentPackets + recentPacketsCapacity); ++it) { + for (it = base; it < (base + recentPacketsCapacity); ++it) { if (it->id == id && it->sender == sender) { #if VERBOSE_PACKET_HISTORY LOG_DEBUG("Packet History - find: s=%08x id=%08x FOUND nh=%02x rby=%02x %02x %02x age=%d slot=%d/%d", it->sender, it->id, it->next_hop, it->relayed_by[0], it->relayed_by[1], it->relayed_by[2], millis() - (it->rxTimeMsec), - it - recentPackets, recentPacketsCapacity); + it - base, recentPacketsCapacity); #endif // only the first match is returned, so be careful not to create duplicate entries return it; // Return pointer to the found record @@ -229,39 +223,38 @@ void PacketHistory::insert(const PacketRecord &r) { uint32_t now_millis = millis(); // Should not jump with time changes uint32_t OldtrxTimeMsec = 0; + PacketRecord *base = recentPackets.get(); PacketRecord *tu = NULL; // Will insert here. PacketRecord *it = NULL; // Find a free, matching or oldest used slot in the recentPackets array - for (it = recentPackets; it < (recentPackets + recentPacketsCapacity); ++it) { + for (it = base; it < (base + recentPacketsCapacity); ++it) { if (it->id == 0 && it->sender == 0 /*&& rxTimeMsec == 0*/) { // Record is empty tu = it; // Remember the free slot #if VERBOSE_PACKET_HISTORY >= 2 - LOG_DEBUG("Packet History - insert: Free slot@ %d/%d", tu - recentPackets, recentPacketsCapacity); + LOG_DEBUG("Packet History - insert: Free slot@ %d/%d", tu - base, recentPacketsCapacity); #endif // We have that, Exit the loop - it = (recentPackets + recentPacketsCapacity); + it = (base + recentPacketsCapacity); } else if (it->id == r.id && it->sender == r.sender) { // Record matches the packet we want to insert tu = it; // Remember the matching slot OldtrxTimeMsec = now_millis - it->rxTimeMsec; // ..and save current entry's age #if VERBOSE_PACKET_HISTORY >= 2 - LOG_DEBUG("Packet History - insert: Matched slot@ %d/%d age=%d", tu - recentPackets, recentPacketsCapacity, - OldtrxTimeMsec); + LOG_DEBUG("Packet History - insert: Matched slot@ %d/%d age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec); #endif // We have that, Exit the loop - it = (recentPackets + recentPacketsCapacity); + it = (base + recentPacketsCapacity); } else { if (it->rxTimeMsec == 0) { LOG_WARN( "Packet History - insert: Found packet s=%08x id=%08x with rxTimeMsec = 0, slot %d/%d. Should never happen!", - it->sender, it->id, it - recentPackets, recentPacketsCapacity); + it->sender, it->id, it - base, recentPacketsCapacity); } if ((now_millis - it->rxTimeMsec) > OldtrxTimeMsec) { // 49.7 days rollover friendly OldtrxTimeMsec = now_millis - it->rxTimeMsec; tu = it; // remember the oldest packet #if VERBOSE_PACKET_HISTORY >= 2 - LOG_DEBUG("Packet History - insert: Older slot@ %d/%d age=%d", tu - recentPackets, recentPacketsCapacity, - OldtrxTimeMsec); + LOG_DEBUG("Packet History - insert: Older slot@ %d/%d age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec); #endif } // keep looking for oldest till entire array is checked @@ -276,13 +269,11 @@ void PacketHistory::insert(const PacketRecord &r) #if VERBOSE_PACKET_HISTORY if (tu->id == 0 && tu->sender == 0) { - LOG_DEBUG("Packet History - insert: slot@ %d/%d is NEW", tu - recentPackets, recentPacketsCapacity); + LOG_DEBUG("Packet History - insert: slot@ %d/%d is NEW", tu - base, recentPacketsCapacity); } else if (tu->id == r.id && tu->sender == r.sender) { - LOG_DEBUG("Packet History - insert: slot@ %d/%d MATCHED, age=%d", tu - recentPackets, recentPacketsCapacity, - OldtrxTimeMsec); + LOG_DEBUG("Packet History - insert: slot@ %d/%d MATCHED, age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec); } else { - LOG_DEBUG("Packet History - insert: slot@ %d/%d REUSE OLDEST, age=%d", tu - recentPackets, recentPacketsCapacity, - OldtrxTimeMsec); + LOG_DEBUG("Packet History - insert: slot@ %d/%d REUSE OLDEST, age=%d", tu - base, recentPacketsCapacity, OldtrxTimeMsec); } #endif @@ -315,9 +306,9 @@ void PacketHistory::insert(const PacketRecord &r) #endif #if VERBOSE_PACKET_HISTORY - LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d BEFORE", - tu - recentPackets, recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1], - tu->relayed_by[2], tu->rxTimeMsec); + LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d BEFORE", tu - base, + recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1], tu->relayed_by[2], + tu->rxTimeMsec); #endif if (r.rxTimeMsec == 0) { @@ -330,9 +321,9 @@ void PacketHistory::insert(const PacketRecord &r) *tu = r; // store the packet #if VERBOSE_PACKET_HISTORY - LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d AFTER", - tu - recentPackets, recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1], - tu->relayed_by[2], tu->rxTimeMsec); + LOG_DEBUG("Packet History - insert: Store slot@ %d/%d s=%08x id=%08x nh=%02x rby=%02x %02x %02x rxT=%d AFTER", tu - base, + recentPacketsCapacity, tu->sender, tu->id, tu->next_hop, tu->relayed_by[0], tu->relayed_by[1], tu->relayed_by[2], + tu->rxTimeMsec); #endif } diff --git a/src/mesh/PacketHistory.h b/src/mesh/PacketHistory.h index 9b6a93280..756fa86c1 100644 --- a/src/mesh/PacketHistory.h +++ b/src/mesh/PacketHistory.h @@ -1,6 +1,7 @@ #pragma once #include "NodeDB.h" +#include // Number of relayers we keep track of. Use 6 to be efficient with memory alignment of PacketRecord to 20 bytes #define NUM_RELAYERS 6 @@ -26,7 +27,7 @@ class PacketHistory uint32_t recentPacketsCapacity = 0; // Can be set in constructor, no need to recompile. Used to allocate memory for mx_recentPackets. - PacketRecord *recentPackets = NULL; // Simple and fixed in size. Debloat. + std::unique_ptr recentPackets; // Simple and fixed in size. Debloat. /** Find a packet record in history. * @param sender NodeNum @@ -48,11 +49,8 @@ class PacketHistory uint8_t getOurTxHopLimit(const PacketRecord &r); void setOurTxHopLimit(PacketRecord &r, uint8_t hopLimit); - PacketHistory(const PacketHistory &); // non construction-copyable - PacketHistory &operator=(const PacketHistory &); // non copyable public: explicit PacketHistory(uint32_t size = -1); // Constructor with size parameter, default is PACKETHISTORY_MAX - ~PacketHistory(); /** * Update recentBroadcasts and return true if we have already seen this packet @@ -74,5 +72,5 @@ class PacketHistory void removeRelayer(const uint8_t relayer, const uint32_t id, const NodeNum sender); // To check if the PacketHistory was initialized correctly by constructor - bool initOk(void) { return recentPackets != NULL && recentPacketsCapacity != 0; } + bool initOk(void) { return recentPackets != nullptr && recentPacketsCapacity != 0; } }; diff --git a/src/modules/StoreForwardModule.cpp b/src/modules/StoreForwardModule.cpp index 6df0e18f0..0ac70acf8 100644 --- a/src/modules/StoreForwardModule.cpp +++ b/src/modules/StoreForwardModule.cpp @@ -80,9 +80,9 @@ void StoreForwardModule::populatePSRAM() (this->records ? this->records : (((memGet.getFreePsram() / 4) * 3) / sizeof(PacketHistoryStruct))); this->records = numberOfPackets; #if defined(ARCH_ESP32) - this->packetHistory = static_cast(ps_calloc(numberOfPackets, sizeof(PacketHistoryStruct))); + this->packetHistory.reset(static_cast(ps_calloc(numberOfPackets, sizeof(PacketHistoryStruct)))); #elif defined(ARCH_PORTDUINO) - this->packetHistory = static_cast(calloc(numberOfPackets, sizeof(PacketHistoryStruct))); + this->packetHistory.reset(static_cast(calloc(numberOfPackets, sizeof(PacketHistoryStruct)))); #endif diff --git a/src/modules/StoreForwardModule.h b/src/modules/StoreForwardModule.h index 148568e1b..77565b22c 100644 --- a/src/modules/StoreForwardModule.h +++ b/src/modules/StoreForwardModule.h @@ -7,6 +7,7 @@ #include "configuration.h" #include #include +#include #include struct PacketHistoryStruct { @@ -29,11 +30,17 @@ struct PacketHistoryStruct { class StoreForwardModule : private concurrency::OSThread, public ProtobufModule { + // packetHistory is allocated with ps_calloc / calloc, so it must be released with free(), + // not delete[]. + struct CFreeDeleter { + void operator()(PacketHistoryStruct *p) const noexcept { free(p); } + }; + bool busy = 0; uint32_t busyTo = 0; char routerMessage[meshtastic_Constants_DATA_PAYLOAD_LEN] = {0}; - PacketHistoryStruct *packetHistory = 0; + std::unique_ptr packetHistory; uint32_t packetHistoryTotalCount = 0; uint32_t last_time = 0; uint32_t requestCount = 0; diff --git a/src/platform/portduino/PortduinoGlue.cpp b/src/platform/portduino/PortduinoGlue.cpp index 1f59e78b5..bfa2f6efb 100644 --- a/src/platform/portduino/PortduinoGlue.cpp +++ b/src/platform/portduino/PortduinoGlue.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -297,10 +298,9 @@ void portduinoSetup() // Try CH341 try { std::cout << "autoconf: Looking for CH341 device..." << std::endl; - ch341Hal = new Ch341Hal(0, portduino_config.lora_usb_serial_num, portduino_config.lora_usb_vid, - portduino_config.lora_usb_pid); - ch341Hal->getProductString(autoconf_product, 95); - delete ch341Hal; + auto probe = std::unique_ptr(new Ch341Hal(0, portduino_config.lora_usb_serial_num, + portduino_config.lora_usb_vid, portduino_config.lora_usb_pid)); + probe->getProductString(autoconf_product, 95); std::cout << "autoconf: Found CH341 device " << autoconf_product << std::endl; found_ch341 = true;