From fe15786dc10c02bacb87546e4fd21a4532f5084a Mon Sep 17 00:00:00 2001 From: Ben Meadors Date: Tue, 18 Aug 2026 18:15:02 +0000 Subject: [PATCH] fix(api): stop rebooting ESP32 nodes when a client connects to a fragmented heap (#11537) * fix(api): stop rebooting ESP32 nodes when a client connects to a fragmented heap Connecting a client to an ESP32 node over WiFi/TCP rebooted the node. Two allocations on the accept + config path use operator new, and on ESP32 that is fatal when it fails: the framework builds with CONFIG_COMPILER_CXX_EXCEPTIONS=n (esp32-common.ini), and ESP-IDF's cxx component then --wraps __cxa_throw and every unwinder entry point straight to abort(). libstdc++'s operator new throws std::bad_alloc on a NULL from malloc, so any new that cannot get its block is a reboot with no chance to recover. Both hit on a Meshnology W12 running develop c308d0a (no PSRAM detected, WiFi + HTTPS + TLS up, ~83 KB free heap, fragmented): 1. PhoneAPI::handleStartConfig -> getFiles() -> filenames.reserve(64) 64 * sizeof(meshtastic_FileInfo) = 14,848 B contiguous, requested with the SPI lock held, on the very first client handshake. The try/catch around it (from #10778) is dead code on this platform for the reason above. abort() was called at PC 0x4216f733 on core 1 __cxa_throw / operator new std::vector<_meshtastic_FileInfo>::reserve (getFiles, FSCommon.cpp:275) PhoneAPI::handleStartConfig (PhoneAPI.cpp:325) StreamAPI::readStream / ServerAPI::runOnce 2. APIServerPort::runOnce -> openAPI.reset(new T(client)) sizeof(WiFiServerAPI) is 4,512 B (stream rx/tx buffers + FromRadio/ToRadio scratch). Under a little more pressure - a few TCP sockets held open on 80/4403 plus pending TLS handshakes - the accept itself aborts, before the manifest is ever reached: abort() was called at PC 0x4216f66b on core 1 __cxa_throw / operator new APIServerPort::runOnce (ServerAPI.cpp:120) new (std::nothrow) is not the answer on this platform. libstdc++ implements it as `try { return operator new(sz); } catch (...) { return nullptr; }` (new_opnt.cc:39; objdump shows call8 to the throwing form then __cxa_begin_catch), so with the unwinder wrapped to abort() it aborts one frame deeper - verified by decoding exactly that. malloc() does return NULL here (HEAP_ABORT_WHEN_ALLOCATION_FAILS is off), so both fixes go through it: - getFiles(): size the reservation to what the allocator can actually give, and never let reserve() be the thing that finds out there is no room. On ESP32 ask heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT) - the capability heap_caps_malloc_default() (what new resolves to) falls back to across every region - less a 1 KB margin, divided by sizeof(FileInfo). Nothing is freed before the reserve, so no hole to lose to another task. Elsewhere, probe with malloc() and halve until it fits. The walk is capped at the reserved count so push_back() never grows the vector, and wasLimited reports the truncation exactly as it did for the 64-entry cap. The manifest degrades to fewer entries; the handshake completes. - APIServerPort::runOnce(): take the ServerAPI's block from malloc(), construct it in place, and hold it in a unique_ptr whose deleter runs ~T() and free()s. If there is no room, log and drop that client instead of the node; it retries and the next accept gets a fresh look at the heap. The ServerAPI/PhoneAPI/OSThread constructors do not allocate (default-constructed containers, fixed-size thread table), so nothing inside the placement new can throw either. malloc()'s alignment is the one operator new gives (it calls malloc), so the object is well-formed. Also: the two manifest LOG lines used %zu, which newlib-nano's vsnprintf on ESP32 does not know - they printed "Got zu files in manifest". Cast to unsigned like the rest of the file. Not in this PR, flagged for discussion: every other operator new / container growth in the image has the same failure mode on ESP32, and so does every try/catch in firmware source. A project-wide nothrow global operator new (returning nullptr per the platform's own -fno-exceptions contract) would close the class, but it changes semantics for every library in the image and moves the failure from a clean abort-with-backtrace at the alloc site to whatever the caller does with a nullptr. That is a policy call, not a bug fix. Verified on the W12 (Endor AP): before, the first TCP-API connect aborts; after, 6/6 connects complete full config sends, 3/3 under held-socket + TLS pressure, node never reboots. Both degraded branches driven deliberately with a verify-only heap starvation build: largest block pinned at 7.4 KB gives "reserved=27 of 64 ... (limited to 64 entries/depth 3)" and the handshake runs; pinned at 2.8 KB gives "No heap for API connection (4512 bytes), dropping client" three times with no reboot, where the std::nothrow version aborted three times. test_fscommon_getfiles 8/8 on native-macos; full native suite green in Docker. * fix(api): cap the manifest probe count; suppress cppcheck's placement-new memleak - getFiles(): cap reservedCount at filenames.max_size() before the byte-count multiply in the portable probe. A huge maxCount could wrap reservedCount * sizeof(FileInfo), let malloc() succeed on the wrapped size, and then hand reserve() the original count - a length_error, which on ESP32 is the abort this change exists to remove. max_size() is also exactly the bound reserve() would reject, so one comparison covers both. (CodeRabbit) - APIServerPort::runOnce(): cppcheck 2.20 reports "Memory leak: block" at the end of the accept scope because it does not model ownership passing through placement new into openAPI (MallocDeleter frees it). Inline-suppress with the reason, per the tree's convention. pio check -e rak3172 goes FAILED -> PASSED; every check job in CI was red on only this finding while all builds passed. --- src/FSCommon.cpp | 54 ++++++++++++++++++++++++++++++-------- src/mesh/PhoneAPI.cpp | 6 ++--- src/mesh/api/ServerAPI.cpp | 19 +++++++++++++- src/mesh/api/ServerAPI.h | 18 ++++++++++++- 4 files changed, 81 insertions(+), 16 deletions(-) diff --git a/src/FSCommon.cpp b/src/FSCommon.cpp index c00b07684b..ef0d5841ad 100644 --- a/src/FSCommon.cpp +++ b/src/FSCommon.cpp @@ -129,10 +129,13 @@ bool renameFile(const char *pathFrom, const char *pathTo) #endif } +#include +#include #include -#include -#include #include +#ifdef ARCH_ESP32 +#include +#endif /** * @brief Platform-agnostic filesystem format / wipe. @@ -250,6 +253,12 @@ void collectFiles(const char *dirname, uint8_t levels, size_t maxCount, std::vec } // namespace #endif +#ifdef ARCH_ESP32 +// Headroom kept below the allocator's largest free block when sizing the manifest: the block reported +// includes the allocator's own bookkeeping, and other tasks keep allocating while the SPI lock is held. +static constexpr size_t FILES_MANIFEST_HEAP_MARGIN = 1024; +#endif + /** * @brief Get the list of files in a directory. * @@ -268,18 +277,41 @@ std::vector getFiles(const char *dirname, uint8_t levels, s if (wasLimited) *wasLimited = false; #ifdef FSCom -#if defined(__cpp_exceptions) || defined(__EXCEPTIONS) - size_t reservedCount = maxCount; + // Size the vector once, up front, to what the heap can actually hand out, and cap the walk at that + // count so push_back() never has to grow it. Any allocation that fails here goes through operator + // new and raises std::bad_alloc; the ESP32 framework is built with CONFIG_COMPILER_CXX_EXCEPTIONS=n, + // so there is no unwinder and a throw is std::terminate() -> abort() -> reboot. That fires on the + // very first client handshake whenever the heap is fragmented (WiFi + TLS up, no PSRAM), which is + // exactly when this runs. So: never let reserve() be the thing that discovers there is no room. + // Cap at what a vector of FileInfo can hold at all: it keeps the probe's byte count from wrapping + // for a huge maxCount, and it is also the bound reserve() would otherwise reject with a throw. + size_t reservedCount = std::min(maxCount, filenames.max_size()); +#ifdef ARCH_ESP32 + // Ask the allocator for the largest contiguous block malloc() could hand out. MALLOC_CAP_DEFAULT + // is the capability heap_caps_malloc_default() (what operator new resolves to) falls back to + // across every region, internal and PSRAM alike, so this is the "will new succeed" question + // asked directly. Nothing is freed before the reserve, so there is no hole for another task to + // take between the probe and the allocation. + const size_t largest = heap_caps_get_largest_free_block(MALLOC_CAP_DEFAULT); + // Leave a margin below the largest block: the allocator's own overhead sits inside it, and other + // threads keep allocating while we hold the SPI lock. + const size_t usable = largest > FILES_MANIFEST_HEAP_MARGIN ? largest - FILES_MANIFEST_HEAP_MARGIN : 0; + reservedCount = std::min(reservedCount, usable / sizeof(meshtastic_FileInfo)); +#else + // Other targets have no largest-block query. Probe with malloc() - the allocation that returns + // nullptr on failure under every build (new(std::nothrow) is not that: libstdc++ implements it as + // a try/catch around the throwing form) - free the probe, and reserve the size that fit. Not + // airtight against a concurrent allocator, but the SPI lock the caller holds serialises the usual + // competitors and it is strictly better than letting reserve() be the first to find out. while (reservedCount > 0) { - try { - filenames.reserve(reservedCount); + void *probe = malloc(reservedCount * sizeof(meshtastic_FileInfo)); + if (probe) { + free(probe); break; - } catch (const std::bad_alloc &) { - reservedCount /= 2; - } catch (const std::length_error &) { - reservedCount /= 2; } + reservedCount /= 2; } +#endif if (reservedCount == 0) { if (wasLimited) *wasLimited = true; @@ -290,7 +322,7 @@ std::vector getFiles(const char *dirname, uint8_t levels, s *wasLimited = true; maxCount = reservedCount; } -#endif + filenames.reserve(reservedCount); collectFiles(dirname, levels, maxCount, filenames, wasLimited); #endif return filenames; diff --git a/src/mesh/PhoneAPI.cpp b/src/mesh/PhoneAPI.cpp index 813d413dea..b45783677e 100644 --- a/src/mesh/PhoneAPI.cpp +++ b/src/mesh/PhoneAPI.cpp @@ -325,10 +325,10 @@ void PhoneAPI::handleStartConfig() filesManifest = getFiles("/", FILES_MANIFEST_LEVELS, FILES_MANIFEST_MAX_COUNT, &filesManifestLimited); } if (filesManifestLimited) { - LOG_WARN("Got %zu files in manifest (limited to %zu entries/depth %u)", filesManifest.size(), - FILES_MANIFEST_MAX_COUNT, static_cast(FILES_MANIFEST_LEVELS)); + LOG_WARN("Got %u files in manifest (limited to %u entries/depth %u)", (unsigned)filesManifest.size(), + (unsigned)FILES_MANIFEST_MAX_COUNT, static_cast(FILES_MANIFEST_LEVELS)); } else { - LOG_DEBUG("Got %zu files in manifest", filesManifest.size()); + LOG_DEBUG("Got %u files in manifest", (unsigned)filesManifest.size()); } } else { releaseFilesManifest(filesManifest); diff --git a/src/mesh/api/ServerAPI.cpp b/src/mesh/api/ServerAPI.cpp index 20ff8af999..7303ae3044 100644 --- a/src/mesh/api/ServerAPI.cpp +++ b/src/mesh/api/ServerAPI.cpp @@ -5,6 +5,8 @@ #include "ServerAPI.h" #include "Throttle.h" #include +#include +#include static constexpr uint32_t TCP_IDLE_TIMEOUT_MS = 15 * 60 * 1000UL; @@ -117,7 +119,22 @@ template int32_t APIServerPort::runOnce() openAPI.reset(); } - openAPI.reset(new T(client)); + // A ServerAPI carries the stream rx/tx buffers plus the FromRadio/ToRadio scratch, several + // KB in one block. On ESP32 a new that cannot get that block is a reboot (see the note on + // openAPI in the header), and std::nothrow does not help there because libstdc++ builds it + // on the throwing form. malloc() does return nullptr, so take the block from malloc() and + // construct in place; if there is no room drop this connection instead of the node - the + // client retries and the next accept gets a fresh look at the heap. The T constructors do + // not allocate (default-constructed containers, fixed-size thread table), so nothing inside + // the placement new can throw either. + void *block = malloc(sizeof(T)); + if (!block) { + LOG_ERROR("No heap for API connection (%u bytes), dropping client", (unsigned)sizeof(T)); + client.stop(); + } else { + openAPI.reset(new (block) T(client)); + } + // cppcheck-suppress memleak ; block is owned by openAPI via placement new, freed by MallocDeleter } #if RAK_4631 diff --git a/src/mesh/api/ServerAPI.h b/src/mesh/api/ServerAPI.h index ece8e0ba23..05c3bb56f5 100644 --- a/src/mesh/api/ServerAPI.h +++ b/src/mesh/api/ServerAPI.h @@ -1,6 +1,7 @@ #pragma once #include "StreamAPI.h" +#include #include #define SERVER_API_DEFAULT_PORT 4403 @@ -44,8 +45,23 @@ template class APIServerPort : public U, private concurrency: * * FIXME: We currently only allow one open TCP connection at a time, because we depend on the loop() call in this class to * delegate to the worker. Once coroutines are implemented we can relax this restriction. + * + * The ServerAPI is built in a malloc()'d block with placement new rather than operator new: on ESP32 the framework + * is compiled with CONFIG_COMPILER_CXX_EXCEPTIONS=n and every throw is wrapped to abort(), which makes a failed + * operator new - the plain form and, because libstdc++ implements it as a try/catch around the plain form, the + * std::nothrow form too - a reboot. malloc() is the one allocation on that platform that hands back nullptr, so + * a fragmented heap drops the incoming client instead of the node. The deleter runs the destructor and free()s. */ - std::unique_ptr openAPI; + struct MallocDeleter { + void operator()(T *p) const + { + if (p) { + p->~T(); + free(p); + } + } + }; + std::unique_ptr openAPI; #if defined(RAK_4631) || defined(RAK11310) // Track wait time for RAK13800 Ethernet requests int32_t waitTime = 100;