From cd6ad2333abd4b239b454b842e06c216756cb2e1 Mon Sep 17 00:00:00 2001 From: agessaman Date: Fri, 10 Jul 2026 18:51:17 -0700 Subject: [PATCH] fix(esp32): stop IDF 4.4 ws-transport heap overflow crashing bridge teardown The precompiled IDF 4.4 WebSocket transport (libtcp_transport.a) has an off-by-one in ws_connect(): when a wss:// endpoint answers the upgrade request with >=1024 bytes of HTTP response before the blank-line terminator (typical of a down broker behind a proxy serving a large error page), it writes a NUL one byte past the 1024-byte ws->buffer. Heap poisoning catches the clobbered tail canary (0xbaad5678 -> 0xbaad5600) only when the block is freed in ws_destroy() during esp_mqtt_client_destroy() - i.e. MQTTBridge::end() - so a single down broker made every deferred 'ota update' panic and reboot at teardown, before the download started. Decoded from a Heltec V3 crash backtrace on v1.16.0.11; line numbers match ESP-IDF release/v4.4 exactly. The transport code ships precompiled, so patch at link time instead: [esp32_base] wraps esp_transport_ws_init and the wrapper swaps the fresh buffer for a (WS_BUFFER_SIZE + 1)-byte allocation, making the out-of-bounds index land on owned memory. The oversized handshake then fails cleanly instead of corrupting the heap. Pass-through on IDF 5.x, where upstream already fixed it; delete with the Arduino core 3.x move. Verified: wrap resolves from ESP32WsTransportFix.cpp.o in the observer firmware.map; observer, room-server observer and plain repeater ESP32 targets build. RAK_4631_repeater failure is pre-existing (reproduced on the merge base without these changes). --- platformio.ini | 3 + src/helpers/AlertReporter.cpp | 10 ++- src/helpers/ESP32WsTransportFix.cpp | 130 ++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 2 deletions(-) create mode 100644 src/helpers/ESP32WsTransportFix.cpp diff --git a/platformio.ini b/platformio.ini index e16f7b83..d38402c7 100644 --- a/platformio.ini +++ b/platformio.ini @@ -61,6 +61,9 @@ monitor_filters = esp32_exception_decoder extra_scripts = merge-bin.py build_flags = ${arduino_base.build_flags} -D ESP32_PLATFORM + ; Route esp_transport_ws_init through src/helpers/ESP32WsTransportFix.cpp to + ; fix a heap-overflow in the precompiled IDF 4.4 WS transport (see that file). + -Wl,--wrap=esp_transport_ws_init ; -D ESP32_CPU_FREQ=80 ; change it to your need build_src_filter = ${arduino_base.build_src_filter} diff --git a/src/helpers/AlertReporter.cpp b/src/helpers/AlertReporter.cpp index a6841277..4bce1fd0 100644 --- a/src/helpers/AlertReporter.cpp +++ b/src/helpers/AlertReporter.cpp @@ -218,6 +218,12 @@ void AlertReporter::onLoop(unsigned long now_ms) { // already enforces this on set, but a stale prefs file or future field // tweak shouldn't be able to drag the floor below 1 hour and let a // flapping link spam the mesh. + // + // The rate limiter only applies between two real sends: fired_at_ms == 0 + // means "never fired since boot/config change", and treating it as a send + // at millis()==0 would suppress every first alert until uptime reaches + // min_interval (observed as a 30-minute alert.mqtt threshold not reporting + // until 60 minutes after a reboot). uint16_t cfg_min = _obs->alert_min_interval_min; if (cfg_min < 60) cfg_min = 60; unsigned long min_interval_ms = (unsigned long)cfg_min * 60000UL; @@ -232,7 +238,7 @@ void AlertReporter::onLoop(unsigned long now_ms) { if (_wifi.state == OK) { if (wifi_down && down_ms >= thresh_ms && - (now_ms - _wifi.fired_at_ms) >= min_interval_ms) { + (_wifi.fired_at_ms == 0 || (now_ms - _wifi.fired_at_ms) >= min_interval_ms)) { char age[16]; formatAge(down_ms, age, sizeof(age)); uint8_t reason = MQTTBridge::getLastWifiDisconnectReason(); @@ -282,7 +288,7 @@ void AlertReporter::onLoop(unsigned long now_ms) { if (f.state == OK) { if (down && down_ms >= thresh_ms && - (now_ms - f.fired_at_ms) >= min_interval_ms) { + (f.fired_at_ms == 0 || (now_ms - f.fired_at_ms) >= min_interval_ms)) { char age[16]; formatAge(down_ms, age, sizeof(age)); char text[100]; diff --git a/src/helpers/ESP32WsTransportFix.cpp b/src/helpers/ESP32WsTransportFix.cpp new file mode 100644 index 00000000..2e14bb33 --- /dev/null +++ b/src/helpers/ESP32WsTransportFix.cpp @@ -0,0 +1,130 @@ +#ifdef ESP_PLATFORM + +// Link-time workaround for an off-by-one heap overflow in ESP-IDF v4.4's +// WebSocket transport (components/tcp_transport/transport_ws.c), which ships +// PRECOMPILED in the Arduino-ESP32 2.x SDK (libtcp_transport.a) and cannot be +// patched at source level. +// +// The bug (transport_ws.c, ws_connect() response-read loop): +// +// header_len += len; +// ws->buffer[header_len] = '\0'; // header_len can reach WS_BUFFER_SIZE +// } while (... && header_len < WS_BUFFER_SIZE); +// +// ws->buffer is malloc(WS_BUFFER_SIZE) (1024). When a wss:// endpoint answers +// the WebSocket upgrade with >= 1024 bytes of HTTP response before the blank +// line terminator (typical for a down/misconfigured broker behind a proxy or +// CDN that serves a large HTML error page), the final iteration writes one +// '\0' one byte past the block. With heap poisoning enabled that zeroes the +// LSB of the tail canary (0xbaad5678 -> 0xbaad5600); the corruption then sits +// silent until the block is freed — which happens in ws_destroy() during +// esp_mqtt_client_destroy(), i.e. MQTTBridge::end() — and the free asserts: +// +// CORRUPT HEAP: Bad tail at 0x.... Expected 0xbaad5678 got 0xbaad5600 +// assert failed: multi_heap_free multi_heap_poisoning.c:259 +// +// On observer builds that teardown runs at the start of the deferred +// `ota update`, so a single down wss broker made every online OTA panic and +// reboot before the download began (backtrace decoded from a Heltec V3 on +// v1.16.0.11: free <- ws_destroy <- esp_transport_list_destroy <- +// esp_mqtt_client_destroy <- ~PsychicMqttClient <- destroySlotClients <- +// MQTTBridge::end <- MyMesh::setBridgeState <- MyMesh::loop). +// +// Fix: [esp32_base] adds `-Wl,--wrap=esp_transport_ws_init`, so every +// creation of a WS transport (esp-mqtt does one per wss slot) is routed +// through __wrap_esp_transport_ws_init below, which replaces the freshly +// allocated 1024-byte buffer with a (WS_BUFFER_SIZE + 1)-byte one. The +// out-of-bounds index WS_BUFFER_SIZE then lands on our extra byte and the +// handshake fails cleanly ("Upgrade" header not found) instead of corrupting +// the heap. Upstream fixed this in ESP-IDF 5.x, so this file compiles to a +// pass-through there and can be deleted (together with the --wrap flag) when +// the fork moves to Arduino core 3.x. +// +// transport_ws_t below is copied verbatim from ESP-IDF release/v4.4 +// transport_ws.c (the struct is file-private, so it is not in any shipped +// header). Source fidelity was verified against the shipped binary: addr2line +// on the crash backtrace resolves to the exact line numbers of that file +// (e.g. free(ws->buffer) at transport_ws.c:546). Only the first two members +// (path, buffer) are dereferenced here. + +#include "esp_idf_version.h" + +#if ESP_IDF_VERSION_MAJOR == 4 + +#include +#include "sdkconfig.h" +#include "esp_transport.h" +#include "esp_transport_ws.h" + +#ifndef CONFIG_WS_BUFFER_SIZE +#define CONFIG_WS_BUFFER_SIZE 1024 +#endif + +// --- copied from ESP-IDF release/v4.4 components/tcp_transport/transport_ws.c --- +typedef struct { + uint8_t opcode; + char mask_key[4]; + int payload_len; + int bytes_remaining; + bool header_received; +} ws_transport_frame_state_t; + +typedef struct { + char *path; + char *buffer; + char *sub_protocol; + char *user_agent; + char *headers; + bool propagate_control_frames; + ws_transport_frame_state_t frame_state; + esp_transport_handle_t parent; +} transport_ws_t; +// -------------------------------------------------------------------------------- + +extern "C" { + +esp_transport_handle_t __real_esp_transport_ws_init(esp_transport_handle_t parent_handle); + +esp_transport_handle_t __wrap_esp_transport_ws_init(esp_transport_handle_t parent_handle) { + esp_transport_handle_t t = __real_esp_transport_ws_init(parent_handle); + if (t != nullptr) { + transport_ws_t* ws = (transport_ws_t*)esp_transport_get_context_data(t); + if (ws != nullptr && ws->buffer != nullptr) { + // The buffer is untouched at this point (allocated moments ago inside + // __real_esp_transport_ws_init), so a swap is safe. + char* padded = (char*)malloc(CONFIG_WS_BUFFER_SIZE + 1); + if (padded != nullptr) { + free(ws->buffer); + ws->buffer = padded; + } + // On alloc failure keep the original buffer: same behavior as before + // this fix, which is still strictly better than failing init here. + } + } + return t; +} + +} // extern "C" + +#else // ESP_IDF_VERSION_MAJOR != 4 + +// IDF 5.x fixed the overflow upstream; keep a pass-through so the --wrap flag +// (set for all ESP32 envs in [esp32_base]) still links if anything references +// the symbol. + +#include "esp_transport.h" +#include "esp_transport_ws.h" + +extern "C" { + +esp_transport_handle_t __real_esp_transport_ws_init(esp_transport_handle_t parent_handle); + +esp_transport_handle_t __wrap_esp_transport_ws_init(esp_transport_handle_t parent_handle) { + return __real_esp_transport_ws_init(parent_handle); +} + +} // extern "C" + +#endif // ESP_IDF_VERSION_MAJOR + +#endif // ESP_PLATFORM