console: fix vfs tail follow-mode use-after-free on session disconnect

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)

vehicle/OVMS.V3/components/console_ssh/src/console_ssh.h

@@ -30,6 +30,7 @@
 #ifndef __CONSOLE_SSH_H__
 #define __CONSOLE_SSH_H__
 
+#include <list>
 #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<ConsoleSSH*> 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;

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);

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);
   }

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,13 +748,15 @@ bool enableInsert(OvmsWriter* writer, void* v, char ch)
       {
       writer->SetSecure(true);
       writer->printf("\nSecure mode");
+      writer->DeregisterTerminationCallback(enableTerminate);
       delete pc;
       return false;
       }
     if (++pc->tries == 3)
       {
       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)
   {
 

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 <typename T>
@@ -326,16 +346,18 @@ 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;
     OvmsWriter* writer;
     OvmsCommand* cmd;
     int argc;
     char** argv;
-    OvmsCommandState_t m_state;
+    volatile OvmsCommandState_t m_state;
   };
 
 class OvmsCommandApp : public OvmsWriter

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);