Fix TC crash: unsynchronized MyTCServer::clients + null derefs in DDOP parsing

CMakeLists.txt

@@ -131,6 +131,19 @@ target_link_libraries(
           nlohmann_json::nlohmann_json
           cmake_git_version_tracking)
 
+if(WIN32)
+  # For crash_handler.cpp's minidump writer (MiniDumpWriteDump).
+  target_link_libraries(${PROJECT_NAME} PRIVATE dbghelp)
+endif()
+
+if(MSVC)
+  # Keep Release optimizations but still emit a PDB, so a crash dump (see
+  # crash_handler.hpp) can actually be symbolicated afterwards — without this,
+  # .dmp files are close to useless.
+  target_compile_options(${PROJECT_NAME} PRIVATE /Zi)
+  target_link_options(${PROJECT_NAME} PRIVATE /DEBUG /OPT:REF /OPT:ICF)
+endif()
+
 install(TARGETS ${PROJECT_NAME} RUNTIME DESTINATION bin COMPONENT applications)
 
 option(AOG_TC_VALIDATE_IOP "Build and register the object pool validator test"

docs/CONCURRENCY.md

@@ -0,0 +1,79 @@
+# AOG-TaskController — Concurrency Model
+
+This document exists because the threading model here is easy to get wrong by analogy with the
+main loop, and a wrong assumption here previously produced a crash with **zero trace** — no
+exception, no log line, no console output. Read this before adding a new callback (a
+`TaskControllerServer` override, an event-dispatcher listener, a raw
+`add_global_parameter_group_number_callback`) or before touching state that such a callback reads
+or writes.
+
+## The two threads
+
+`isobus::CANHardwareInterface::start()` (called once, in `Application::setup_can_hardware()`)
+spawns its own background thread by default — this is standard AgIsoStack behavior, not something
+this repo opted into. That thread calls `CANNetworkManager::CANNetwork.update()` internally, which
+is what actually parses incoming CAN frames, handles address claims, and dispatches every PGN to
+whatever is registered to receive it.
+
+Meanwhile, `Application::update()` — the loop driven from `main.cpp`, doing UDP handling and
+periodic sends — runs on the **main thread**. It never calls `CANHardwareInterface::update()`
+itself; it relies entirely on that background thread.
+
+So: **two threads exist for the lifetime of the process**, and the question for any new code is
+which one it runs on.
+
+## Which callbacks run on which thread — checked, not assumed
+
+Don't guess this from a class's "manual update mode" comment (that pattern is unrelated to this
+question and led to a wrong conclusion once already). Check how the *specific* callback is
+registered:
+
+- **Runs on the CAN stack background thread by default** (CANHardwareInterface's updateThread): `TaskControllerServer`
+  (`MyTCServer`). Treat every `TaskControllerServer` override (`activate_object_pool`,
+  `on_value_command`, ...) as background-thread code unless you have traced the library
+  implementation and proven it is deferred to `TaskControllerServer::update()`.
+- **Runs directly on the background thread, synchronously, as the frame is processed** — not
+  deferred to any `update()` call: `VirtualTerminalClient` (confirmed: `process_rx_message` is
+  registered via `add_global_parameter_group_number_callback` and invokes
+  `softKeyEventDispatcher`/`buttonEventDispatcher`/`changeNumericValueEventDispatcher`/etc.
+  synchronously). Any raw
+  `add_global_parameter_group_number_callback`/`add_any_control_function_parameter_group_number_callback`
+  registered directly in `app.cpp` is in this category too, unless proven otherwise the same way:
+  read the registration call, not the surrounding comments.
+
+If you're not sure which category a new callback falls into, trace it the way this file's history
+did: find where it's registered with `CANNetworkManager`/`ControlFunction` and check whether that
+registration point itself defers to an `update()` call, or invokes the listener/dispatcher inline.
+
+## What's protected today
+
+- **`MyTCServer::clients` / `uploadedPools`** (`task_controller.hpp`/`.cpp`) — guarded by
+  `MyTCServer::clientsMutex` (a `std::recursive_mutex`, since several methods call each other:
+  e.g. `update_section_states()` → `send_section_setpoint_states()`). Locked at the top of every
+  `TaskControllerServer` override and every method `Application` calls into. `get_clients()`
+  returns a **copy**, not a reference — call sites hold the result across multiple operations or
+  take a second `get_clients()` call, either of which would leave a dangling reference to a
+  temporary if it returned by reference. If you add a new method that reads or writes `clients`,
+  take `clientsMutex` at its top, even though (per the note above) the `TaskControllerServer`
+  overrides may turn out to already be main-thread-only — the lock is cheap insurance and keeps
+  every entry point consistent regardless of how the library's internals might change.
+
+## What's *not* protected yet — known gap
+
+Calls into `vtClient` (`isobus::VirtualTerminalClient`) happen from both the main thread
+(`Application::update_vt_client()` and everything it calls: `send_vt_string_if_changed`,
+`vtUpdateHelper`, etc.) **and** from the VT event-dispatcher lambdas registered in
+`setup_vt_client()` (soft-key, button, numeric-value-change), which — per the confirmed
+registration pattern above — run on the background thread. Whether `VirtualTerminalClient`'s own
+internal state is safe under that concurrent access is not verified; no external mutex wraps
+`vtClient` usage today. If a crash dump ever points into `isobus::VirtualTerminalClient`, or a
+crash correlates with VT/AUX-N activity rather than DDOP upload, this is the first place to look.
+
+## Also worth knowing: `get_object_by_index()` can return null
+
+Unrelated to threading, but found while investigating a real crash report ("usually happens right
+after DDOP loading"): `isobus::DeviceDescriptorObjectPool::get_object_by_index()` can return
+`nullptr` for an index within `[0, size())` — most likely a symptom of a partially- or
+incorrectly-parsed pool (e.g. a multi-chunk DDOP transfer). Two call sites in this file already
+null-checked it; the rest didn't and were fixed alongside the mutex work above. **Any new call to
+`get_object_by_index()` must null-check the result before dereferencing.**

docs/PROTOCOL.md

@@ -2,7 +2,7 @@
 
 This document describes the interfaces AOG-TaskController exposes to the rest of the system. The intended audience is anyone writing a client — AgOpenGPS (AgIO), AgValoniaGPS, or a generic ISOBUS controller — that needs to talk to the TC.
 
-For Linux deployment of the TC itself (systemd, CAN setup, the daemon footprint), see [LINUX_DAEMON.md](LINUX_DAEMON.md).
+For Linux deployment of the TC itself (systemd, CAN setup, the daemon footprint), see [LINUX_DAEMON.md](LINUX_DAEMON.md). For the TC's threading model — which callbacks run on which thread, and what that means for any state you touch — see [CONCURRENCY.md](CONCURRENCY.md).
 
 ---
 

include/crash_handler.hpp

@@ -0,0 +1,24 @@
+/**
+ * @file crash_handler.hpp
+ * @brief Last-resort crash diagnostics: writes a crash log (and, on Windows, a
+ *        minidump) to the config "logs" directory before the process dies.
+ *
+ * This exists because AOG-TaskController normally runs with no visible console
+ * (launched by AgIO/AOG) and without --log2file, so a hard crash (access
+ * violation, uncaught exception, std::terminate) otherwise leaves zero trace.
+ * install_crash_handlers() should be called once, as early as possible in
+ * main/WinMain.
+ */
+#pragma once
+
+#include <string>
+
+/// @brief Install process-wide crash handlers (SEH filter + minidump on
+/// Windows, signal handlers on POSIX; std::terminate handler on both).
+void install_crash_handlers();
+
+/// @brief Append a line to the same crash log file the handlers above write to
+/// (config dir's logs/crash_<timestamp>.log), regardless of whether --log2file
+/// was passed. For fatal-but-caught conditions (e.g. an exception caught at the
+/// top of main()) that should still leave a trace on disk.
+void log_crash(const std::string &reason);

include/task_controller.hpp

@@ -16,6 +16,7 @@
 
 #include <cstdint>
 #include <map>
+#include <mutex>
 #include <queue>
 
 constexpr std::uint8_t NUMBER_SECTIONS_PER_CONDENSED_MESSAGE = 16;
@@ -103,7 +104,14 @@ class MyTCServer : public isobus::TaskControllerServer
 	                      std::int32_t processDataValue,
 	                      std::uint8_t &errorCodes) override;
 	bool store_device_descriptor_object_pool(std::shared_ptr<isobus::ControlFunction> partnerCF, const std::vector<std::uint8_t> &binaryPool, bool appendToPool) override;
-	std::map<std::shared_ptr<isobus::ControlFunction>, ClientState> &get_clients();
+
+	/// @brief Returns a snapshot copy of the client map, not a live reference.
+	/// See the concurrency note on clientsMutex below for why: the isobus stack
+	/// invokes the TaskControllerServer overrides above from its own background
+	/// thread, concurrently with whichever thread calls this. A returned reference
+	/// could be mutated (even reallocated, on insert/erase) out from under a caller
+	/// mid-iteration — a returned copy can't.
+	std::map<std::shared_ptr<isobus::ControlFunction>, ClientState> get_clients();
 	void request_measurement_commands();
 	void update_section_states(std::vector<bool> &sectionStates);
 	void update_section_control_enabled(bool enabled);
@@ -115,4 +123,23 @@ class MyTCServer : public isobus::TaskControllerServer
 
 	std::map<std::shared_ptr<isobus::ControlFunction>, ClientState> clients;
 	std::map<std::shared_ptr<isobus::ControlFunction>, std::queue<std::vector<std::uint8_t>>> uploadedPools;
+
+	/// @brief Guards clients and uploadedPools.
+	///
+	/// CONCURRENCY: the isobus/AgIsoStack stack runs its own background thread
+	/// (CANHardwareInterface's updateThread) that calls CANNetworkManager::update(),
+	/// which is what actually invokes every TaskControllerServer override in this
+	/// class (activate_object_pool, on_value_command, ...) — NOT the thread that
+	/// runs Application::update(). Meanwhile Application::update() (and the methods
+	/// it calls: request_measurement_commands, update_section_states/_control_enabled,
+	/// get_clients) reads/writes the SAME maps from the main thread. Without this
+	/// lock, that's a concurrent std::map read + insert/erase — undefined behavior,
+	/// seen in practice as the TC silently crashing (no exception, no log line)
+	/// whenever a new control function appeared on the bus. See docs/CONCURRENCY.md
+	/// before touching clients/uploadedPools or adding a new callback here: every
+	/// entry point the isobus stack can call into must take this lock
+	/// (std::recursive_mutex — some of these methods call each other, e.g.
+	/// update_section_states -> send_section_setpoint_states), and get_clients()
+	/// must keep returning a copy, not a reference (see its declaration above).
+	mutable std::recursive_mutex clientsMutex;
 };

src/app.cpp

@@ -1106,9 +1106,10 @@ void Application::update_vt_section_map()
 	if (isobus::SystemTiming::time_expired_ms(lastVtSectionUpdateMs, 100))
 	{
 		std::string sectionMap = "No sections connected";
-		if (!tcServer->get_clients().empty())
+		auto clients = tcServer->get_clients(); // snapshot copy — see get_clients()'s declaration
+		if (!clients.empty())
 		{
-			auto &state = tcServer->get_clients().begin()->second;
+			auto &state = clients.begin()->second;
 			const auto sectionCount = std::min<std::uint8_t>(state.get_number_of_sections(), 64);
 			if (sectionCount > 0)
 			{
@@ -1184,7 +1185,7 @@ void Application::update_vt_status_strings(bool aogConnected)
 	send_vt_string_if_changed(VTAogIPStr, udpConnections->get_bound_ip_address());
 	const std::string packetAge = (lastAogPacketMs == 0) ? "never" : (std::to_string(isobus::SystemTiming::get_time_elapsed_ms(lastAogPacketMs) / 1000) + " s");
 	const bool taskRunning = tcServer->get_task_totals_active();
-	auto &clients = tcServer->get_clients();
+	auto clients = tcServer->get_clients(); // snapshot copy — see get_clients()'s declaration
 
 	std::uint32_t totalSections = 0;
 	for (const auto &client : clients)

src/crash_handler.cpp

@@ -0,0 +1,189 @@
+#include "crash_handler.hpp"
+
+#include "logging_utils.hpp"
+#include "settings.hpp"
+
+#include <cstdio>
+#include <cstdlib>
+#include <ctime>
+#include <exception>
+#include <fstream>
+#include <iostream>
+#include <sstream>
+#include <string>
+
+namespace
+{
+	std::string crash_file_path(const std::string &extension)
+	{
+		std::time_t now = std::time(nullptr);
+		std::tm localTime;
+#if defined(_WIN32)
+		localtime_s(&localTime, &now);
+#else
+		localtime_r(&now, &localTime);
+#endif
+		char stamp[32];
+		std::snprintf(stamp, sizeof(stamp), "%04d-%02d-%02d_%02d-%02d-%02d", localTime.tm_year + 1900, localTime.tm_mon + 1, localTime.tm_mday, localTime.tm_hour, localTime.tm_min, localTime.tm_sec);
+		// Settings::get_filename_path() throws if it can't create the directory; a crash
+		// handler must never throw, so fall back to the current directory on failure.
+		try
+		{
+			return Settings::get_filename_path(std::string("logs/crash_") + stamp + extension);
+		}
+		catch (...)
+		{
+			return std::string("crash_") + stamp + extension;
+		}
+	}
+
+	void log_terminate_reason()
+	{
+		std::string reason = "std::terminate called";
+		if (auto currentException = std::current_exception())
+		{
+			try
+			{
+				std::rethrow_exception(currentException);
+			}
+			catch (const std::exception &e)
+			{
+				reason += std::string(" due to unhandled exception: ") + e.what();
+			}
+			catch (...)
+			{
+				reason += " due to an unhandled exception of unknown type";
+			}
+		}
+		else
+		{
+			reason += " with no active exception (direct std::terminate()/std::abort(), a noexcept "
+			          "violation, or a failure during stack unwinding)";
+		}
+		log_crash(reason);
+	}
+} // namespace
+
+void log_crash(const std::string &reason)
+{
+	std::ofstream out(crash_file_path(".log"), std::ios::app);
+	std::ostream &sink = out.is_open() ? static_cast<std::ostream &>(out) : std::cout;
+	sink << "[" << get_timestamp() << "] [Crash] " << reason << std::endl;
+}
+
+#if defined(_WIN32)
+
+#include <windows.h>
+
+#include <dbghelp.h>
+
+#pragma comment(lib, "dbghelp.lib")
+
+namespace
+{
+	LONG WINAPI unhandled_exception_filter(EXCEPTION_POINTERS *exceptionPointers)
+	{
+		const std::string dumpPath = crash_file_path(".dmp");
+
+		{
+			std::ostringstream reason;
+			reason << "Unhandled SEH exception 0x" << std::hex << exceptionPointers->ExceptionRecord->ExceptionCode
+			       << std::dec << " at address " << exceptionPointers->ExceptionRecord->ExceptionAddress
+			       << ". Writing minidump to " << dumpPath;
+			log_crash(reason.str());
+		}
+
+		HANDLE dumpFile = CreateFileA(dumpPath.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr);
+		if (dumpFile != INVALID_HANDLE_VALUE)
+		{
+			MINIDUMP_EXCEPTION_INFORMATION mdInfo{};
+			mdInfo.ThreadId = GetCurrentThreadId();
+			mdInfo.ExceptionPointers = exceptionPointers;
+			mdInfo.ClientPointers = FALSE;
+
+			MiniDumpWriteDump(GetCurrentProcess(), GetCurrentProcessId(), dumpFile, MiniDumpWithIndirectlyReferencedMemory, &mdInfo, nullptr, nullptr);
+			CloseHandle(dumpFile);
+		}
+
+		// Let Windows terminate the process normally after we've captured diagnostics.
+		return EXCEPTION_EXECUTE_HANDLER;
+	}
+} // namespace
+
+void install_crash_handlers()
+{
+	SetUnhandledExceptionFilter(unhandled_exception_filter);
+	std::set_terminate(log_terminate_reason);
+}
+
+#else // POSIX
+
+#include <unistd.h>
+#include <csignal>
+
+namespace
+{
+	void fatal_signal_handler(int signalNumber)
+	{
+		const char *name = "unknown signal";
+		switch (signalNumber)
+		{
+			case SIGSEGV:
+				name = "SIGSEGV (segmentation fault)";
+				break;
+			case SIGABRT:
+				name = "SIGABRT (abort)";
+				break;
+			case SIGFPE:
+				name = "SIGFPE (arithmetic error)";
+				break;
+			case SIGILL:
+				name = "SIGILL (illegal instruction)";
+				break;
+			case SIGBUS:
+				name = "SIGBUS (bus error)";
+				break;
+			default:
+				break;
+		}
+
+		// Signal-handler-safe I/O only: no iostreams, no dynamic allocation, no
+		// get_timestamp(), and no strlen() — it's not on POSIX's async-signal-safe
+		// function list, so a hand-rolled length count is used instead.
+		const int fd = 2; // stderr
+		auto writeRaw = [fd](const char *text) {
+			std::size_t len = 0;
+			while (text[len] != '\0')
+			{
+				++len;
+			}
+			(void)write(fd, text, len);
+		};
+		writeRaw("[Crash] Fatal signal: ");
+		writeRaw(name);
+		writeRaw("\n");
+
+		// Re-trigger the signal with default disposition so the process terminates normally
+		// (and may produce a core dump), without calling non-async-signal-safe functions.
+		::kill(::getpid(), signalNumber);
+		::_exit(128 + signalNumber);
+	}
+} // namespace
+
+void install_crash_handlers()
+{
+	struct sigaction sa
+	{
+	};
+	sa.sa_handler = fatal_signal_handler;
+	sigemptyset(&sa.sa_mask);
+	sa.sa_flags = SA_RESETHAND;
+	const int fatalSignals[] = { SIGSEGV, SIGABRT, SIGFPE, SIGILL, SIGBUS };
+	for (int sig : fatalSignals)
+	{
+		(void)sigaction(sig, &sa, nullptr);
+	}
+	std::set_terminate(log_terminate_reason);
+}
+
+#endif