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..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); @@ -340,6 +364,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); @@ -959,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; @@ -1068,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/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..e286fc31c 100644 --- a/vehicle/OVMS.V3/main/ovms_command.cpp +++ b/vehicle/OVMS.V3/main/ovms_command.cpp @@ -80,10 +80,58 @@ 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; + } + +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() @@ -683,6 +731,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 +748,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 +756,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 +766,7 @@ bool enableInsert(OvmsWriter* writer, void* v, char ch) } if (ch == 'C'-0100) { + writer->DeregisterTerminationCallback(enableTerminate); delete pc; return false; } @@ -736,6 +794,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 +1803,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 +1836,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 +1854,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..27ed201a6 100644 --- a/vehicle/OVMS.V3/main/ovms_command.h +++ b/vehicle/OVMS.V3/main/ovms_command.h @@ -52,10 +52,12 @@ #define COMMAND_RESULT_VERBOSE 65535 class OvmsWriter; +class OvmsCommandTask; 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 +82,22 @@ 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; } + OvmsCommandTask* GetFollowModeTask(); // bound follow-mode task, or NULL + 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 +116,8 @@ class OvmsWriter InsertCallback m_insert; void* m_userData; bool m_monitoring; + volatile TerminationCallback m_termination; // active interactive-command teardown handler, or NULL (read cross-task) + void* m_termData; }; template @@ -326,8 +346,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; @@ -335,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_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); 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;