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

[kezarjg]

vfs tail <file> is a documented VFS command (user guide; the CAN-logging guide also shows using tail to follow a growing log). With no line count it enters follow mode by default — "[tail: in follow mode, press Ctrl-C to abort]" — running its Service() loop on its own task, writing to the launching session's OvmsWriter via a raw pointer. The only stop path is the Ctrl-C Terminator callback.

If the SSH/Telnet session closes or drops by any means other than Ctrl-C, the console (OvmsWriter) is destroyed while the task keeps running; the next writer->write() dereferences the freed object → LoadProhibited.

Decoded backtrace (two separate occurrences):

LoadProhibited, EXCCAUSE 0x1c, EXCVADDR 0x10, A8=0, task "tail"
VfsTailCommand::Service()  main/ovms_vfs.cpp:756  -> writer->write(buf, len);
TaskBase::Task(void*)      main/task_base.cpp:161

EXCVADDR 0x10 / A8=0 = virtual call through a freed (zeroed) vtable.

Fix: OvmsWriter tracks its single active follow-mode task; TerminateCommandTask() sets OCS_StopRequested and waits for the task to exit and release the writer before the transport is freed. Called from ~ConsoleSSH/~ConsoleTelnet, with a backstop in ~OvmsWriter(). Cooperative stop+join (no vTaskDelete) so the task is never force-killed mid-write. Generic to any OvmsCommandTask.

Teardown briefly blocks the close handler until the task exits (≤ one ~250 ms poll); a concurrent Ctrl-C during teardown isn't possible on the disconnect path.

Tested on hw3.3-5 (Solterra): built, flashed, repeatedly dropped an SSH session mid-vfs tail → no crash; normal Ctrl-C exit and one-shot vfs tail -N unaffected.

[dexterbg]

The fix applies at the wrong level / from the wrong direction.

The potential necessity of a clean termination for running commands from a session disconnect applies not only to running command tasks, but to generally all interactive commands started within a shell. There is no requirement for these to use an OvmsCommandTask instance, the common property of interactive commands is to register an insert callback.

Most commands registering an insert callback do not need any cleanup though, as they don't allocate any resources (e.g. simple y/n listeners like module factory reset). Other interactive commands besides vfs tail needing a cleanup would for example be enable and vfs edit.

So what's needed here is either an extension of the insert callback handler scheme for passing a termination request (e.g. by introducing a specific session termination character code), or a dedicated registry for a command agnostic termination handler similar to the insert callback registry.

Side note: don't add fixes to changes.txt, except if they need users to change something on their side.

Regards,
Michael

[kezarjg]

Reworked along your second suggestion — a dedicated termination-handler registry on OvmsWriter, parallel to the insert-callback registry, rather than the OvmsCommandTask-specific path.

RegisterTerminationCallback(cb, ctx) / DeregisterTerminationCallback(cb) mirror the insert-callback registry; an interactive command registers a termination handler alongside its insert callback. Consoles call 'RunTerminationCallback() during teardown — before freeing the transport they own — so the handler can release resources / stop tasks while the writer is still valid (~ConsoleSSH/~ConsoleTelnetbefore freeing their transport,~OvmsConsolebefore freeing the shared queue so the Bluetooth console is covered too, and a backstop in~OvmsWriter`).

Handlers wired up:

• OvmsCommandTask — requests its follow-mode task to stop and joins it (cooperative, no vTaskDelete); generic to any OvmsCommandTask, not just vfs tail.
• vfs edit and enable — free their editor_state / PasswordContext, fixing the disconnect leaks you pointed out (and they deregister on normal completion to avoid a double-free).
• The y/n insert callbacks with no allocated state (factory reset, OTA prompts) register nothing, as you noted.

Also dropped the changes.txt entry.

Compiles clean; I'll test on hardware (hw3.3-5 Solterra) this weekend — repeating the mid-vfs tail SSH-disconnect case plus normal Ctrl-C / one-shot tail -N, and disconnects during vfs edit / enable — and report back.

[kezarjg]

I've pushed a revision (it's the current PR head). Short version: while validating the rework on-vehicle I found the previous approach deadlocks on SSH, so I reworked the teardown to fix it. Wanted to walk you through it plainly and get your read on a couple of design calls, since it's in the area you flagged.

Your review points are all addressed. The cleanup is now a generic termination-handler registry, not OvmsCommandTask-specific: any interactive command that takes over the session registers a handler. enable and vfs edit register one (they free their prompt/editor state); the simple y/n listeners like module factory reset allocate nothing and register nothing. I took the dedicated command-agnostic registry option of the two you suggested, and dropped the changes.txt entry.

The deadlock (new; surfaced during testing)
The follow-mode cleanup stopped the command task and then waited for it to exit before freeing the transport. On SSH that's creates a deadlock: ~ConsoleSSH runs on the Mongoose task, which holds the Mongoose lock for the whole mg_mgr_poll(), while the vfs tail task's write() → SendCallback needs that same lock. So the teardown waits forever for the task, and the task waits forever for the lock. The SSH server wedges and only a power cycle recovered it. Dropping a session mid-vfs tail reproduced it every time once the log was actively growing (a static/idle file didn't).

The fix; "signal and reap" instead of "stop and wait"
When an SSH connection closes with a follow-mode task still bound, I now:

• mark the console closing, so the task's in-flight write() bails out immediately without taking the lock or touching the (about-to-be-freed) connection;
• ask the task to stop, but don't wait for it; and
• defer freeing the console. A reap pass on the listener's poll cleans it up once the task has actually exited.

SendCallback is also re-keyed off the ConsoleSSH* rather than the mg_connection, so it can never dereference a connection Mongoose has already freed. I kept this SSH-local on purpose: Telnet/USB/Bluetooth keep the existing join, since their send paths don't take the Mongoose lock and don't deadlock.

Validated on-vehicle
(hw3.3‑5 Solterra): abrupt SSH disconnects during an active vfs tail now survive every time (it wedged 100% before); free heap is flat over 15 disconnect cycles (the reap frees the deferred consoles; no session-slot leak); and Ctrl‑C abort, one-shot vfs tail -N, and disconnects during enable / vfs edit / module factory reset are all unaffected.

Two calls I'd value your view on:

1. SSH-local vs. general. I scoped the defer-and-reap to the SSH console because it's the only transport that actually deadlocks; the termination-handler registry itself stays generic, as you suggested. I'm happy to generalize the deferral across transports if you'd rather it be uniform.
2. Reap placement. I run it from the listener's MG_EV_POLL. A NetManager ticker or a shared console reaper would work equally well — open to your preference.

(Longer term, routing SSH output through a bounded queue the way ConsoleAsync does would mean a follow task never touches the Mongoose lock at all. This change is compatible with that direction but doesn't require it.)