mirror of
https://github.com/meshtastic/firmware.git
synced 2026-10-02 03:05:20 -04:00
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<NetworkClient>::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<WiFiServerAPI, NetworkServer>::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.
This commit is contained in:
1 parent
ee401242aa
commit
fe15786dc1
4 files changed
+81
-16
No files matched your search
@@ -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<unsigned>(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<unsigned>(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);
|
||||
|
||||
Reference in new issue
Block a user