From 7778bf1ecffe216c27cef5dd5f3179deea25ae57 Mon Sep 17 00:00:00 2001 From: Jerry Kezar Date: Wed, 3 Jun 2026 23:59:11 +0000 Subject: [PATCH 1/2] console: fix interactive-command use-after-free / leak on session disconnect An interactive command that has taken over the session input (by registering an insert callback) may keep resources, or a background task, bound to the launching session's OvmsWriter. If the SSH/Telnet session is closed or dropped by any means other than the command's normal exit, the writer (and the transport it points to) was freed while the command still referenced it: - "vfs tail " in follow mode (OCS_RunLoop) runs its Service() loop on its own task, writing through the raw OvmsWriter*. The next writer->write() after the console was freed dereferenced a zeroed vtable -> LoadProhibited crash (EXCVADDR 0x10, task "tail", VfsTailCommand::Service()). - "vfs edit" and "enable" leaked their heap-allocated state (editor_state / PasswordContext) when the session dropped mid-input. Add a generic termination-handler registry to OvmsWriter, parallel to the insert-callback registry: an interactive command registers a termination handler alongside its insert callback, and a console invokes RunTerminationCallback() during teardown -- before freeing any transport it owns -- so the handler can release resources / stop background tasks while the writer is still valid. At most one handler is active at a time, mirroring the single active insert callback. - OvmsCommandTask registers a handler that requests its task to stop and joins it (cooperative stop+join, no vTaskDelete) before the writer is freed; covers any follow-mode command, not just vfs tail. - vfs edit / enable register handlers that free their allocations. - The y/n insert callbacks with no allocated state (module factory reset, OTA partition prompts, test echo) need no handler. Called from ~ConsoleSSH/~ConsoleTelnet before they free their transport, from ~OvmsConsole before it frees the shared queue (covers the Bluetooth console), with a final backstop in ~OvmsWriter. Replaces the earlier OvmsCommandTask-specific approach per review: the shared abstraction for an interactive command owning the session is the insert callback, not OvmsCommandTask. --- .../console_ssh/src/console_ssh.cpp | 4 ++ .../console_telnet/src/console_telnet.cpp | 4 ++ .../components/vfsedit/src/vfsedit.cpp | 11 +++ vehicle/OVMS.V3/main/ovms_command.cpp | 72 +++++++++++++++++++ vehicle/OVMS.V3/main/ovms_command.h | 20 ++++++ vehicle/OVMS.V3/main/ovms_console.cpp | 5 ++ 6 files changed, 116 insertions(+) diff --git a/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp b/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp index 88e99ecd2..a19e49c05 100644 --- a/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp +++ b/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp @@ -340,6 +340,10 @@ ConsoleSSH::ConsoleSSH(OvmsSSH* server, struct mg_connection* nc) ConsoleSSH::~ConsoleSSH() { + // Clean up any interactive command (e.g. a "vfs tail" follow-mode task) bound + // to this console before freeing the transport it writes to, otherwise it + // would dereference a freed writer/transport. + RunTerminationCallback(); WOLFSSH* ssh = m_ssh; m_ssh = NULL; wolfSSH_free(ssh); diff --git a/vehicle/OVMS.V3/components/console_telnet/src/console_telnet.cpp b/vehicle/OVMS.V3/components/console_telnet/src/console_telnet.cpp index ec7fb0ca1..708b1c3d5 100644 --- a/vehicle/OVMS.V3/components/console_telnet/src/console_telnet.cpp +++ b/vehicle/OVMS.V3/components/console_telnet/src/console_telnet.cpp @@ -182,6 +182,10 @@ ConsoleTelnet::ConsoleTelnet(struct mg_connection* nc) ConsoleTelnet::~ConsoleTelnet() { + // Clean up any interactive command (e.g. a "vfs tail" follow-mode task) bound + // to this console before freeing the transport it writes to, otherwise it + // would dereference a freed writer/transport. + RunTerminationCallback(); telnet_t *telnet = m_telnet; m_telnet = NULL; telnet_free(telnet); diff --git a/vehicle/OVMS.V3/components/vfsedit/src/vfsedit.cpp b/vehicle/OVMS.V3/components/vfsedit/src/vfsedit.cpp index 9eb8b0f1d..f21553184 100644 --- a/vehicle/OVMS.V3/components/vfsedit/src/vfsedit.cpp +++ b/vehicle/OVMS.V3/components/vfsedit/src/vfsedit.cpp @@ -38,6 +38,15 @@ size_t vfs_edit_write(struct editor_state* E, const char *buf, size_t nbyte) return writer->write(buf,nbyte); } +// Release the editor state if the session is closed while the editor is still +// open (the insert callback would otherwise leak the buffer it allocated). +void vfs_edit_terminate(OvmsWriter* writer, void* ctx) + { + struct editor_state* ed = (struct editor_state*)ctx; + editor_free(ed); + free(ed); + } + bool vfs_edit_insert(OvmsWriter* writer, void* ctx, char ch) { struct editor_state* ed = (struct editor_state*)ctx; @@ -47,6 +56,7 @@ bool vfs_edit_insert(OvmsWriter* writer, void* ctx, char ch) { editor_free(ed); free(ed); + writer->DeregisterTerminationCallback(vfs_edit_terminate); return false; } else @@ -68,4 +78,5 @@ void vfs_edit(int verbosity, OvmsWriter* writer, OvmsCommand* cmd, int argc, con editor_open(ed,argv[0]); writer->RegisterInsertCallback(vfs_edit_insert, ed); + writer->RegisterTerminationCallback(vfs_edit_terminate, ed); } diff --git a/vehicle/OVMS.V3/main/ovms_command.cpp b/vehicle/OVMS.V3/main/ovms_command.cpp index 691f7fe04..c91f11e08 100644 --- a/vehicle/OVMS.V3/main/ovms_command.cpp +++ b/vehicle/OVMS.V3/main/ovms_command.cpp @@ -80,10 +80,48 @@ OvmsWriter::OvmsWriter() m_insert = NULL; m_userData = NULL; m_monitoring = false; + m_termination = NULL; + m_termData = NULL; } OvmsWriter::~OvmsWriter() { + // Backstop: clean up any interactive command still bound to this writer. + // Consoles that own a transport must call this earlier (before freeing that + // transport); here it only guards writers that did not. + RunTerminationCallback(); + } + +void OvmsWriter::RegisterTerminationCallback(TerminationCallback cb, void* ctx) + { + m_termination = cb; + m_termData = ctx; + } + +void OvmsWriter::DeregisterTerminationCallback(TerminationCallback cb) + { + if (m_termination == cb) + { + m_termination = NULL; + m_termData = NULL; + } + } + +void OvmsWriter::RunTerminationCallback() + { + // Invoked during console/writer teardown. Gives the active interactive + // command (if any) a chance to release resources / stop background tasks + // while this writer is still valid; the handler blocks until that is done. + // Idempotent: a follow-mode task clears its own registration before this + // returns, and the explicit clear afterwards makes repeat calls (e.g. a + // console destructor followed by the ~OvmsWriter backstop) a no-op. + TerminationCallback cb = m_termination; + void* data = m_termData; + if (cb == NULL) + return; + cb(this, data); + m_termination = NULL; + m_termData = NULL; } void OvmsWriter::Exit() @@ -683,6 +721,13 @@ typedef struct int tries; } PasswordContext; +// Release the password prompt context if the session is closed while "enable" +// is still waiting for input (the insert callback would otherwise leak it). +void enableTerminate(OvmsWriter* writer, void* v) + { + delete (PasswordContext*)v; + } + bool enableInsert(OvmsWriter* writer, void* v, char ch) { PasswordContext* pc = (PasswordContext*)v; @@ -693,6 +738,7 @@ bool enableInsert(OvmsWriter* writer, void* v, char ch) { writer->SetSecure(true); writer->printf("\nSecure mode"); + writer->DeregisterTerminationCallback(enableTerminate); delete pc; return false; } @@ -700,6 +746,7 @@ bool enableInsert(OvmsWriter* writer, void* v, char ch) { writer->printf("\nError: %d incorrect password attempts", pc->tries); vTaskDelay(5000 / portTICK_PERIOD_MS); + writer->DeregisterTerminationCallback(enableTerminate); delete pc; return false; } @@ -709,6 +756,7 @@ bool enableInsert(OvmsWriter* writer, void* v, char ch) } if (ch == 'C'-0100) { + writer->DeregisterTerminationCallback(enableTerminate); delete pc; return false; } @@ -736,6 +784,7 @@ void enable(int verbosity, OvmsWriter* writer, OvmsCommand* cmd, int argc, const pc->tries = 0; writer->printf("Password:"); writer->RegisterInsertCallback(enableInsert, pc); + writer->RegisterTerminationCallback(enableTerminate, pc); } } @@ -1744,8 +1793,13 @@ bool OvmsCommandTask::Run() case OCS_RunLoop: // start task: writer->RegisterInsertCallback(Terminator, (void*) this); + // Register the termination handler before Instantiate() so a console + // tearing down can never miss the task (it could otherwise start, finish + // and deregister itself before we registered it). + writer->RegisterTerminationCallback(TerminationHandler, (void*) this); if (!Instantiate()) { + writer->DeregisterTerminationCallback(TerminationHandler); delete this; return false; } @@ -1772,6 +1826,9 @@ OvmsCommandTask::~OvmsCommandTask() if (m_state == OCS_StopRequested) writer->puts("^C"); writer->DeregisterInsertCallback(Terminator); + // Must be the last access to writer: it releases a console blocked in + // RunTerminationCallback(), which may then free the writer. + writer->DeregisterTerminationCallback(TerminationHandler); if (argv) { for (int i=0; i < argc; i++) @@ -1787,6 +1844,21 @@ bool OvmsCommandTask::Terminator(OvmsWriter* writer, void* userdata, char ch) return true; } +void OvmsCommandTask::TerminationHandler(OvmsWriter* writer, void* userdata) + { + // Called on the console's teardown task when the session is closing while a + // follow-mode (OCS_RunLoop) task is still running. Ask it to stop, then wait + // until it has fully exited: ~OvmsCommandTask deregisters this handler as its + // last access to the writer, so once the registration is cleared the task can + // no longer dereference the writer. Cooperative stop+join (no vTaskDelete) so + // the task is never force-killed mid-write. This relies on no concurrent + // Ctrl-C termination, which holds on the connection-close path. + OvmsCommandTask* task = (OvmsCommandTask*) userdata; + task->RequestStop(); + while (writer->HasTerminationCallback(TerminationHandler)) + vTaskDelay(pdMS_TO_TICKS(20)); + } + bool option_completer_t::check_param(char param, bool has_more) { diff --git a/vehicle/OVMS.V3/main/ovms_command.h b/vehicle/OVMS.V3/main/ovms_command.h index f6e6ac208..213ec1969 100644 --- a/vehicle/OVMS.V3/main/ovms_command.h +++ b/vehicle/OVMS.V3/main/ovms_command.h @@ -56,6 +56,7 @@ class OvmsCommand; class OvmsCommandMap; class LogBuffers; typedef bool (*InsertCallback)(OvmsWriter* writer, void* userData, char); +typedef void (*TerminationCallback)(OvmsWriter* writer, void* userData); class OvmsWriter { @@ -80,6 +81,21 @@ class OvmsWriter virtual void finalise() {} virtual void ProcessChar(char c) {} + public: + // Termination handler registry, parallel to the insert callback registry. + // An interactive command that has taken over the session input (by + // registering an insert callback) may also register a termination handler. + // A console invokes RunTerminationCallback() during teardown — before it + // frees any transport it owns — so the command can release resources or stop + // background tasks while this writer is still valid. At most one handler is + // active at a time (input is owned by a single interactive command). The + // handler must block until cleanup is complete (e.g. a follow-mode task must + // be joined) so the writer is safe to free once it returns. + void RegisterTerminationCallback(TerminationCallback cb, void* ctx); + void DeregisterTerminationCallback(TerminationCallback cb); + void RunTerminationCallback(); + bool HasTerminationCallback(TerminationCallback cb) { return m_termination == cb; } + public: // Used to notify the writer of a migration of a file within the VFS virtual const std::string GetPath() { return std::string(""); } @@ -98,6 +114,8 @@ class OvmsWriter InsertCallback m_insert; void* m_userData; bool m_monitoring; + TerminationCallback m_termination; // active interactive-command teardown handler, or NULL + void* m_termData; }; template @@ -326,8 +344,10 @@ class OvmsCommandTask : public TaskBase virtual OvmsCommandState_t Prepare(); bool Run(); static bool Terminator(OvmsWriter* writer, void* userdata, char ch); + static void TerminationHandler(OvmsWriter* writer, void* userdata); bool IsRunning() { return m_state == OCS_RunLoop; } bool IsTerminated() { return m_state == OCS_StopRequested; } + void RequestStop() { m_state = OCS_StopRequested; } protected: int verbosity; diff --git a/vehicle/OVMS.V3/main/ovms_console.cpp b/vehicle/OVMS.V3/main/ovms_console.cpp index 95075f136..f6509fa95 100644 --- a/vehicle/OVMS.V3/main/ovms_console.cpp +++ b/vehicle/OVMS.V3/main/ovms_console.cpp @@ -66,6 +66,11 @@ OvmsConsole::OvmsConsole() OvmsConsole::~OvmsConsole() { + // Clean up any interactive command (e.g. a "vfs tail" follow-mode task) bound + // to this console before freeing its queues, so it can no longer dereference + // this writer. SSH/Telnet already do this before freeing their own transport; + // this covers consoles that don't (e.g. Bluetooth) and is a no-op once done. + RunTerminationCallback(); // stop receiving log events: m_ready = false; MyCommandApp.DeregisterConsole(this); From c1407a60f4fc396747d1c0c148c46c75b2385abb Mon Sep 17 00:00:00 2001 From: Jerry Kezar Date: Sat, 6 Jun 2026 00:33:29 +0000 Subject: [PATCH 2/2] console_ssh: make follow-mode session teardown deadlock-safe (defer-and-reap) The termination-registry cleanup joined the follow-mode command task synchronously during ~ConsoleSSH. But ConsoleSSH::write() takes the Mongoose lock that the teardown thread already holds (it runs on the Mongoose task, under that lock for the whole mg_mgr_poll), so dropping a session mid "vfs tail" deadlocked and wedged the SSH server. Instead, when an SSH connection closes with a follow-mode task still bound: mark the console closing (so its in-flight write()/SendCallback bail without touching the now-freed connection), ask the task to stop WITHOUT joining, and defer the console's destruction to a reap pass on the listener poll once the task has left the write path. SendCallback is re-keyed off the ConsoleSSH* (not the mg_connection) so it can never dereference a freed connection. The shared termination-handler join is unchanged for Telnet/Async/Bluetooth, which do not take the Mongoose lock in their send path. On-vehicle validated (hw3.3-5 Solterra): repeated abrupt SSH disconnects during an active "vfs tail" no longer wedge or crash the module; free heap flat over 15 cycles (no leak); idle tail, Ctrl-C abort and one-shot "vfs tail -N" unaffected. --- .../console_ssh/src/console_ssh.cpp | 40 ++++++++++++++++--- .../components/console_ssh/src/console_ssh.h | 6 +++ vehicle/OVMS.V3/main/ovms_command.cpp | 10 +++++ vehicle/OVMS.V3/main/ovms_command.h | 6 ++- vehicle/OVMS.V3/main/ovms_vfs.cpp | 4 ++ 5 files changed, 58 insertions(+), 8 deletions(-) diff --git a/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp b/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp index a19e49c05..52c72f40b 100644 --- a/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp +++ b/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.cpp @@ -96,6 +96,15 @@ void OvmsSSH::EventHandler(struct mg_connection *nc, int ev, void *p) case MG_EV_POLL: { + // Reap consoles whose follow task has exited (GetFollowModeTask() now NULL). + for (auto it = m_reaping.begin(); it != m_reaping.end(); ) + { + ConsoleSSH* z = *it; + if (z->GetFollowModeTask() == NULL) + { delete z; it = m_reaping.erase(it); } + else + ++it; + } //ESP_EARLY_LOGV(tag, "Event MG_EV_ACCEPT conn %p, data %p", nc, p); ConsoleSSH* child = (ConsoleSSH*)nc->user_data; if (child) @@ -145,7 +154,21 @@ void OvmsSSH::EventHandler(struct mg_connection *nc, int ev, void *p) ESP_EARLY_LOGV(tag, "Event MG_EV_CLOSE conn %p, data %p", nc, p); ConsoleSSH* child = (ConsoleSSH*)nc->user_data; if (child) - delete child; + { + OvmsCommandTask* ft = child->GetFollowModeTask(); + if (ft) + { + // Follow task still running: we cannot join here (we hold the Mongoose + // lock its write() needs). Mark closing so its in-flight write bails, + // ask it to stop, and defer delete until it has left the write path. + child->SetClosing(); + ft->RequestStop(); + m_reaping.push_back(child); + } + else + delete child; // no follow task: synchronous termination + free (unchanged) + } + nc->user_data = NULL; } break; @@ -315,6 +338,7 @@ ConsoleSSH::ConsoleSSH(OvmsSSH* server, struct mg_connection* nc) m_ssh = NULL; m_state = ACCEPT; m_drain = 0; + m_closing = false; m_sent = true; m_rekey = false; m_needDir = false; @@ -331,7 +355,7 @@ ConsoleSSH::ConsoleSSH(OvmsSSH* server, struct mg_connection* nc) wolfSSH_SetIORecv(m_server->ctx(), ::RecvCallback); wolfSSH_SetIOSend(m_server->ctx(), ::SendCallback); wolfSSH_SetIOReadCtx(m_ssh, this); - wolfSSH_SetIOWriteCtx(m_ssh, m_connection); + wolfSSH_SetIOWriteCtx(m_ssh, this); wolfSSH_SetUserAuthCtx(m_ssh, this); /* Use the session object for its own highwater callback ctx */ wolfSSH_SetHighwaterCtx(m_ssh, (void*)m_ssh); @@ -963,7 +987,10 @@ int ConsoleSSH::printf(const char* fmt, ...) ssize_t ConsoleSSH::write(const void *buf, size_t nbyte) { - if (!m_ssh || (m_connection->flags & MG_F_SEND_AND_CLOSE)) + // m_closing is checked first on purpose: once the connection is closing the + // mongoose nc (m_connection) may already be freed, so short-circuit before + // dereferencing it. Same reason SendCallback bails on IsClosing() first. + if (m_closing || !m_ssh || (m_connection->flags & MG_F_SEND_AND_CLOSE)) return 0; int ret = 0; @@ -1072,11 +1099,12 @@ int ConsoleSSH::RecvCallback(char* buf, uint32_t size) int SendCallback(WOLFSSH* ssh, void* data, word32 size, void* ctx) { - mg_connection* nc = (mg_connection*) ctx; - if (!nc) return 0; - ConsoleSSH* console = (ConsoleSSH*) nc->user_data; + ConsoleSSH* console = (ConsoleSSH*) ctx; if (!console) return 0; auto mglock = console->MongooseLock(); + if (console->IsClosing()) // connection closing: don't touch the (freed) nc + return WS_CBIO_ERR_WANT_WRITE; + mg_connection* nc = console->GetConnection(); nc->flags |= MG_F_SEND_IMMEDIATELY; size_t ret = mg_send(nc, (char*)data, size); if (ret == 0) diff --git a/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.h b/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.h index bb0830c17..33cba6a26 100644 --- a/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.h +++ b/vehicle/OVMS.V3/components/console_ssh/src/console_ssh.h @@ -30,6 +30,7 @@ #ifndef __CONSOLE_SSH_H__ #define __CONSOLE_SSH_H__ +#include #include "freertos/FreeRTOS.h" #include "freertos/task.h" #include "ovms_console.h" @@ -57,6 +58,7 @@ class OvmsSSH : public MongooseClient protected: WOLFSSH_CTX* m_ctx; bool m_keyed; + std::list m_reaping; // consoles awaiting follow-task exit before delete }; class ConsoleSSH : public OvmsConsole, public MongooseClient @@ -80,6 +82,9 @@ class ConsoleSSH : public OvmsConsole, public MongooseClient ssize_t write(const void *buf, size_t nbyte); int RecvCallback(char* buf, uint32_t size); bool IsDraining() { return m_drain > 0; } + void SetClosing() { m_closing = true; } + bool IsClosing() { return m_closing; } + mg_connection* GetConnection() { return m_connection; } private: typedef struct {DIR* dir; size_t size;} Level; @@ -106,6 +111,7 @@ class ConsoleSSH : public OvmsConsole, public MongooseClient int m_index; // Index into m_buffer of data remaining to send int m_size; // Size of data remaining to send int m_drain; // Bytes discarded waiting for socket to drain + volatile bool m_closing; // true once the connection is closing; read cross-task bool m_sent; bool m_rekey; bool m_needDir; diff --git a/vehicle/OVMS.V3/main/ovms_command.cpp b/vehicle/OVMS.V3/main/ovms_command.cpp index c91f11e08..e286fc31c 100644 --- a/vehicle/OVMS.V3/main/ovms_command.cpp +++ b/vehicle/OVMS.V3/main/ovms_command.cpp @@ -124,6 +124,16 @@ void OvmsWriter::RunTerminationCallback() m_termData = NULL; } +OvmsCommandTask* OvmsWriter::GetFollowModeTask() + { + // Returns the bound follow-mode command task iff the active termination + // handler is OvmsCommandTask's (i.e. a separate task is running), else NULL. + // A NULL m_termination (already cleared by an exiting task) safely yields NULL. + if (m_termination == OvmsCommandTask::TerminationHandler) + return (OvmsCommandTask*) m_termData; + return NULL; + } + void OvmsWriter::Exit() { puts("This console cannot exit."); diff --git a/vehicle/OVMS.V3/main/ovms_command.h b/vehicle/OVMS.V3/main/ovms_command.h index 213ec1969..27ed201a6 100644 --- a/vehicle/OVMS.V3/main/ovms_command.h +++ b/vehicle/OVMS.V3/main/ovms_command.h @@ -52,6 +52,7 @@ #define COMMAND_RESULT_VERBOSE 65535 class OvmsWriter; +class OvmsCommandTask; class OvmsCommand; class OvmsCommandMap; class LogBuffers; @@ -95,6 +96,7 @@ class OvmsWriter void DeregisterTerminationCallback(TerminationCallback cb); void RunTerminationCallback(); bool HasTerminationCallback(TerminationCallback cb) { return m_termination == cb; } + OvmsCommandTask* GetFollowModeTask(); // bound follow-mode task, or NULL public: // Used to notify the writer of a migration of a file within the VFS @@ -114,7 +116,7 @@ class OvmsWriter InsertCallback m_insert; void* m_userData; bool m_monitoring; - TerminationCallback m_termination; // active interactive-command teardown handler, or NULL + volatile TerminationCallback m_termination; // active interactive-command teardown handler, or NULL (read cross-task) void* m_termData; }; @@ -355,7 +357,7 @@ class OvmsCommandTask : public TaskBase OvmsCommand* cmd; int argc; char** argv; - OvmsCommandState_t m_state; + volatile OvmsCommandState_t m_state; }; class OvmsCommandApp : public OvmsWriter diff --git a/vehicle/OVMS.V3/main/ovms_vfs.cpp b/vehicle/OVMS.V3/main/ovms_vfs.cpp index 1a306dcd0..0da6d9d5d 100644 --- a/vehicle/OVMS.V3/main/ovms_vfs.cpp +++ b/vehicle/OVMS.V3/main/ovms_vfs.cpp @@ -753,7 +753,11 @@ class VfsTailCommand : public OvmsCommandTask } lseek(fd, fpos, SEEK_SET); while ((len = read(fd, buf, sizeof buf)) > 0) + { writer->write(buf, len); + if (m_state != OCS_RunLoop) // stop/close requested: exit promptly + break; + } fpos = lseek(fd, 0, SEEK_CUR); close(fd); fd = -1;