From 92a88dd56849473e574724f6e9efc0442b2477f7 Mon Sep 17 00:00:00 2001 From: Thiker Shreah Date: Mon, 27 Apr 2026 15:21:41 +0300 Subject: [PATCH 1/2] Add delay after netif stop prior sending disconnect to CMUX virtual terminals --- components/esp_modem/Kconfig | 6 ++++++ components/esp_modem/src/esp_modem_dce.cpp | 1 + 2 files changed, 7 insertions(+) diff --git a/components/esp_modem/Kconfig b/components/esp_modem/Kconfig index 4e6fb88332..9b127d6f13 100644 --- a/components/esp_modem/Kconfig +++ b/components/esp_modem/Kconfig @@ -36,6 +36,12 @@ menu "esp-modem" The typical reason for failing SABM request without a delay is that some devices (SIM800) send MSC requests just after opening a new DLCI. + config ESP_MODEM_CMUX_DELAY_AFTER_NETIF_DISCONNECT + int "Delay in ms to wait before closing virtual terminal abd rigt after PPP disconnect(NETIF)" + default 0 + help + Some devices might need a pause before sending disconnect command that closes + virtual terminal. This delay applies only in CMUX mode. config ESP_MODEM_CMUX_USE_SHORT_PAYLOADS_ONLY bool "CMUX to support only short payloads (<128 bytes)" default n diff --git a/components/esp_modem/src/esp_modem_dce.cpp b/components/esp_modem/src/esp_modem_dce.cpp index f105516325..6b21c27cf7 100644 --- a/components/esp_modem/src/esp_modem_dce.cpp +++ b/components/esp_modem/src/esp_modem_dce.cpp @@ -160,6 +160,7 @@ bool DCE_Mode::set_unsafe(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m if (mode == modem_mode::CMUX_MODE) { netif.stop(); netif.wait_until_ppp_exits(); + usleep(CONFIG_ESP_MODEM_CMUX_DELAY_AFTER_NETIF_DISCONNECT * 1'000); if (!dte->set_mode(modem_mode::COMMAND_MODE)) { return false; } From 323bc7977e0ea8af77eab52570a6be8cf9d1fb9c Mon Sep 17 00:00:00 2001 From: Thiker Shreah Date: Sat, 20 Jun 2026 13:25:23 +0300 Subject: [PATCH 2/2] fix(modem): Fix terminal-callback use-after-free and harden task teardown Terminal read/error callbacks (Uart/Fd on_read/on_error and CMux read_cb[]) were reassigned (set_mode/set_command_callbacks), cleared, or had their owning DTE destroyed while the RX task was invoking them, with no synchronization (internal_lock does not help - the RX task never takes it). The invoked std::function or captured state (command_cb.line_lock) could be destroyed mid-call, surfacing as a spinlock_acquire assert in xQueueTakeMutexRecursive. - Serialize callback (re)assignment against invocation with a recursive cb_lock (Terminal/Uart/Fd; CMux gets its own, separate from the state lock so a blocking RX upcall cannot deadlock write()). - Clear/stop callbacks in ~DTE() and ~CMux() before the captured state is destroyed. - Make stop() graceful and synchronous (TASK_STOPPED handshake) so the RX task is quiesced before deletion instead of force-deleted mid-callback or in the driver; this also removes the self-delete vs owner-delete double-delete. ~DTE() stops the task first, and CMuxInstance::stop() forwards to the underlying physical terminal so it works in CMUX mode too. --- .../include/cxx_include/esp_modem_cmux.hpp | 24 +++++- .../include/cxx_include/esp_modem_dte.hpp | 2 +- .../cxx_include/esp_modem_terminal.hpp | 10 +++ components/esp_modem/src/esp_modem_cmux.cpp | 25 ++++++ components/esp_modem/src/esp_modem_dte.cpp | 22 ++++++ .../esp_modem/src/esp_modem_term_fs.cpp | 47 +++++++++--- components/esp_modem/src/esp_modem_uart.cpp | 76 +++++++++++++------ 7 files changed, 168 insertions(+), 38 deletions(-) diff --git a/components/esp_modem/include/cxx_include/esp_modem_cmux.hpp b/components/esp_modem/include/cxx_include/esp_modem_cmux.hpp index a17c3518d5..afd94913d8 100644 --- a/components/esp_modem/include/cxx_include/esp_modem_cmux.hpp +++ b/components/esp_modem/include/cxx_include/esp_modem_cmux.hpp @@ -49,7 +49,7 @@ class CMux { public: explicit CMux(std::shared_ptr t, unique_buffer &&b): term(std::move(t)), payload_start(nullptr), total_payload_size(0), buffer(std::move(b)) {} - ~CMux() = default; + ~CMux(); /** * @brief Initializes CMux protocol @@ -96,6 +96,14 @@ class CMux { */ bool recover(); + /** + * @brief Stops the underlying (physical) terminal's RX task. + * + * Used on teardown to quiesce the data pump that drives the virtual terminals before the + * CMux/DTE state it touches is destroyed. + */ + void stop(); + private: enum class protocol_mismatch_reason { @@ -150,6 +158,13 @@ class CMux { unique_buffer buffer; Lock lock; + /** + * @brief Serializes (re)assignment of read_cb[] (set_read_cb()) against their invocation from + * the underlying terminal's RX task. Deliberately separate from `lock`: the read callback + * upcall may take a long time (or block in the network stack) and must not block write(), + * which uses `lock`. Recursive, so the upcall may re-enter write(). + */ + Lock cb_lock; }; /** @@ -173,7 +188,12 @@ class CMuxInstance: public Terminal { return 0; } void start() override { } - void stop() override { } + // A virtual terminal has no task of its own; the physical terminal pumps all of them. Forward + // stop() so that tearing down a DTE in CMUX mode quiesces that physical terminal's RX task. + void stop() override + { + cmux->stop(); + } private: std::shared_ptr cmux; size_t instance; diff --git a/components/esp_modem/include/cxx_include/esp_modem_dte.hpp b/components/esp_modem/include/cxx_include/esp_modem_dte.hpp index 4e11515ff7..75c67269d7 100644 --- a/components/esp_modem/include/cxx_include/esp_modem_dte.hpp +++ b/components/esp_modem/include/cxx_include/esp_modem_dte.hpp @@ -53,7 +53,7 @@ class DTE : public CommandableIf { explicit DTE(const esp_modem_dte_config *config, std::unique_ptr t, std::unique_ptr s); explicit DTE(std::unique_ptr t, std::unique_ptr s); - ~DTE() = default; + ~DTE(); /** * @brief Writing to the underlying terminal diff --git a/components/esp_modem/include/cxx_include/esp_modem_terminal.hpp b/components/esp_modem/include/cxx_include/esp_modem_terminal.hpp index 07e72b4156..e41bbe7800 100644 --- a/components/esp_modem/include/cxx_include/esp_modem_terminal.hpp +++ b/components/esp_modem/include/cxx_include/esp_modem_terminal.hpp @@ -45,11 +45,13 @@ class Terminal { void set_error_cb(std::function f) { + Scoped l(cb_lock); on_error = std::move(f); } virtual void set_read_cb(std::function f) { + Scoped l(cb_lock); on_read = std::move(f); } @@ -74,6 +76,14 @@ class Terminal { virtual void stop() = 0; protected: + /** + * @brief Serializes (re)assignment of on_read/on_error (set_read_cb/set_error_cb) against their + * invocation by the terminal's RX task. Recursive, so a callback may re-enter set_read_cb() + * (e.g. DTE clears its own read callback from within the callback). Holding this lock while + * clearing a callback guarantees no callback is in flight afterwards, which is what makes it + * safe to tear down the state the callback captures. + */ + Lock cb_lock{}; std::function on_read; std::function on_error; }; diff --git a/components/esp_modem/src/esp_modem_cmux.cpp b/components/esp_modem/src/esp_modem_cmux.cpp index 8089d66e6c..25bb7ed0cf 100644 --- a/components/esp_modem/src/esp_modem_cmux.cpp +++ b/components/esp_modem/src/esp_modem_cmux.cpp @@ -124,6 +124,10 @@ bool CMux::data_available(uint8_t *data, size_t len) if (data && (type & FT_UIH) == FT_UIH && len > 0 && dlci > 0) { // valid payload on a virtual term int virtual_term = dlci - 1; if (virtual_term < MAX_TERMINALS_NUM) { + // Hold cb_lock (not the state lock) across the read_cb check and invocation, so the + // callback cannot be reassigned/cleared (set_read_cb()/teardown) while we're using it, + // without blocking write() during the (potentially long) upcall. + Scoped l(cb_lock); if (read_cb[virtual_term] == nullptr) { // ignore all virtual terminal's data before we completely establish CMUX ESP_LOG_BUFFER_HEXDUMP("CMUX Rx before init", data, len, ESP_LOG_DEBUG); @@ -148,6 +152,7 @@ bool CMux::data_available(uint8_t *data, size_t len) } else if (data == nullptr && dlci > 0) { int virtual_term = dlci - 1; if (virtual_term < MAX_TERMINALS_NUM) { + Scoped l(cb_lock); if (read_cb[virtual_term] == nullptr) { // silently ignore this CMUX frame (not finished entering CMUX, yet) return true; @@ -430,6 +435,18 @@ bool CMux::deinit() return true; } +CMux::~CMux() +{ + // The underlying terminal's RX task drives on_cmux_data(), which touches CMux state (lock, + // buffer, read_cb[]). Detach it before that state is destroyed, otherwise a late RX event + // would use this CMux after free. set_read_cb()/set_error_cb() are synchronized against the + // RX task, so once they return no callback is in flight. + if (term) { + term->set_read_cb(nullptr); + term->set_error_cb(nullptr); + } +} + bool CMux::init() { frame_header_offset = 0; @@ -494,6 +511,7 @@ int CMux::write(int virtual_term, uint8_t *data, size_t len) void CMux::set_read_cb(int inst, std::function f) { + Scoped l(cb_lock); if (inst < MAX_TERMINALS_NUM) { read_cb[inst] = std::move(f); } @@ -519,3 +537,10 @@ bool CMux::recover() recover_protocol(protocol_mismatch_reason::UNKNOWN); return true; } + +void CMux::stop() +{ + if (term) { + term->stop(); + } +} diff --git a/components/esp_modem/src/esp_modem_dte.cpp b/components/esp_modem/src/esp_modem_dte.cpp index 0ca8604ba3..1fd93f5aa6 100644 --- a/components/esp_modem/src/esp_modem_dte.cpp +++ b/components/esp_modem/src/esp_modem_dte.cpp @@ -61,6 +61,28 @@ DTE::DTE(std::unique_ptr t, std::unique_ptr s) } +DTE::~DTE() +{ + // The terminals' RX tasks invoke read/error callbacks that capture this DTE (and reference + // command_cb.line_lock). Members are destroyed after this body runs, and command_cb is + // destroyed first of all (declared last), so we must guarantee no callback can fire into a + // half-destroyed DTE. + // + // stop() quiesces the RX task (for CMUX, CMuxInstance::stop() forwards to the physical + // terminal); clearing the callbacks afterwards is synchronized against the RX task, so once + // these return no callback is in flight and none will start. + if (primary_term) { + primary_term->stop(); + primary_term->set_read_cb(nullptr); + primary_term->set_error_cb(nullptr); + } + if (secondary_term && secondary_term != primary_term) { + secondary_term->stop(); + secondary_term->set_read_cb(nullptr); + secondary_term->set_error_cb(nullptr); + } +} + void DTE::set_command_callbacks() { primary_term->set_read_cb([this](uint8_t *data, size_t len) { diff --git a/components/esp_modem/src/esp_modem_term_fs.cpp b/components/esp_modem/src/esp_modem_term_fs.cpp index e7cc7bfd09..0eea11c642 100644 --- a/components/esp_modem/src/esp_modem_term_fs.cpp +++ b/components/esp_modem/src/esp_modem_term_fs.cpp @@ -45,7 +45,16 @@ class FdTerminal : public Terminal { void stop() override { - signal.clear(TASK_START); + if (signal.is_any(TASK_STOPPED)) { + return; // already stopped (stop() is also called from the destructor) + } + signal.clear(TASK_START); // make the processing loop exit + signal.set(TASK_STOP); // release the task if it never started + // Block until the task confirms it has left the processing loop, so no read callback is + // in flight when the task is deleted . + if (!signal.wait_any(TASK_STOPPED, stop_timeout_ms)) { + ESP_LOGW(TAG, "Timed out waiting for fs task to stop"); + } } int write(uint8_t *data, size_t len) override; @@ -54,17 +63,28 @@ class FdTerminal : public Terminal { void set_read_cb(std::function f) override { + Scoped l(cb_lock); on_read = std::move(f); - signal.set(TASK_PARAMS); } private: void task(); + // Invoke on_read under cb_lock so it cannot be reassigned/cleared (set_mode()/teardown on + // another task) while we're executing it; re-check under the lock. + void notify_read() + { + Scoped l(cb_lock); + if (on_read) { + on_read(nullptr, 0); + } + } + static const size_t TASK_INIT = SignalGroup::bit0; static const size_t TASK_START = SignalGroup::bit1; static const size_t TASK_STOP = SignalGroup::bit2; - static const size_t TASK_PARAMS = SignalGroup::bit3; + static const size_t TASK_STOPPED = SignalGroup::bit3; /*!< Set by the task once it has left the processing loop */ + static const uint32_t stop_timeout_ms = 2000; /*!< Max wait for a graceful stop (must exceed the select() timeout) */ File f; SignalGroup signal; @@ -86,13 +106,22 @@ FdTerminal::FdTerminal(const esp_modem_dte_config *config) : { auto t = static_cast(p); t->task(); - Task::Delete(); + // task() returns only after stop() requested it. Acknowledge that the processing loop has + // been left -- this is the last access to any member. + t->signal.set(TASK_STOPPED); +#if !defined(CONFIG_IDF_TARGET_LINUX) + // FreeRTOS: a task function must not return. Idle until the owner (Task destructor) deletes + // us; we deliberately do not self-delete, to avoid racing that delete. + while (true) { + Task::Delay(3600 * 1000); + } +#endif + // Linux: returning ends the std::thread, which the Task destructor join()s. }) {} void FdTerminal::task() { - std::function on_read_priv = nullptr; signal.set(TASK_INIT); signal.wait_any(TASK_START | TASK_STOP, portMAX_DELAY); if (signal.is_any(TASK_STOP)) { @@ -110,10 +139,6 @@ void FdTerminal::task() FD_SET(f.fd, &rfds); s = select(f.fd + 1, &rfds, nullptr, nullptr, &tv); - if (signal.is_any(TASK_PARAMS)) { - on_read_priv = on_read; - signal.clear(TASK_PARAMS); - } if (s < 0) { break; @@ -121,9 +146,7 @@ void FdTerminal::task() // ESP_LOGV(TAG, "Select exited with timeout"); } else { if (FD_ISSET(f.fd, &rfds)) { - if (on_read_priv) { - on_read_priv(nullptr, 0); - } + notify_read(); } } Task::Relinquish(); diff --git a/components/esp_modem/src/esp_modem_uart.cpp b/components/esp_modem/src/esp_modem_uart.cpp index ebe31fdded..0e363004ea 100644 --- a/components/esp_modem/src/esp_modem_uart.cpp +++ b/components/esp_modem/src/esp_modem_uart.cpp @@ -46,7 +46,12 @@ class UartTerminal : public Terminal { event_queue(), uart(&config->uart_config, &event_queue, -1), signal(), task_handle(config->task_stack_size, config->task_priority, this, s_task) {} - ~UartTerminal() override = default; + ~UartTerminal() override + { + // Stop the RX task gracefully (and synchronously) before our members are torn down, + // so it is never force-deleted while inside a callback or blocked in the UART driver. + UartTerminal::stop(); + } void start() override { @@ -55,7 +60,16 @@ class UartTerminal : public Terminal { void stop() override { - signal.set(TASK_STOP); + if (signal.is_any(TASK_STOPPED)) { + return; // already stopped (stop() is also called from the destructor) + } + signal.clear(TASK_START); // make the processing loop exit + signal.set(TASK_STOP); // release the task if it never started + // Block until the task confirms it has left the processing loop: no read/error callback + // is in flight, and it is not blocked in the UART driver, so it is safe to delete. + if (!signal.wait_any(TASK_STOPPED, stop_timeout_ms)) { + ESP_LOGW(TAG, "Timed out waiting for uart_task to stop"); + } } int write(uint8_t *data, size_t len) override; @@ -64,6 +78,7 @@ class UartTerminal : public Terminal { void set_read_cb(std::function f) override { + Scoped l(cb_lock); on_read = std::move(f); } @@ -72,8 +87,13 @@ class UartTerminal : public Terminal { { auto t = static_cast(task_param); t->task(); - t->task_handle.task_handle = nullptr; - vTaskDelete(nullptr); + // task() returns only after stop() requested it. Acknowledge that the processing loop has + // been left -- this is the last access to any member -- then idle until the owner deletes + // us from ~uart_task(). We deliberately do not self-delete, to avoid racing that delete. + t->signal.set(TASK_STOPPED); + while (true) { + vTaskDelay(portMAX_DELAY); + } } void task(); @@ -82,6 +102,24 @@ class UartTerminal : public Terminal { return xQueueReceive(event_queue, &event, pdMS_TO_TICKS(time_ms)); } + // Invoke the read/error callbacks under cb_lock so they cannot be reassigned or cleared + // (by set_mode()/teardown on another task) while we're executing them. Re-check under the + // lock, since the callback may have been cleared between the outer check and acquiring it. + void notify_read(size_t len) + { + Scoped l(cb_lock); + if (on_read) { + on_read(nullptr, len); + } + } + void notify_error(terminal_error err) + { + Scoped l(cb_lock); + if (on_error) { + on_error(err); + } + } + void reset_events() { uart_flush_input(uart.port); @@ -91,6 +129,8 @@ class UartTerminal : public Terminal { static const size_t TASK_INIT = BIT0; static const size_t TASK_START = BIT1; static const size_t TASK_STOP = BIT2; + static const size_t TASK_STOPPED = BIT3; /*!< Set by the task once it has left the processing loop */ + static const uint32_t stop_timeout_ms = 1000; /*!< Max wait for a graceful stop before forcing deletion */ QueueHandle_t event_queue; uart_resource uart; @@ -121,41 +161,31 @@ void UartTerminal::task() switch (event.type) { case UART_DATA: uart_get_buffered_data_len(uart.port, &len); - if (len && on_read) { - on_read(nullptr, len); + if (len) { + notify_read(len); } break; case UART_FIFO_OVF: ESP_LOGW(TAG, "HW FIFO Overflow"); - if (on_error) { - on_error(terminal_error::BUFFER_OVERFLOW); - } + notify_error(terminal_error::BUFFER_OVERFLOW); reset_events(); break; case UART_BUFFER_FULL: ESP_LOGW(TAG, "Ring Buffer Full"); - if (on_error) { - on_error(terminal_error::BUFFER_OVERFLOW); - } + notify_error(terminal_error::BUFFER_OVERFLOW); reset_events(); break; case UART_BREAK: ESP_LOGW(TAG, "Rx Break"); - if (on_error) { - on_error(terminal_error::UNEXPECTED_CONTROL_FLOW); - } + notify_error(terminal_error::UNEXPECTED_CONTROL_FLOW); break; case UART_PARITY_ERR: ESP_LOGE(TAG, "Parity Error"); - if (on_error) { - on_error(terminal_error::CHECKSUM_ERROR); - } + notify_error(terminal_error::CHECKSUM_ERROR); break; case UART_FRAME_ERR: ESP_LOGE(TAG, "Frame Error"); - if (on_error) { - on_error(terminal_error::UNEXPECTED_CONTROL_FLOW); - } + notify_error(terminal_error::UNEXPECTED_CONTROL_FLOW); break; default: ESP_LOGW(TAG, "unknown uart event type: %d", event.type); @@ -163,8 +193,8 @@ void UartTerminal::task() } } else { uart_get_buffered_data_len(uart.port, &len); - if (len && on_read) { - on_read(nullptr, len); + if (len) { + notify_read(len); } } }