fix(modem): spinlock_acquire assert (use-after-free) when tearing down / switching modes while RX data is in flight

[thiker5]

Description

Destroying the DTE/DCE (or calling set_mode() / exit_data() / exit_cmux())
while the modem link is still receiving data can crash with a FreeRTOS spinlock
corruption assert. The crash is a use-after-free of a terminal callback (or the
mutex it takes) by the terminal RX task.

Backtrace
assert failed: spinlock_acquire spinlock.h:123 ((result == SPINLOCK_FREE) == (lock->count == 0))
0x400818ef: panic_abort at panic.c:407
0x4008b815: esp_system_abort at system_api.c:112
0x40092645: __assert_func at assert.c:85
0x4008ea8d: spinlock_acquire at spinlock.h:123
(inlined by) vPortEnterCritical at port.c:448
0x4008c6d9: xQueueSemaphoreTake at queue.c:1466
0x4008c848: xQueueTakeMutexRecursive at queue.c:687
0x400f756a: esp_modem::Lock::lock() at esp_modem_primitives_freertos.cpp:33
0x400fa9a1: esp_modem::Scoped<esp_modem::Lock>::Scoped() at esp_modem_primitives.hpp:53
(inlined by) operator() at esp_modem_dte.cpp:67 // the read callback taking command_cb.line_lock
0x400f7c41: esp_modem::UartTerminal::task() at esp_modem_uart.cpp:167
0x400f7c51: esp_modem::UartTerminal::s_task(void*) at esp_modem_uart.cpp:74
0x4008e8e6: vPortTaskWrapper at port.c:168

Root cause
Terminal callbacks are mutated without any synchronization against the RX task
that invokes them:

Terminal::set_read_cb()/set_error_cb() and CMux::set_read_cb() reassign a
std::function (or read_cb[]) with no lock.
set_mode()/set_command_callbacks() swap and rebind these during DATA/CMUX
transitions; ~DTE lets command_cb (which owns line_lock) be destroyed
while the RX task is still running.
internal_lock (held during set_mode) does not protect this, because the
RX task (UartTerminal::task()) never takes internal_lock.
So the RX task can be executing a callback whose std::function target is being
destroyed, or whose captured this/command_cb.line_lock has already been freed
— it then locks a freed/garbage mutex and the spinlock consistency assert fires.

Separately, stop() is effectively a no-op for loop exit (it sets TASK_STOP
but the loop checks TASK_START), so the RX task is always force-deleted via
vTaskDelete() — potentially mid-callback or inside the UART driver — and
FdTerminal additionally has a self-delete vs owner-delete double-delete window.

Reproduction
Bring up PPP/CMUX so the modem is actively pushing RX data.
Tear down the DCE/DTE (or call exit_data() / set_mode(COMMAND_MODE))
while data is still arriving.
Intermittent crash with the assert above (timing/SMP dependent).
Note: the below will have the modem crash in no time
In file : esp_modem_uart.cpp/get_event --> change the timeout to 1
*Initialize via
cell_netif= esp_netif_new(&config)
dce = esp_modem_new(...)
esp_event_handler_instance_register(...)
Call long AT command (AT+COPS? for example) with immediate return (timeout 1 ms) then
*Destroy
esp_event_handler_instance_unregister(....)
sct_esp_modem_destroy(dce);
esp_netif_destroy(cell_netif);
cell_netif= NULL;
dce = NULL;

Related

Testing

---

Checklist

Before submitting a Pull Request, please ensure the following:

• [x ] 🚨 This PR does not introduce breaking changes.
• All CI checks (GH Actions) pass.
• Documentation is updated as needed.
• Tests are updated or added as necessary.
• [ x] Code is well-commented, especially in complex areas.
• Git history is clean — commits are squashed to the minimum necessary.

---

Note

High Risk
Touches modem RX threading, callback lifetime, and FreeRTOS task teardown on all UART/VFS/CMUX paths; incorrect ordering could still race or hang stop, but the change directly addresses security-relevant use-after-free crashes during destroy and mode transitions.

Overview
Fixes intermittent FreeRTOS spinlock_acquire asserts when destroying the modem stack or switching modes while UART/CMUX traffic is still arriving. The RX task could run read/error callbacks after their std::function targets or captured DTE state (e.g. command_cb.line_lock) were torn down.

Synchronization: Terminal and CMUX gain a dedicated cb_lock so set_read_cb / set_error_cb (and CMUX read_cb[]) are serialized against RX-task invocation; UART/VFS invoke callbacks via notify_read / notify_error under that lock. CMUX keeps cb_lock separate from lock so long PPP upcalls do not block CMUX writes.

Graceful RX shutdown: UART and VFS stop() now clears TASK_START, signals stop, and blocks until TASK_STOPPED instead of relying on a broken stop path and vTaskDelete mid-callback. Tasks idle after exiting the loop (no self-delete race). ~DTE and ~CMux call stop() then clear callbacks; CMuxInstance::stop() forwards to the physical terminal. ~UartTerminal stops synchronously in the destructor.

CMUX teardown: New Kconfig ESP_MODEM_CMUX_DELAY_AFTER_NETIF_DISCONNECT adds an optional pause after PPP disconnect before exiting CMUX to command mode.

^{Reviewed by Cursor Bugbot for commit 323bc79. Bugbot is set up for automated code reviews on this repo. Configure here.}

[david-cermak]

Thanks for fixing the race condition!

[david-cermak]

Hi @thiker5

Thanks for this PR and sorry for missing it. I think we can merge it as is, I'll just need to "forward-port" the cb_lock change to master.
Also, If you can rebase on top of release-v1.4 branch? I've made some changes and cleaned up the CI, so the checks should pass now.