diff --git a/esphome/components/socket/bsd_sockets_impl.cpp b/esphome/components/socket/bsd_sockets_impl.cpp index aea7c776c6..92691b17ab 100644 --- a/esphome/components/socket/bsd_sockets_impl.cpp +++ b/esphome/components/socket/bsd_sockets_impl.cpp @@ -14,38 +14,34 @@ 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) - this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - this->loop_monitored_ = App.register_socket(this->cached_sock_); + this->cached_sock_ = hook_fd_for_fast_select(this->fd_); #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_) { - // 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; - } -#else - 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; +#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_); + } +#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..57c1a430a2 100644 --- a/esphome/components/socket/bsd_sockets_impl.h +++ b/esphome/components/socket/bsd_sockets_impl.h @@ -119,12 +119,21 @@ 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 - struct lwip_sock *cached_sock_{nullptr}; // Cached for direct rcvevent read in ready() -#endif - bool closed_{false}; + // Cached lwip_sock pointer used for direct rcvevent reads in ready() on the + // fast-select path. Replaces loop_monitored_: null means this socket is not being + // monitored for read events — either monitoring was not requested, the fd was + // invalid, or esphome_lwip_get_sock() failed. Non-null means the netconn event + // callback was hooked and notifications are flowing. close() nulls this to prevent + // use-after-free via a recycled lwip slot. + struct lwip_sock *cached_sock_{nullptr}; +#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 2fad429e0f..b4eba3febf 100644 --- a/esphome/components/socket/lwip_sockets_impl.cpp +++ b/esphome/components/socket/lwip_sockets_impl.cpp @@ -14,38 +14,34 @@ 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) - this->cached_sock_ = esphome_lwip_get_sock(this->fd_); - this->loop_monitored_ = App.register_socket(this->cached_sock_); + this->cached_sock_ = hook_fd_for_fast_select(this->fd_); #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_) { - // 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; - } -#else - 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; +#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_); + } +#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..7f3b706cd8 100644 --- a/esphome/components/socket/lwip_sockets_impl.h +++ b/esphome/components/socket/lwip_sockets_impl.h @@ -85,12 +85,21 @@ 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 - struct lwip_sock *cached_sock_{nullptr}; // Cached for direct rcvevent read in ready() -#endif - bool closed_{false}; + // Cached lwip_sock pointer used for direct rcvevent reads in ready() on the + // fast-select path. Replaces loop_monitored_: null means this socket is not being + // monitored for read events — either monitoring was not requested, the fd was + // invalid, or esphome_lwip_get_sock() failed. Non-null means the netconn event + // callback was hooked and notifications are flowing. close() nulls this to prevent + // use-after-free via a recycled lwip slot. + struct lwip_sock *cached_sock_{nullptr}; +#else bool loop_monitored_{false}; +#endif }; } // namespace esphome::socket diff --git a/esphome/components/socket/socket.h b/esphome/components/socket/socket.h index ad55e889e8..204113e4b2 100644 --- a/esphome/components/socket/socket.h +++ b/esphome/components/socket/socket.h @@ -42,8 +42,23 @@ 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 install the netconn event-callback hook so the +/// main loop is woken by FreeRTOS task notifications when data arrives. Shared between +/// BSD and LwIP socket impls on the fast-select path. Returns the cached lwip_sock +/// pointer (or nullptr if the fd does not map to a valid lwip_sock). +inline struct lwip_sock *hook_fd_for_fast_select(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. @@ -69,7 +84,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 diff --git a/esphome/core/application.cpp b/esphome/core/application.cpp index e5e1b36a65..affee20066 100644 --- a/esphome/core/application.cpp +++ b/esphome/core/application.cpp @@ -509,32 +509,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 645caa2404..a512af9c61 100644 --- a/esphome/core/application.h +++ b/esphome/core/application.h @@ -345,16 +345,13 @@ class Application { Scheduler scheduler; - /// 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) - /// Fallback select() path: monitors file descriptors. +#ifdef USE_HOST + /// Register/unregister a socket file descriptor with the host select() fallback loop. + /// USE_LWIP_FAST_SELECT builds do not use this API — sockets hook the lwIP netconn + /// event_callback directly (see socket.h hook_fd_for_fast_select) and rely on FreeRTOS + /// task notifications for wake-up. /// NOTE: File descriptors >= FD_SETSIZE (typically 10 on ESP) will be rejected with an error. + /// WARNING: These functions are NOT thread-safe. They must only be called from the main loop. /// @return true if registration was successful, false if fd exceeds limits bool register_socket_fd(int fd); void unregister_socket_fd(int fd); @@ -488,9 +485,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 @@ -705,26 +700,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. Additional wake sources: + // wake_loop_threadsafe() from background tasks, and the delay_ms 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); }