2 Commits

Include renames

Author	SHA1	Message	Date
Jonathan BennettandClaude Opus 5	e691bd3790	Claude/dmshell lock (#12024) ... * Lock: give Portduino a real mutex instead of the empty fallback Lock.cpp has a FreeRTOS implementation and an empty one, and Portduino takes the empty one: every lock() and unlock() on a Linux build is a no-op, so concurrency::Lock protects nothing there. TrafficManagementModule's cacheLock and SPILock are both built on it, and native meshtasticd runs the radio and the API on separate threads. Add a pthread implementation under ARCH_PORTDUINO. The timed lock(uint32_t) blocks rather than returning early, because there is no portable timed pthread_mutex_lock across Linux and macOS and returning true without acquiring would leave a caller such as SPILock unlocking a mutex it never took. Targets that have neither FreeRTOS nor pthreads, such as STM32WL, keep the existing empty implementation byte for byte. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 * test: create spiLock in the shared setup, which NodeDB needs and no test had Making Portduino's Lock real turns a latent null dereference into a crash. spiLock is a bare pointer that initSPI() fills in, and only main.cpp calls that, so in a test binary it stays null. NodeDB's constructor reaches it through loadFromDisk(), and while Lock::lock() was an empty function the call never touched `this`, so 23 suites have been calling a method on a null pointer and getting away with it. With a pthread mutex behind it the same call reads through the null pointer and takes SIGSEGV at offset 0x10, which is what test_phone_api_config_dump, test_muted_source, test_nodeinfo_send_window and test_module_config hit. Create it once in initializeTestEnvironment(), which every affected suite calls as the first statement of setup(), before any of them constructs a NodeDB. The guard is the idiom test_xmodem and test_nodedb_identity_hygiene already use; theirs stay correct and become no-ops. test_safefile called initSPI() bare right after the harness, which would now trip its assert, so that call goes away. No firmware behaviour changes: main.cpp still calls initSPI() exactly once, and nothing outside the test harness is touched. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 * test: create cryptLock where a suite reaches it without building a Router Second instance of the same latent null dereference the previous commit fixed for spiLock. cryptLock is a bare pointer that Router's constructor creates (Router.cpp:246); AdminModule::setPassKey takes a LockGuard on it, and a suite that exercises an admin path without standing up a Router leaves it null. While Lock::lock() was empty on Portduino the guard never touched `this`; with a pthread mutex it reads through null, which is test_tak_config's SIGSEGV in handleGetModuleConfig. It cannot go in initializeTestEnvironment() the way spiLock did, because Router asserts cryptLock is unset before allocating its own, so creating it for every suite would break the ones that do build a Router. It is a named helper instead, testEnsureCryptLock(), called by the six suites that reach a cryptLock path with no Router: test_ack_proof, test_admin_session_repro, test_fuzz_packets, test_hop_scaling, test_module_config and test_tak_config. The three that define setup() twice behind a PKI #if get the call only in the branch that compiles the tests in. No firmware behaviour changes; nothing outside test/ is touched. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 * test: create cryptLock in the harness instead of chasing suites Router's constructor asserted cryptLock was unset, then allocated it. That assert is why ten suites carry a mock-router destructor whose only job is to delete the global and null it so the next router can be built. While Lock::lock() was an empty function on Portduino a null cryptLock cost nothing, so those null windows were invisible; with a real mutex, anything reaching perhapsDecode() or the ack-proof paths after one of those destructors runs dereferences null. The fix is the idiom already on the next line of the same constructor, which routingAuthCacheLock has used all along: reuse the lock if one exists. Nothing in src/ ever deleted cryptLock, so a Router that finds one is finding the process's only one. initializeTestEnvironment() can then create it for every suite, the way it now does for spiLock, and the ten teardowns and the per-suite helper from the previous commit all go away. Replaces the six testEnsureCryptLock() call sites with one creation point, and removes the null windows in test_admin_radio, test_mesh_beacon, test_mesh_module, test_mqtt, test_nexthop_routing, test_nodeinfo_send_window, test_traffic_management, test_event_channel_phone_api, test_event_channel_router and test_phone_api_config_dump. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 --------- Co-authored-by: Claude <noreply@anthropic.com>	2026-10-01 21:33:14 +00:00
Clive BlackledgeandClaude Opus 5	0271be9369	fix(SafeFile): remove a stale .tmp before opening it for write (#11428) ... * fix(SafeFile): remove a stale .tmp before opening it for write SafeFile writes to <filename>.tmp, verifies it by readback, then renames it over the real file. openFile() never removed a pre-existing .tmp - an unfinished FIXME - and FILE_O_WRITE appends rather than truncates on Adafruit_LittleFS (nRF52) and STM32 LittleFS. So a .tmp left behind by a reset in the window between close() and renameFile() is appended to on the next save. The readback hash covers only the bytes just written, so it mismatches, close() returns false, and the tmp is left behind again - the failure latches and every subsequent save of that file fails. Today saveProto() discards close()'s result, so this is silent and permanent. Guard the remove with exists(): a bare remove() of a missing file logs on Portduino. The same guarded pattern is already used for this exact append trap in xmodem.cpp. Note the FIXME's commented-out body named the wrong path - it removed 'filename', the real file, not 'filenameTmp' - so it would have destroyed the good copy had it ever been enabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(SafeFile): cover the stale-.tmp path, and trim the fix comment Adds test_safefile, the first coverage of SafeFile's write-tmp / verify-by-readback / rename-over path that every saveProto() caller goes through. Five cases pin the contract the fix restores: whatever the backend does on open, a completed save leaves the real file holding exactly the bytes written and nothing else, on both the fullAtomic and the !fullAtomic construction, with no .tmp left behind. These tests cannot go red on the native host, and that gap cannot be closed here. FILE_O_WRITE is an append-then-seek-to-end open only on Adafruit_LittleFS (nRF52) and on the in-repo STM32 port (STM32_LittleFS_File.cpp: LFS_O_RDWR | LFS_O_CREAT followed by lfs_file_seek to LFS_SEEK_END). On Portduino FILE_O_WRITE is the string "w" (FSCommon.h:13), which reaches fopen() and truncates. Reverting the source fix and re-running leaves all five green, verified rather than assumed. test_write_open_truncates _on_this_host asserts that premise out loud, so if the host ever gains the append behaviour the suite starts discriminating instead of quietly agreeing. Why the original FIXME stayed commented out, since that is the real history here. It read "if (fullAtomic) FSCom.remove(filename)" and named the real file, not the tmp. Running it would delete the last good copy before the replacement had been written and verified, which is precisely the guarantee fullAtomic exists to provide. Disabling it was correct. The fix under test removes filenameTmp instead, which is the file that actually carries the stale bytes, and is safe to drop at any point because nothing has been promised about it yet. Scoping the remove to fullAtomic would be wrong for the same reason. Both paths open the same filenameTmp with the same FILE_O_WRITE; fullAtomic only decides whether the real file is nuked up front to free space. The !fullAtomic path is the space-constrained one, so it is if anything the more likely to be interrupted mid-write and inherit a stale tmp. Test 2 pins that. On the cost of the added exists(). Every saveProto() already ends in SafeFile::close(), which calls testReadback(): it reopens the tmp and reads the whole proto back one byte at a time through f2.read() to XOR a verification hash, then renames. So the per-save cost is already an open, a full write, a close, a full byte-wise reread, and a rename. One exists() is a single path lookup with no erase, no program and no data read, and on the common path there is no remove() at all. Next to the readback loop it is noise. Happy to put a number on it if wanted. Scoping it to fullAtomic would also not do what it looks like it does. SafeFile's constructor defaults fullAtomic to false (SafeFile.h:28), and of the saveProto call sites only saveDeviceStateToDisk passes true. Config, moduleconfig, channels, nodedatabase and backup all take the default, so scoping would leave the stale tmp live on almost every save path, including the space-constrained one most likely to be interrupted mid-write. Also trims the fix's comment to two lines per AGENTS.md, and drops the stale-tmp removal log from LOG_WARN to LOG_DEBUG: an interrupted write is recoverable and self-healing, so it does not warrant a warning on every boot after one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(SafeFile): fail the write when a stale tmp cannot be removed openFile() ignored the result of FSCom.remove(). If the removal failed on an append-on-write backend, the open that follows appended to the stale bytes, and the readback hash is an 8 bit XOR over the whole tmp, so polluted content has a real chance of verifying and being renamed over the good file. It now logs and returns an invalid File. SafeFile::write() already no-ops on !f and close() already returns false, so the caller sees the save fail rather than silently getting a corrupt one. This is the only checked FSCom.remove() in the tree; the other call sites are all best-effort cleanups where failure does not compromise anything. Also gates test_write_open_truncates_on_this_host to ARCH_PORTDUINO. It asserts that this host truncates on FILE_O_WRITE, which is false by design on the Adafruit_LittleFS and STM32 backends the fix exists for, so running the suite there would fail on a premise that is only meant to describe the test host. Both reported by CodeRabbit on #11428. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-24 08:23:23 +00:00