From e8e700a683f2ad210892393e6f67260855e493d6 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 10 Mar 2026 20:51:54 -1000 Subject: [PATCH] [socket] Fix RP2040 heap corruption from malloc in lwip accept callback (#14687) --- .../components/socket/lwip_raw_tcp_impl.cpp | 142 ++++++++++++++---- esphome/components/socket/lwip_raw_tcp_impl.h | 41 ++--- 2 files changed, 138 insertions(+), 45 deletions(-) diff --git a/esphome/components/socket/lwip_raw_tcp_impl.cpp b/esphome/components/socket/lwip_raw_tcp_impl.cpp index d7fa6a2694..fd1b8a9554 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.cpp +++ b/esphome/components/socket/lwip_raw_tcp_impl.cpp @@ -425,20 +425,24 @@ LWIPRawImpl::~LWIPRawImpl() { // Base class destructor handles pcb_ cleanup via tcp_abort } -void LWIPRawImpl::init() { +void LWIPRawImpl::init(struct pbuf *initial_rx, bool initial_rx_closed) { LWIP_LOCK(); LWIP_LOG("init(%p)", this->pcb_); tcp_arg(this->pcb_, this); tcp_recv(this->pcb_, LWIPRawImpl::s_recv_fn); tcp_err(this->pcb_, LWIPRawImpl::s_err_fn); + if (initial_rx != nullptr) { + this->rx_buf_ = initial_rx; + this->rx_buf_offset_ = 0; + } + this->rx_closed_ = initial_rx_closed; } void LWIPRawImpl::s_err_fn(void *arg, err_t err) { - // Called by lwip core which already holds the async_context lock on RP2040. - // No LWIP_LOCK() needed — acquiring it would be redundant (recursive mutex). + // LWIP CALLBACK — runs from IRQ context on RP2040 (low-priority user IRQ). + // No heap allocation allowed — malloc is not IRQ-safe (see #14687). + // No LWIP_LOCK() needed — lwip core already holds the async_context lock. // - // "If a connection is aborted because of an error, the application is alerted of this event by - // the err callback." // pcb is already freed when this callback is called // ERR_RST: connection was reset by remote host // ERR_ABRT: aborted through tcp_abort or TCP timer @@ -453,11 +457,15 @@ err_t LWIPRawImpl::s_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, er } err_t LWIPRawImpl::recv_fn(struct pbuf *pb, err_t err) { - // Called by lwip core which already holds the async_context lock on RP2040. + // LWIP CALLBACK — runs from IRQ context on RP2040 (low-priority user IRQ). + // No heap allocation allowed — malloc is not IRQ-safe (see #14687). LWIP_LOG("recv(pb=%p err=%d)", pb, err); if (err != 0) { // "An error code if there has been an error receiving Only return ERR_ABRT if you have // called tcp_abort from within the callback function!" + if (pb != nullptr) { + pbuf_free(pb); + } this->rx_closed_ = true; return ERR_OK; } @@ -664,6 +672,22 @@ ssize_t LWIPRawImpl::writev(const struct iovec *iov, int iovcnt) { LWIPRawListenImpl::~LWIPRawListenImpl() { LWIP_LOCK(); + // Abort any queued PCBs that were never accepted by the main loop. + // Clear the error callback first — tcp_abort triggers it, and we don't + // want s_queued_err_fn writing to slots during destruction. + for (uint8_t i = 0; i < this->accepted_socket_count_; i++) { + auto &entry = this->accepted_pcbs_[i]; + if (entry.pcb != nullptr) { + tcp_err(entry.pcb, nullptr); + tcp_abort(entry.pcb); + entry.pcb = nullptr; + } + if (entry.rx_buf != nullptr) { + pbuf_free(entry.rx_buf); + entry.rx_buf = nullptr; + } + } + this->accepted_socket_count_ = 0; // Listen PCBs must use tcp_close(), not tcp_abort(). // tcp_abandon() asserts pcb->state != LISTEN and would access // fields that don't exist in the smaller tcp_pcb_listen struct. @@ -683,12 +707,49 @@ void LWIPRawListenImpl::init() { } void LWIPRawListenImpl::s_err_fn(void *arg, err_t err) { - // Called by lwip core which already holds the async_context lock on RP2040. + // LWIP CALLBACK — runs from IRQ context on RP2040 (low-priority user IRQ). + // No heap allocation allowed — malloc is not IRQ-safe (see #14687). auto *arg_this = reinterpret_cast(arg); ESP_LOGVV(TAG, "socket %p: err(err=%d)", arg_this, err); arg_this->pcb_ = nullptr; } +void LWIPRawListenImpl::s_queued_err_fn(void *arg, err_t err) { + // LWIP CALLBACK — runs from IRQ context on RP2040 (low-priority user IRQ). + // No heap allocation allowed — malloc is not IRQ-safe (see #14687). + // Called when a queued (not yet accepted) PCB errors — e.g., remote sent RST. + // The PCB is already freed by lwip. Null our pointer so accept() skips it. + (void) err; + auto *entry = reinterpret_cast(arg); + entry->pcb = nullptr; + // Don't free rx_buf here — accept() will clean it up when it sees pcb==nullptr +} + +err_t LWIPRawListenImpl::s_queued_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, err_t err) { + // LWIP CALLBACK — runs from IRQ context on RP2040 (low-priority user IRQ). + // No heap allocation allowed — malloc is not IRQ-safe (see #14687). + // Temporary recv callback for PCBs queued between accept_fn_ and accept(). + // Without this, lwip's default tcp_recv_null handler would ACK and drop the data, + // causing the API handshake to silently fail (client sends Hello, server never sees it). + (void) pcb; + auto *entry = reinterpret_cast(arg); + if (pb == nullptr || err != ERR_OK) { + // Remote closed or error + if (pb != nullptr) { + pbuf_free(pb); + } + entry->rx_closed = true; + return ERR_OK; + } + // Buffer the data — tcp_recved() is deferred to read() after accept() creates the socket. + if (entry->rx_buf == nullptr) { + entry->rx_buf = pb; + } else { + pbuf_cat(entry->rx_buf, pb); + } + return ERR_OK; +} + err_t LWIPRawListenImpl::s_accept_fn(void *arg, struct tcp_pcb *newpcb, err_t err) { auto *arg_this = reinterpret_cast(arg); return arg_this->accept_fn_(newpcb, err); @@ -700,23 +761,40 @@ std::unique_ptr LWIPRawListenImpl::accept(struct sockaddr *addr, so errno = EBADF; return nullptr; } - if (this->accepted_socket_count_ == 0) { - errno = EWOULDBLOCK; - return nullptr; + // Dequeue front entry, skipping any null entries (PCBs freed by lwip while queued). + // The error callback nulled their pcb pointers; clean up buffered data and discard. + while (this->accepted_socket_count_ > 0) { + QueuedPcb entry = this->accepted_pcbs_[0]; + // Shift remaining entries forward, updating tcp_arg pointers as we go. + // Safe because we hold LWIP_LOCK, so err/recv callbacks can't fire during the update. + for (uint8_t i = 1; i < this->accepted_socket_count_; i++) { + this->accepted_pcbs_[i - 1] = this->accepted_pcbs_[i]; + if (this->accepted_pcbs_[i - 1].pcb != nullptr) { + tcp_arg(this->accepted_pcbs_[i - 1].pcb, &this->accepted_pcbs_[i - 1]); + } + } + this->accepted_pcbs_[this->accepted_socket_count_ - 1] = {}; + this->accepted_socket_count_--; + if (entry.pcb == nullptr) { + // PCB was freed by lwip (RST/timeout) while queued — discard and try next + if (entry.rx_buf != nullptr) { + pbuf_free(entry.rx_buf); + } + continue; + } + LWIP_LOG("Connection accepted by application, queue size: %d", this->accepted_socket_count_); + // Create socket wrapper on the main loop (not in accept callback) to avoid + // heap allocation in IRQ context on RP2040. Transfer any data received while queued. + auto sock = make_unique(this->family_, entry.pcb); + sock->init(entry.rx_buf, entry.rx_closed); + if (addr != nullptr) { + sock->getpeername(addr, addrlen); + } + LWIP_LOG("accept(%p)", sock.get()); + return sock; } - // Take from front for FIFO ordering - std::unique_ptr sock = std::move(this->accepted_sockets_[0]); - // Shift remaining sockets forward - for (uint8_t i = 1; i < this->accepted_socket_count_; i++) { - this->accepted_sockets_[i - 1] = std::move(this->accepted_sockets_[i]); - } - this->accepted_socket_count_--; - LWIP_LOG("Connection accepted by application, queue size: %d", this->accepted_socket_count_); - if (addr != nullptr) { - sock->getpeername(addr, addrlen); - } - LWIP_LOG("accept(%p)", sock.get()); - return sock; + errno = EWOULDBLOCK; + return nullptr; } int LWIPRawListenImpl::listen(int backlog) { @@ -746,7 +824,8 @@ int LWIPRawListenImpl::listen(int backlog) { } err_t LWIPRawListenImpl::accept_fn_(struct tcp_pcb *newpcb, err_t err) { - // Called by lwip core which already holds the async_context lock on RP2040. + // LWIP CALLBACK — runs from IRQ context on RP2040 (low-priority user IRQ). + // No heap allocation allowed — malloc is not IRQ-safe (see #14687). LWIP_LOG("accept(newpcb=%p err=%d)", newpcb, err); if (err != ERR_OK || newpcb == nullptr) { // "An error code if there has been an error accepting. Only return ERR_ABRT if you have @@ -763,9 +842,18 @@ err_t LWIPRawListenImpl::accept_fn_(struct tcp_pcb *newpcb, err_t err) { // Must return ERR_ABRT since we called tcp_abort() return ERR_ABRT; } - auto sock = make_unique(this->family_, newpcb); - sock->init(); - this->accepted_sockets_[this->accepted_socket_count_++] = std::move(sock); + // Store the raw PCB — LWIPRawImpl creation is deferred to the main-loop accept(). + // This avoids heap allocation in this callback, which is unsafe from IRQ context on RP2040. + uint8_t idx = this->accepted_socket_count_++; + this->accepted_pcbs_[idx] = {newpcb, nullptr, false}; + // Register temporary callbacks so that while the PCB is queued: + // - err: nulls our pointer if the connection errors (RST, timeout) + // - recv: buffers any data that arrives before accept() creates the LWIPRawImpl + // (without this, lwip's default tcp_recv_null would ACK and drop the data) + // tcp_arg points to our queue entry; accept() updates these pointers after shifting. + tcp_arg(newpcb, &this->accepted_pcbs_[idx]); + tcp_err(newpcb, LWIPRawListenImpl::s_queued_err_fn); + tcp_recv(newpcb, LWIPRawListenImpl::s_queued_recv_fn); LWIP_LOG("Accepted connection, queue size: %d", this->accepted_socket_count_); #if (defined(USE_ESP8266) || defined(USE_RP2040)) // Wake the main loop immediately so it can accept the new connection. diff --git a/esphome/components/socket/lwip_raw_tcp_impl.h b/esphome/components/socket/lwip_raw_tcp_impl.h index 5b2c11cfe2..95931afcf3 100644 --- a/esphome/components/socket/lwip_raw_tcp_impl.h +++ b/esphome/components/socket/lwip_raw_tcp_impl.h @@ -66,7 +66,7 @@ class LWIPRawImpl : public LWIPRawCommon { using LWIPRawCommon::LWIPRawCommon; ~LWIPRawImpl(); - void init(); + void init(struct pbuf *initial_rx = nullptr, bool initial_rx_closed = false); // Non-listening sockets return error std::unique_ptr accept(struct sockaddr *, socklen_t *) { @@ -182,23 +182,28 @@ class LWIPRawListenImpl : public LWIPRawCommon { err_t accept_fn_(struct tcp_pcb *newpcb, err_t err); static err_t s_accept_fn(void *arg, struct tcp_pcb *newpcb, err_t err); - // Accept queue - holds incoming connections briefly until the event loop calls accept() - // This is NOT a connection pool - just a temporary queue between LWIP callbacks and the main loop - // 3 slots is plenty since connections are pulled out quickly by the event loop - // - // Memory analysis: std::array<3> vs original std::queue implementation: - // - std::queue uses std::deque internally which on 32-bit systems needs: - // 24 bytes (deque object) + 32+ bytes (map array) + heap allocations - // Total: ~56+ bytes minimum, plus heap fragmentation - // - std::array<3>: 12 bytes fixed (3 pointers × 4 bytes) - // Saves ~44+ bytes RAM per listening socket + avoids ALL heap allocations - // Used on ESP8266 and RP2040 (platforms using LWIP_TCP implementation) - // - // By using a separate listening socket class, regular connected sockets save - // 16 bytes (12 bytes array + 1 byte count + 3 bytes padding) of memory overhead on 32-bit systems - static constexpr size_t MAX_ACCEPTED_SOCKETS = 3; - std::array, MAX_ACCEPTED_SOCKETS> accepted_sockets_; - uint8_t accepted_socket_count_ = 0; // Number of sockets currently in queue + // Temporary callbacks for queued PCBs (between accept_fn_ and accept()) + static void s_queued_err_fn(void *arg, err_t err); + static err_t s_queued_recv_fn(void *arg, struct tcp_pcb *pcb, struct pbuf *pb, err_t err); + + // Accept queue entry — stores a raw tcp_pcb and any data received while queued. + // lwip's default tcp_recv_null handler drops data and ACKs it, so we must register + // a temporary recv callback to buffer any data that arrives between accept_fn_ + // (which stores the PCB) and accept() (which creates the LWIPRawImpl). + struct QueuedPcb { + struct tcp_pcb *pcb{nullptr}; + struct pbuf *rx_buf{nullptr}; // Data received while queued (before accept() picks it up) + bool rx_closed{false}; // Remote sent FIN while queued + }; + + // Accept queue — stores raw tcp_pcb entries instead of heap-allocated LWIPRawImpl objects. + // LWIPRawImpl creation is deferred to the main-loop accept() call. This avoids: + // - Heap allocation in the accept callback (unsafe from IRQ context on RP2040) + // - Dangling LWIPRawImpl if the connection errors before accept() picks it up + // 2 slots is plenty since the main loop drains the queue every iteration. + static constexpr size_t MAX_ACCEPTED_SOCKETS = 2; + std::array accepted_pcbs_{}; + uint8_t accepted_socket_count_ = 0; // Number of entries currently in queue }; } // namespace esphome::socket