From 437da814fa30cd2e32187c4d4f36877c23c8bac7 Mon Sep 17 00:00:00 2001 From: Jonathan Bennett Date: Wed, 8 Jul 2026 19:16:27 -0500 Subject: [PATCH] Make Safe File actually safe --- src/MessageStore.cpp | 16 ++--------- src/SafeFile.cpp | 16 ++++++++--- src/gps/GPS.cpp | 6 +---- .../niche/Utils/CannedMessageStore.cpp | 9 +------ src/graphics/niche/Utils/FlashData.h | 15 ++++------- src/mesh/NodeDB.cpp | 27 ++----------------- src/mesh/WarmNodeStore.cpp | 17 +++--------- src/mesh/mesh-pb-constants.cpp | 7 +++-- src/modules/CannedMessageModule.cpp | 7 +---- src/modules/HopScalingModule.cpp | 1 - src/motion/MotionSensor.cpp | 2 +- 11 files changed, 33 insertions(+), 90 deletions(-) diff --git a/src/MessageStore.cpp b/src/MessageStore.cpp index 6332c0e827b..108ef6a41de 100644 --- a/src/MessageStore.cpp +++ b/src/MessageStore.cpp @@ -291,23 +291,16 @@ static inline bool readMessageRecord(File &f, StoredMessage &m) void MessageStore::saveToFlash() { #ifdef FSCom - // Ensure root exists - spiLock->lock(); - FSCom.mkdir("/"); - spiLock->unlock(); - SafeFile f(filename.c_str(), false); - spiLock->lock(); uint8_t count = static_cast(liveMessages.size()); if (count > MAX_MESSAGES_SAVED) count = MAX_MESSAGES_SAVED; - f.write(&count, 1); + f.write(&count, 1); // SafeFile::write takes spiLock internally for (uint8_t i = 0; i < count; ++i) { writeMessageRecord(f, liveMessages[i]); } - spiLock->unlock(); f.close(); #endif @@ -367,12 +360,7 @@ void MessageStore::clearAllMessages() SafeFile f(filename.c_str(), false); uint8_t count = 0; - // SafeFile already does its own spiLock in its constructor and close(). - // Avoid nesting spiLocks, as this will hang until watchdog reset! - { - concurrency::LockGuard guard(spiLock); - f.write(&count, 1); // write "0 messages" - } + f.write(&count, 1); // write "0 messages"; SafeFile::write takes spiLock internally f.close(); #endif diff --git a/src/SafeFile.cpp b/src/SafeFile.cpp index 0173fde816a..bcfd2569a02 100644 --- a/src/SafeFile.cpp +++ b/src/SafeFile.cpp @@ -5,15 +5,23 @@ // Only way to work on both esp32 and nrf52 static File openFile(const char *filename, bool fullAtomic) { + std::string filenameTmp = filename; + filenameTmp += ".tmp"; + concurrency::LockGuard g(spiLock); + + // Create the containing directory if it doesn't already exist + size_t slashIndex = filenameTmp.find_last_of('/'); + if (slashIndex != std::string::npos && slashIndex > 0) { + std::string dir = filenameTmp.substr(0, slashIndex); + FSCom.mkdir(dir.c_str()); + } + LOG_DEBUG("Opening %s, fullAtomic=%d", filename, fullAtomic); if (!fullAtomic) { FSCom.remove(filename); // Nuke the old file to make space (ignore if it !exists) } - String filenameTmp = filename; - filenameTmp += ".tmp"; - // FIXME: If we are doing a full atomic write, we may need to remove the old tmp file now // if (fullAtomic) { // FSCom.remove(filename); @@ -33,6 +41,7 @@ size_t SafeFile::write(uint8_t ch) if (!f) return 0; + concurrency::LockGuard g(spiLock); hash ^= ch; return f.write(ch); } @@ -42,6 +51,7 @@ size_t SafeFile::write(const uint8_t *buffer, size_t size) if (!f) return 0; + concurrency::LockGuard g(spiLock); for (size_t i = 0; i < size; i++) { hash ^= buffer[i]; } diff --git a/src/gps/GPS.cpp b/src/gps/GPS.cpp index 430030422b2..a64645e77bf 100644 --- a/src/gps/GPS.cpp +++ b/src/gps/GPS.cpp @@ -666,17 +666,13 @@ bool GPS::saveProbeCache() const return false; } - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); GPSProbeCacheRecord record = { GPS_PROBE_CACHE_MAGIC, GPS_PROBE_CACHE_VERSION, 0, static_cast(detectedBaud), static_cast(gnssModel), }; auto file = SafeFile(GPS_PROBE_CACHE_FILE, true); - spiLock->lock(); + // SafeFile::write takes spiLock internally; don't hold it here (non-recursive lock). const size_t written = file.write(reinterpret_cast(&record), sizeof(record)); - spiLock->unlock(); return (written == sizeof(record)) && file.close(); #else return false; diff --git a/src/graphics/niche/Utils/CannedMessageStore.cpp b/src/graphics/niche/Utils/CannedMessageStore.cpp index 182b7e1f8ff..391b949d003 100644 --- a/src/graphics/niche/Utils/CannedMessageStore.cpp +++ b/src/graphics/niche/Utils/CannedMessageStore.cpp @@ -121,14 +121,7 @@ void CannedMessageStore::handleSet(const meshtastic_AdminMessage *request) strncpy(cannedMessageModuleConfig.messages, request->set_canned_message_module_messages, sizeof(cannedMessageModuleConfig.messages)); - // Ensure the directory exists -#ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); -#endif - - // Write to flash + // Write to flash ("/prefs" is created by SafeFile (openFile) inside saveProto) nodeDB->saveProto(cannedMessagesConfigFile, meshtastic_CannedMessageModuleConfig_size, &meshtastic_CannedMessageModuleConfig_msg, &cannedMessageModuleConfig); diff --git a/src/graphics/niche/Utils/FlashData.h b/src/graphics/niche/Utils/FlashData.h index 233d0922eb4..f3c80d2f6a2 100644 --- a/src/graphics/niche/Utils/FlashData.h +++ b/src/graphics/niche/Utils/FlashData.h @@ -107,19 +107,16 @@ template class FlashData return okay; } - // Save module's custom data (settings?) to flash. Doesn't use protobufs - // Takes the firmware's SPI lock, in case the files are stored on SD card - // Need to lock and unlock around specific FS methods, as the SafeFile class takes the lock for itself internally. + // Save module's custom data (settings?) to flash. Doesn't use protobufs. + // SafeFile (constructor/write/close) takes the firmware's SPI lock for itself internally + // (relevant when files are on an SD card), so we must not hold it across those calls. static void save(T *data, const char *label) { // Get a filename based on the label std::string filename = getFilename(label); #ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/NicheGraphics"); - spiLock->unlock(); - + // "/NicheGraphics" is created by SafeFile (openFile) below. auto f = SafeFile(filename.c_str(), true); // "true": full atomic. Write new data to temp file, then rename. LOG_INFO("Saving %s", filename.c_str()); @@ -127,10 +124,8 @@ template class FlashData // Calculate a hash of the data uint32_t hash = getHash(data); - spiLock->lock(); - f.write((uint8_t *)data, sizeof(T)); // Write the actual data + f.write((uint8_t *)data, sizeof(T)); // Write the actual data (SafeFile::write takes spiLock internally) f.write((uint8_t *)&hash, sizeof(hash)); // Append the hash - spiLock->unlock(); bool writeSucceeded = f.close(); diff --git a/src/mesh/NodeDB.cpp b/src/mesh/NodeDB.cpp index 76e7384dc34..c5e6eace400 100644 --- a/src/mesh/NodeDB.cpp +++ b/src/mesh/NodeDB.cpp @@ -2787,12 +2787,7 @@ bool NodeDB::saveChannelsToDisk() return false; } -#ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); -#endif - + // Directory is created by SafeFile (openFile) inside saveProto. return saveProto(channelFileName, meshtastic_ChannelFile_size, &meshtastic_ChannelFile_msg, &channelFile); } @@ -2806,11 +2801,6 @@ bool NodeDB::saveDeviceStateToDisk() return false; } -#ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); -#endif // Note: if MAX_NUM_NODES=100 and meshtastic_NodeInfoLite_size=166, so will be approximately 17KB // Because so huge we _must_ not use fullAtomic, because the filesystem is probably too small to hold two copies of this return saveProto(deviceStateFileName, meshtastic_DeviceState_size, &meshtastic_DeviceState_msg, &devicestate, true); @@ -2843,12 +2833,6 @@ bool NodeDB::saveNodeDatabaseToDisk() } #endif -#ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); -#endif - // Project the maps into the on-disk vectors just before encoding; cleared // again on the way out so we don't carry duplicate state. concurrency::LockGuard guard(&satelliteMutex); @@ -2960,11 +2944,6 @@ bool NodeDB::saveToDiskNoRetry(int saveWhat) #endif bool success = true; -#ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); -#endif if (saveWhat & SEGMENT_CONFIG) { config.has_device = true; @@ -3980,9 +3959,7 @@ bool NodeDB::backupPreferences(meshtastic_AdminMessage_BackupLocation location) size_t backupSize; pb_get_encoded_size(&backupSize, meshtastic_BackupPreferences_fields, &backup); - spiLock->lock(); - FSCom.mkdir("/backups"); - spiLock->unlock(); + // "/backups" is created by SafeFile (openFile) inside saveProto. success = saveProto(backupFileName, backupSize, &meshtastic_BackupPreferences_msg, &backup); if (success) { diff --git a/src/mesh/WarmNodeStore.cpp b/src/mesh/WarmNodeStore.cpp index 550549ad5c4..682d91b888b 100644 --- a/src/mesh/WarmNodeStore.cpp +++ b/src/mesh/WarmNodeStore.cpp @@ -592,21 +592,12 @@ bool WarmNodeStore::save() h.entrySize = sizeof(WarmNodeEntry); h.crc = crc32Buffer(packed.data(), h.count * sizeof(WarmNodeEntry)); - // SafeFile already does its own spiLock in its constructor and close(). - // Avoid nesting spiLocks, as this will hang until watchdog reset! - - { - concurrency::LockGuard g(spiLock); - FSCom.mkdir("/prefs"); - } - + // SafeFile takes spiLock itself in its constructor, write() and close(); + // we must not hold it across those or the non-recursive lock deadlocks (watchdog reset). auto f = SafeFile(warmFileName, false); - { - concurrency::LockGuard g(spiLock); - f.write((const uint8_t *)&h, sizeof(h)); - f.write((const uint8_t *)packed.data(), h.count * sizeof(WarmNodeEntry)); - } + f.write((const uint8_t *)&h, sizeof(h)); + f.write((const uint8_t *)packed.data(), h.count * sizeof(WarmNodeEntry)); bool ok = f.close(); if (!ok) diff --git a/src/mesh/mesh-pb-constants.cpp b/src/mesh/mesh-pb-constants.cpp index a8f4fd6d801..a754cfefdc9 100644 --- a/src/mesh/mesh-pb-constants.cpp +++ b/src/mesh/mesh-pb-constants.cpp @@ -56,12 +56,11 @@ bool readcb(pb_istream_t *stream, uint8_t *buf, size_t count) /// Write to an arduino file bool writecb(pb_ostream_t *stream, const uint8_t *buf, size_t count) { - spiLock->lock(); + // Locking is delegated to the Print's write() (SafeFile::write takes spiLock itself). + // All current callers pass a SafeFile; a non-SafeFile Print would not be bus-locked here. auto file = (Print *)stream->state; // LOG_DEBUG("writing %d bytes to protobuf file", count); - bool status = file->write(buf, count) == count; - spiLock->unlock(); - return status; + return file->write(buf, count) == count; } #endif diff --git a/src/modules/CannedMessageModule.cpp b/src/modules/CannedMessageModule.cpp index b2278133dee..f9faaa6d15c 100644 --- a/src/modules/CannedMessageModule.cpp +++ b/src/modules/CannedMessageModule.cpp @@ -2292,12 +2292,7 @@ bool CannedMessageModule::saveProtoForModule() { bool okay = true; -#ifdef FSCom - spiLock->lock(); - FSCom.mkdir("/prefs"); - spiLock->unlock(); -#endif - + // "/prefs" is created by SafeFile (openFile) inside saveProto. okay &= nodeDB->saveProto(cannedMessagesConfigFile, meshtastic_CannedMessageModuleConfig_size, &meshtastic_CannedMessageModuleConfig_msg, &cannedMessageModuleConfig); diff --git a/src/modules/HopScalingModule.cpp b/src/modules/HopScalingModule.cpp index 1045e4f0217..7d4ef832794 100644 --- a/src/modules/HopScalingModule.cpp +++ b/src/modules/HopScalingModule.cpp @@ -80,7 +80,6 @@ void HopScalingModule::clear() void HopScalingModule::saveToDisk() const { #ifdef FSCom - FSCom.mkdir("/prefs"); PersistedHistogram state{}; state.magic = HISTOGRAM_STATE_MAGIC; state.version = HISTOGRAM_STATE_VERSION; diff --git a/src/motion/MotionSensor.cpp b/src/motion/MotionSensor.cpp index 878b4dd5088..ea23284fc8d 100755 --- a/src/motion/MotionSensor.cpp +++ b/src/motion/MotionSensor.cpp @@ -79,7 +79,7 @@ bool MotionSensor::saveMagnetometerCalibration(const char *filePath, float highe return false; } - FSCom.mkdir("/prefs"); + // "/prefs" is created by SafeFile (openFile) below. CompassCalibrationRecord record = { COMPASS_CALIBRATION_MAGIC, COMPASS_CALIBRATION_VERSION, 0, highestX, lowestX, highestY, lowestY, highestZ, lowestZ};