From 7ce49089a09b931918b374d871b30bda4a228111 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 20:39:01 -1000 Subject: [PATCH 1/5] [core] Remove pre-sleep socket scan from fast select path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-sleep scan in Application::yield_with_select_() walks monitored_sockets_ on every loop iteration, issuing a volatile cross-thread read on each socket's lwip_sock::rcvevent to preserve select() semantics when the FreeRTOS task notification counter had been consumed but a socket still had unread data. That scenario only existed because of a Socket::ready() contract violation: callers could stop reading with rcvevent > 0, leaving data behind with no pending notification. That contract is now documented and enforced (#15590), and #15589 (the first failure that reverted the earlier removal attempt #14475) has been fixed. With the contract honoured, every rcvevent > 0 is paired with a pending xTaskNotifyGive from the lwip event_callback wrapper (see lwip_fast_select.c). ulTaskNotifyTake either returns immediately (counter non-zero) or wakes the moment the notify lands — the scan has nothing left to rescue. Evidence: https://github.com/esphome/esphome/pull/15638 — an instrumentation PR ran across 5 devices (ESP32 rev1/rev3.1/C3 on Ethernet and WiFi, plus LibreTiny RTL8720CF) through Home Assistant disconnect/reconnect cycles, multi-client API logger bursts, and BLE GATT connect storms. Across ~275,000 scans and 4 observed load-bearing candidates, every hit was in the 2–14µs range — the instruction-level window between the lwip callback writing rcvevent and calling xTaskNotifyGive a few instructions later. Zero hits exceeded 100µs. No hit came anywhere near loop_interval (16ms), which is the latency scale the scan was added to prevent. In addition to being unused, the scan is actively harmful on the hot path: N volatile 16-bit loads against cache-cold cross-thread lwip_sock structures on every main-loop iteration, just to reproduce a microsecond-scale ordering artifact the notification path is already handling authoritatively. This also removes the now-unused monitored_sockets_ vector and Application::{register,unregister}_socket() on the fast-select path. Socket implementations now call esphome_lwip_hook_socket() directly to install the netconn event callback wrapper. --- .../components/socket/bsd_sockets_impl.cpp | 16 +++++---- .../components/socket/lwip_sockets_impl.cpp | 16 +++++---- esphome/core/application.cpp | 27 +-------------- esphome/core/application.h | 33 +++++-------------- 4 files changed, 27 insertions(+), 65 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index aea7c776c6..1db905daea 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -14,9 +14,13 @@ BSDSocketImpl::BSDSocketImpl(int fd, bool monitor_loop) { if (!monitor_loop || this->fd_ < 0) return; #ifdef USE_LWIP_FAST_SELECT - // Cache lwip_sock pointer and register for monitoring (hooks callback internally) + // Cache lwip_sock pointer (used by ready() for direct rcvevent reads) and hook the + // netconn event callback so the main loop is notified via FreeRTOS task notifications. this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - this->loop_monitored_ = App.register_socket(this->cached_sock_); + if (this->cached_sock_ != nullptr) { + esphome_lwip_hook_socket(this->cached_sock_); + this->loop_monitored_ = true; + } #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif @@ -30,12 +34,10 @@ BSDSocketImpl::~BSDSocketImpl() { int BSDSocketImpl::close() { if (!this->closed_) { - // Unregister before closing to avoid dangling pointer in monitored set #ifdef USE_LWIP_FAST_SELECT - if (this->loop_monitored_) { - App.unregister_socket(this->cached_sock_); - this->cached_sock_ = nullptr; - } + // All LwIP sockets share the same static event_callback, so there is no per-socket + // unhook needed — just drop the cached pointer before the socket is destroyed. + this->cached_sock_ = nullptr; #else if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index 2fad429e0f..e628a22d8d 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -14,9 +14,13 @@ LwIPSocketImpl::LwIPSocketImpl(int fd, bool monitor_loop) { if (!monitor_loop || this->fd_ < 0) return; #ifdef USE_LWIP_FAST_SELECT - // Cache lwip_sock pointer and register for monitoring (hooks callback internally) + // Cache lwip_sock pointer (used by ready() for direct rcvevent reads) and hook the + // netconn event callback so the main loop is notified via FreeRTOS task notifications. this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - this->loop_monitored_ = App.register_socket(this->cached_sock_); + if (this->cached_sock_ != nullptr) { + esphome_lwip_hook_socket(this->cached_sock_); + this->loop_monitored_ = true; + } #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif @@ -30,12 +34,10 @@ LwIPSocketImpl::~LwIPSocketImpl() { int LwIPSocketImpl::close() { if (!this->closed_) { - // Unregister before closing to avoid dangling pointer in monitored set #ifdef USE_LWIP_FAST_SELECT - if (this->loop_monitored_) { - App.unregister_socket(this->cached_sock_); - this->cached_sock_ = nullptr; - } + // All LwIP sockets share the same static event_callback, so there is no per-socket + // unhook needed — just drop the cached pointer before the socket is destroyed. + this->cached_sock_ = nullptr; #else if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index cd75859880..510e3ec190 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -449,32 +449,7 @@ void Application::enable_pending_loops_() { } } -#ifdef USE_LWIP_FAST_SELECT -bool Application::register_socket(struct lwip_sock *sock) { - // It modifies monitored_sockets_ without locking — must only be called from the main loop. - if (sock == nullptr) - return false; - esphome_lwip_hook_socket(sock); - this->monitored_sockets_.push_back(sock); - return true; -} - -void Application::unregister_socket(struct lwip_sock *sock) { - // It modifies monitored_sockets_ without locking — must only be called from the main loop. - for (size_t i = 0; i < this->monitored_sockets_.size(); i++) { - if (this->monitored_sockets_[i] != sock) - continue; - - // Swap with last element and pop - O(1) removal since order doesn't matter. - // No need to unhook the netconn callback — all LwIP sockets share the same - // static event_callback, and the socket will be closed by the caller. - if (i < this->monitored_sockets_.size() - 1) - this->monitored_sockets_[i] = this->monitored_sockets_.back(); - this->monitored_sockets_.pop_back(); - return; - } -} -#elif defined(USE_HOST) +#ifdef USE_HOST bool Application::register_socket_fd(int fd) { // WARNING: This function is NOT thread-safe and must only be called from the main loop // It modifies socket_fds_ and related variables without locking diff --git a/esphome/core/application.h b/esphome/core/application.h index 6b2969b490..122f9b39c7 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -534,12 +534,7 @@ class Application { /// Register/unregister a socket to be monitored for read events. /// WARNING: These functions are NOT thread-safe. They must only be called from the main loop. -#ifdef USE_LWIP_FAST_SELECT - /// Fast select path: hooks netconn callback and registers for monitoring. - /// @return true if registration was successful, false if sock is null - bool register_socket(struct lwip_sock *sock); - void unregister_socket(struct lwip_sock *sock); -#elif defined(USE_HOST) +#ifdef USE_HOST /// Fallback select() path: monitors file descriptors. /// NOTE: File descriptors >= FD_SETSIZE (typically 10 on ESP) will be rejected with an error. /// @return true if registration was successful, false if fd exceeds limits @@ -653,9 +648,7 @@ class Application { // and active_end_ is incremented // - This eliminates branch mispredictions from flag checking in the hot loop FixedVector looping_components_{}; -#ifdef USE_LWIP_FAST_SELECT - std::vector monitored_sockets_; // Cached lwip_sock pointers for direct rcvevent read -#elif defined(USE_HOST) +#ifdef USE_HOST std::vector socket_fds_; // Vector of all monitored socket file descriptors #endif #ifdef USE_HOST @@ -898,26 +891,16 @@ inline void ESPHOME_ALWAYS_INLINE Application::loop() { #ifndef USE_HOST inline void ESPHOME_ALWAYS_INLINE Application::yield_with_select_(uint32_t delay_ms) { #ifdef USE_LWIP_FAST_SELECT - // Fast path (ESP32/LibreTiny): reads rcvevent directly from cached lwip_sock pointers. - // Safe because this runs on the main loop which owns socket lifetime (create, read, close). + // Fast path (ESP32/LibreTiny): FreeRTOS task notifications posted by the lwip + // event_callback wrapper (see lwip_fast_select.c) are the single source of truth for + // socket wake-ups. Every NETCONN_EVT_RCVPLUS posts an xTaskNotifyGive, so any notification + // that lands between wakes keeps the counter non-zero (next ulTaskNotifyTake returns + // immediately) or wakes a blocked Take directly. Also woken by wake_loop_threadsafe() + // from background tasks, or timeout. if (delay_ms == 0) [[unlikely]] { yield(); return; } - - // Check if any socket already has pending data before sleeping. - // If a socket still has unread data (rcvevent > 0) but the task notification was already - // consumed, ulTaskNotifyTake would block until timeout — adding up to delay_ms latency. - // This scan preserves select() semantics: return immediately when any fd is ready. - for (struct lwip_sock *sock : this->monitored_sockets_) { - if (esphome_lwip_socket_has_data(sock)) { - yield(); - return; - } - } - - // Sleep with instant wake via FreeRTOS task notification. - // Woken by: callback wrapper (socket data), wake_loop_threadsafe() (background tasks), or timeout. #endif esphome::internal::wakeable_delay(delay_ms); } From 4a9d3da962de2512bc0e7373145bd15e28075d04 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 20:58:12 -1000 Subject: [PATCH 2/5] [core] fast_select scan removal: address review feedback - Drop redundant cached_sock_ = nullptr assignment in close(). After closed_ = true the socket is a corpse and no ready() or other member access is valid, so the nulling is not load-bearing. The comment now explains why on both impl variants. - Reword the yield_with_select_ comment so the wake-source sentence reads as a complete list rather than a trailing fragment. --- esphome/components/socket/bsd_sockets_impl.cpp | 9 ++++----- esphome/components/socket/lwip_sockets_impl.cpp | 9 ++++----- esphome/core/application.h | 4 ++-- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index 1db905daea..2d4e0ea7c2 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -34,11 +34,10 @@ BSDSocketImpl::~BSDSocketImpl() { int BSDSocketImpl::close() { if (!this->closed_) { -#ifdef USE_LWIP_FAST_SELECT - // All LwIP sockets share the same static event_callback, so there is no per-socket - // unhook needed — just drop the cached pointer before the socket is destroyed. - this->cached_sock_ = nullptr; -#else +#ifndef USE_LWIP_FAST_SELECT + // All LwIP sockets share the same static event_callback, so on the fast-select path + // there is no per-socket unhook needed. cached_sock_ is not cleared because closed_ + // makes the socket a corpse — no ready() or other member access is valid afterwards. if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); } diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index e628a22d8d..c65d3a227d 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -34,11 +34,10 @@ LwIPSocketImpl::~LwIPSocketImpl() { int LwIPSocketImpl::close() { if (!this->closed_) { -#ifdef USE_LWIP_FAST_SELECT - // All LwIP sockets share the same static event_callback, so there is no per-socket - // unhook needed — just drop the cached pointer before the socket is destroyed. - this->cached_sock_ = nullptr; -#else +#ifndef USE_LWIP_FAST_SELECT + // All LwIP sockets share the same static event_callback, so on the fast-select path + // there is no per-socket unhook needed. cached_sock_ is not cleared because closed_ + // makes the socket a corpse — no ready() or other member access is valid afterwards. if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); } diff --git a/esphome/core/application.h b/esphome/core/application.h index 122f9b39c7..47976fd57b 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -895,8 +895,8 @@ inline void ESPHOME_ALWAYS_INLINE Application::yield_with_select_(uint32_t delay // event_callback wrapper (see lwip_fast_select.c) are the single source of truth for // socket wake-ups. Every NETCONN_EVT_RCVPLUS posts an xTaskNotifyGive, so any notification // that lands between wakes keeps the counter non-zero (next ulTaskNotifyTake returns - // immediately) or wakes a blocked Take directly. Also woken by wake_loop_threadsafe() - // from background tasks, or timeout. + // immediately) or wakes a blocked Take directly. Additional wake sources: + // wake_loop_threadsafe() from background tasks, and the delay_ms timeout. if (delay_ms == 0) [[unlikely]] { yield(); return; From 107915fe36bfeb00130d250a5b5102f3be196084 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 20:59:12 -1000 Subject: [PATCH 3/5] [socket] Dedupe fast_select hook logic into shared helper Lift the lwip_sock resolve + event-callback hook sequence into socket::fast_select_hook_fd() in socket.h so the USE_LWIP_FAST_SELECT constructor blocks in lwip_sockets_impl.cpp and bsd_sockets_impl.cpp stop drifting in lockstep. Both impls now collapse to a two-line call site. --- esphome/components/socket/bsd_sockets_impl.cpp | 9 ++------- esphome/components/socket/lwip_sockets_impl.cpp | 9 ++------- esphome/components/socket/socket.h | 11 +++++++++++ 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index 2d4e0ea7c2..d55523ff77 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -14,13 +14,8 @@ BSDSocketImpl::BSDSocketImpl(int fd, bool monitor_loop) { if (!monitor_loop || this->fd_ < 0) return; #ifdef USE_LWIP_FAST_SELECT - // Cache lwip_sock pointer (used by ready() for direct rcvevent reads) and hook the - // netconn event callback so the main loop is notified via FreeRTOS task notifications. - this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - if (this->cached_sock_ != nullptr) { - esphome_lwip_hook_socket(this->cached_sock_); - this->loop_monitored_ = true; - } + this->cached_sock_ = fast_select_hook_fd(this->fd_); + this->loop_monitored_ = this->cached_sock_ != nullptr; #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index c65d3a227d..475381548b 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -14,13 +14,8 @@ LwIPSocketImpl::LwIPSocketImpl(int fd, bool monitor_loop) { if (!monitor_loop || this->fd_ < 0) return; #ifdef USE_LWIP_FAST_SELECT - // Cache lwip_sock pointer (used by ready() for direct rcvevent reads) and hook the - // netconn event callback so the main loop is notified via FreeRTOS task notifications. - this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - if (this->cached_sock_ != nullptr) { - esphome_lwip_hook_socket(this->cached_sock_); - this->loop_monitored_ = true; - } + this->cached_sock_ = fast_select_hook_fd(this->fd_); + this->loop_monitored_ = this->cached_sock_ != nullptr; #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif diff --git a/esphome/components/socket/socket.h b/esphome/components/socket/socket.h index ad55e889e8..c33a5c42b6 100644 --- a/esphome/components/socket/socket.h +++ b/esphome/components/socket/socket.h @@ -45,6 +45,17 @@ using ListenSocket = LWIPRawListenImpl; inline bool socket_ready(struct lwip_sock *cached_sock, bool loop_monitored) { return !loop_monitored || (cached_sock != nullptr && esphome_lwip_socket_has_data(cached_sock)); } + +/// Resolve an fd to its lwip_sock and hook the netconn event callback so the main loop +/// is woken by FreeRTOS task notifications. Shared between BSD and LwIP socket impls. +/// Returns the cached lwip_sock pointer (or nullptr if fd is invalid). +inline struct lwip_sock *fast_select_hook_fd(int fd) { + struct lwip_sock *sock = esphome_lwip_get_sock(fd); + if (sock != nullptr) { + esphome_lwip_hook_socket(sock); + } + return sock; +} #elif defined(USE_HOST) /// Shared ready() helper for fd-based socket implementations. /// Checks if the Application's select() loop has marked this fd as ready. From b9541cd5db107ecaa95d0dc2eedfbbc0a1623344 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 21:05:21 -1000 Subject: [PATCH 4/5] [socket] Eliminate closed_ and loop_monitored_ redundancy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the separate closed_ bool with fd_ < 0 as the 'not open' sentinel. close() now sets fd_ = -1 after the underlying close call, so the destructor and double-close paths just check fd_ < 0. As a side benefit, get_fd() on a closed socket now returns -1, making use-after-close visible to callers instead of returning a stale descriptor. Drop loop_monitored_ on the USE_LWIP_FAST_SELECT path — the pointer cached_sock_ already encodes monitoring state (non-null iff monitored). On USE_HOST the bool is still needed because there is no cached pointer to derive from. Combined effect on the fast-select path: Before: fd_(4) + cached_sock_(4) + closed_(1) + loop_monitored_(1) + pad(2) = 12 bytes per socket After: fd_(4) + cached_sock_(4) = 8 bytes per socket (aligned, no tail padding) Saves 4 bytes per Socket instance on ESP32/LibreTiny. With typical workloads running 5-10 sockets (API listen + clients + mDNS) that's 20-40 bytes of RAM. --- .../components/socket/bsd_sockets_impl.cpp | 33 ++++++++----------- esphome/components/socket/bsd_sockets_impl.h | 9 +++-- .../components/socket/lwip_sockets_impl.cpp | 33 ++++++++----------- esphome/components/socket/lwip_sockets_impl.h | 9 +++-- esphome/components/socket/socket.h | 9 +++-- 5 files changed, 48 insertions(+), 45 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index d55523ff77..baf030f2b1 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -15,33 +15,28 @@ BSDSocketImpl::BSDSocketImpl(int fd, bool monitor_loop) { return; #ifdef USE_LWIP_FAST_SELECT this->cached_sock_ = fast_select_hook_fd(this->fd_); - this->loop_monitored_ = this->cached_sock_ != nullptr; #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif } -BSDSocketImpl::~BSDSocketImpl() { - if (!this->closed_) { - this->close(); - } -} +BSDSocketImpl::~BSDSocketImpl() { this->close(); } int BSDSocketImpl::close() { - if (!this->closed_) { -#ifndef USE_LWIP_FAST_SELECT - // All LwIP sockets share the same static event_callback, so on the fast-select path - // there is no per-socket unhook needed. cached_sock_ is not cleared because closed_ - // makes the socket a corpse — no ready() or other member access is valid afterwards. - if (this->loop_monitored_) { - App.unregister_socket_fd(this->fd_); - } -#endif - int ret = ::close(this->fd_); - this->closed_ = true; - return ret; + if (this->fd_ < 0) { + // Already closed, or never opened. + return 0; } - return 0; +#ifndef USE_LWIP_FAST_SELECT + // On the fast-select path there is no per-socket unhook needed — all LwIP sockets + // share the same static event_callback. + if (this->loop_monitored_) { + App.unregister_socket_fd(this->fd_); + } +#endif + int ret = ::close(this->fd_); + this->fd_ = -1; // Sentinel for "closed" — prevents double-close and makes use-after-close visible. + return ret; } int BSDSocketImpl::setblocking(bool blocking) { diff --git a/esphome/components/socket/bsd_sockets_impl.h b/esphome/components/socket/bsd_sockets_impl.h index e520784702..64b6adf94a 100644 --- a/esphome/components/socket/bsd_sockets_impl.h +++ b/esphome/components/socket/bsd_sockets_impl.h @@ -119,12 +119,17 @@ class BSDSocketImpl { int get_fd() const { return this->fd_; } protected: + // fd_ < 0 means "not open" — used both pre-open (initial state) and post-close. This + // replaces a separate closed_ flag: close() sets fd_ = -1 after ::close(), and the + // destructor / double-close path just check fd_ < 0. int fd_{-1}; #ifdef USE_LWIP_FAST_SELECT + // Non-null iff this socket is being monitored for read events. Replaces loop_monitored_ + // on the fast-select path: the pointer itself carries the "monitored" bit. struct lwip_sock *cached_sock_{nullptr}; // Cached for direct rcvevent read in ready() -#endif - bool closed_{false}; +#else bool loop_monitored_{false}; +#endif }; } // namespace esphome::socket diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index 475381548b..52c94bc7f6 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -15,33 +15,28 @@ LwIPSocketImpl::LwIPSocketImpl(int fd, bool monitor_loop) { return; #ifdef USE_LWIP_FAST_SELECT this->cached_sock_ = fast_select_hook_fd(this->fd_); - this->loop_monitored_ = this->cached_sock_ != nullptr; #else this->loop_monitored_ = App.register_socket_fd(this->fd_); #endif } -LwIPSocketImpl::~LwIPSocketImpl() { - if (!this->closed_) { - this->close(); - } -} +LwIPSocketImpl::~LwIPSocketImpl() { this->close(); } int LwIPSocketImpl::close() { - if (!this->closed_) { -#ifndef USE_LWIP_FAST_SELECT - // All LwIP sockets share the same static event_callback, so on the fast-select path - // there is no per-socket unhook needed. cached_sock_ is not cleared because closed_ - // makes the socket a corpse — no ready() or other member access is valid afterwards. - if (this->loop_monitored_) { - App.unregister_socket_fd(this->fd_); - } -#endif - int ret = lwip_close(this->fd_); - this->closed_ = true; - return ret; + if (this->fd_ < 0) { + // Already closed, or never opened. + return 0; } - return 0; +#ifndef USE_LWIP_FAST_SELECT + // On the fast-select path there is no per-socket unhook needed — all LwIP sockets + // share the same static event_callback. + if (this->loop_monitored_) { + App.unregister_socket_fd(this->fd_); + } +#endif + int ret = lwip_close(this->fd_); + this->fd_ = -1; // Sentinel for "closed" — prevents double-close and makes use-after-close visible. + return ret; } int LwIPSocketImpl::setblocking(bool blocking) { diff --git a/esphome/components/socket/lwip_sockets_impl.h b/esphome/components/socket/lwip_sockets_impl.h index 942d0ccf85..6e275eb6d1 100644 --- a/esphome/components/socket/lwip_sockets_impl.h +++ b/esphome/components/socket/lwip_sockets_impl.h @@ -85,12 +85,17 @@ class LwIPSocketImpl { int get_fd() const { return this->fd_; } protected: + // fd_ < 0 means "not open" — used both pre-open (initial state) and post-close. This + // replaces a separate closed_ flag: close() sets fd_ = -1 after lwip_close(), and the + // destructor / double-close path just check fd_ < 0. int fd_{-1}; #ifdef USE_LWIP_FAST_SELECT + // Non-null iff this socket is being monitored for read events. Replaces loop_monitored_ + // on the fast-select path: the pointer itself carries the "monitored" bit. struct lwip_sock *cached_sock_{nullptr}; // Cached for direct rcvevent read in ready() -#endif - bool closed_{false}; +#else bool loop_monitored_{false}; +#endif }; } // namespace esphome::socket diff --git a/esphome/components/socket/socket.h b/esphome/components/socket/socket.h index c33a5c42b6..739628ef43 100644 --- a/esphome/components/socket/socket.h +++ b/esphome/components/socket/socket.h @@ -42,8 +42,11 @@ using ListenSocket = LWIPRawListenImpl; #ifdef USE_LWIP_FAST_SELECT /// Shared ready() helper using cached lwip_sock pointer for direct rcvevent read. -inline bool socket_ready(struct lwip_sock *cached_sock, bool loop_monitored) { - return !loop_monitored || (cached_sock != nullptr && esphome_lwip_socket_has_data(cached_sock)); +/// cached_sock == nullptr means the socket is not monitored (monitor_loop was false, fd +/// was invalid, or esphome_lwip_get_sock() failed) — in that case return true so the +/// caller attempts the read and handles blocking itself. +inline bool socket_ready(struct lwip_sock *cached_sock) { + return cached_sock == nullptr || esphome_lwip_socket_has_data(cached_sock); } /// Resolve an fd to its lwip_sock and hook the netconn event callback so the main loop @@ -80,7 +83,7 @@ bool socket_ready_fd(int fd, bool loop_monitored); #if defined(USE_SOCKET_IMPL_BSD_SOCKETS) || defined(USE_SOCKET_IMPL_LWIP_SOCKETS) inline bool Socket::ready() const { #ifdef USE_LWIP_FAST_SELECT - return socket_ready(this->cached_sock_, this->loop_monitored_); + return socket_ready(this->cached_sock_); #else return socket_ready_fd(this->fd_, this->loop_monitored_); #endif From 884dab4a6a98bc125976d3dd7ed8bf2ea9b3f90b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 10 Apr 2026 21:12:51 -1000 Subject: [PATCH 5/5] [socket] Null cached_sock_ in close() to prevent post-close UAF MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restore the cached_sock_ = nullptr assignment inside close() on the fast-select path. The lwip slot can be recycled for a new connection as soon as the underlying close() returns, so any dereference of cached_sock_ afterwards would touch an unrelated socket's pcb. No current caller does this — setsockopt(TCP_NODELAY) and ready() are the only consumers and neither is invoked post-close today — but leaving the pointer dangling is a footgun for future changes. The fd_ = -1 sentinel alone would catch the ready() path via closed semantics, but setsockopt() reaches cached_sock_ directly and would not be protected. Null the pointer so the protection is by construction rather than by caller discipline. --- esphome/components/socket/bsd_sockets_impl.cpp | 11 ++++++++--- esphome/components/socket/lwip_sockets_impl.cpp | 11 ++++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index baf030f2b1..b73440ef55 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -27,9 +27,14 @@ int BSDSocketImpl::close() { // Already closed, or never opened. return 0; } -#ifndef USE_LWIP_FAST_SELECT - // On the fast-select path there is no per-socket unhook needed — all LwIP sockets - // share the same static event_callback. +#ifdef USE_LWIP_FAST_SELECT + // Null the cached lwip_sock pointer before closing. The underlying lwip slot can be + // recycled for a new connection as soon as ::close() returns, so anything that might + // dereference cached_sock_ post-close (e.g. setsockopt(TCP_NODELAY)) would otherwise + // touch an unrelated socket's pcb. No per-socket callback unhook is needed — + // all LwIP sockets share the same static event_callback. + this->cached_sock_ = nullptr; +#else if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); } diff --git a/esphome/components/socket/lwip_sockets_impl.cpp b/esphome/components/socket/lwip_sockets_impl.cpp index 52c94bc7f6..618d17aabe 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -27,9 +27,14 @@ int LwIPSocketImpl::close() { // Already closed, or never opened. return 0; } -#ifndef USE_LWIP_FAST_SELECT - // On the fast-select path there is no per-socket unhook needed — all LwIP sockets - // share the same static event_callback. +#ifdef USE_LWIP_FAST_SELECT + // Null the cached lwip_sock pointer before closing. The underlying lwip slot can be + // recycled for a new connection as soon as lwip_close() returns, so anything that + // might dereference cached_sock_ post-close (e.g. setsockopt(TCP_NODELAY)) would + // otherwise touch an unrelated socket's pcb. No per-socket callback unhook is needed — + // all LwIP sockets share the same static event_callback. + this->cached_sock_ = nullptr; +#else if (this->loop_monitored_) { App.unregister_socket_fd(this->fd_); }