diff --git a/src/helpers/ota/CompanionOtaConfig.cpp b/src/helpers/ota/CompanionOtaConfig.cpp index 8d2107e53..c941f31ea 100644 --- a/src/helpers/ota/CompanionOtaConfig.cpp +++ b/src/helpers/ota/CompanionOtaConfig.cpp @@ -11,6 +11,7 @@ namespace mesh { namespace ota { namespace { FILESYSTEM* settings_fs = nullptr; bool held = false; +bool recoverable_corrupt_store = false; constexpr size_t ImageSize = 16 + MAX_OTA_SIGNERS * 32; constexpr size_t CrcOffset = ImageSize - 4; const char* const Path = "/ota_config"; @@ -69,45 +70,74 @@ bool decode(const uint8_t* image, OtaConfigState& state) { return true; } -bool readImage(const char* path, uint8_t* image, OtaConfigState& state) { +enum class ImageReadResult : uint8_t { Missing, Valid, Invalid, Unreadable }; + +ImageReadResult readImage(const char* path, uint8_t* image, OtaConfigState& state) { + bool present = false; + if (!settings_fs || !pathPresence(path, present)) return ImageReadResult::Unreadable; + if (!present) return ImageReadResult::Missing; #if defined(NRF52_PLATFORM) File file(*settings_fs); - if (!file.open(path, FILE_O_READ)) return false; + if (!file.open(path, FILE_O_READ)) return ImageReadResult::Unreadable; #elif defined(STM32_PLATFORM) File file = settings_fs->open(path, FILE_O_READ); #else File file = settings_fs->open(path, "r"); #endif - if (!file) return false; - const bool complete = file.size() == ImageSize - && file.read(image, ImageSize) == (int)ImageSize; + if (!file) return ImageReadResult::Unreadable; + const bool right_size = file.size() == ImageSize; + const bool complete = right_size && file.read(image, ImageSize) == (int)ImageSize; file.close(); - return complete && decode(image, state); + if (!right_size) return ImageReadResult::Invalid; + if (!complete) return ImageReadResult::Unreadable; + return decode(image, state) ? ImageReadResult::Valid : ImageReadResult::Invalid; +} + +bool discardCorruptSavedImagesForWrite() { + if (!held) return true; + if (!settings_fs || !recoverable_corrupt_store) return false; + // This is a user-triggered recovery for the separate OTA policy/allowlist; + // it never touches node identity, private keys, contacts, or common prefs. + if (!removeIfPresent(Path) || !removeIfPresent(Backup)) return false; + held = false; + recoverable_corrupt_store = false; + return true; } } void beginCompanionOtaConfig(FILESYSTEM* fs) { settings_fs = fs; held = false; + recoverable_corrupt_store = false; } bool loadCompanionOtaConfig(OtaConfigState& state) { if (!settings_fs) return false; uint8_t image[ImageSize]; - bool primary = false, backup = false; - if (!pathPresence(Path, primary)) { held = true; return false; } - if (primary && readImage(Path, image, state)) return true; - if (primary) held = true; - if (!pathPresence(Backup, backup)) { held = true; return false; } - if (backup && readImage(Backup, image, state)) { + const ImageReadResult primary = readImage(Path, image, state); + bool loaded = primary == ImageReadResult::Valid; + ImageReadResult backup = ImageReadResult::Missing; + if (!loaded) backup = readImage(Backup, image, state); + if (!loaded && backup == ImageReadResult::Valid) { // Keep the verified previous settings even if recovery cannot rename them. // An unreadable primary may only have suffered a transient read failure; // do not remove it to promote the fallback. - if (primary || !settings_fs->rename(Backup, Path)) held = true; + if (primary == ImageReadResult::Unreadable + || (primary != ImageReadResult::Missing && !removeIfPresent(Path)) + || !settings_fs->rename(Backup, Path)) held = true; return true; } - if (primary || backup) { - held = true; // unreadable is not permission to overwrite with defaults + if (loaded) return true; + if (!loaded) { + held = primary != ImageReadResult::Missing || backup != ImageReadResult::Missing; + // A later explicit setting can recreate only files that were fully read + // and decoded as invalid. I/O failures and a valid backup stay protected. + recoverable_corrupt_store = held + && primary != ImageReadResult::Unreadable + && backup != ImageReadResult::Unreadable + && (primary == ImageReadResult::Invalid || backup == ImageReadResult::Invalid); + } + if (held) { return false; } state = OtaConfigState(); @@ -115,15 +145,21 @@ bool loadCompanionOtaConfig(OtaConfigState& state) { } bool saveCompanionOtaConfig(const OtaConfigState& state) { - if (!settings_fs || held || !validState(state)) return false; + if (!settings_fs || !validState(state) || !discardCorruptSavedImagesForWrite()) return false; uint8_t image[ImageSize] = {'O', 'C', 1, 0}, verify[ImageSize]; OtaConfigState prior; - bool had_primary = false; - if (!pathPresence(Path, had_primary)) { held = true; return false; } - if (had_primary && !readImage(Path, verify, prior)) { + const ImageReadResult primary = readImage(Path, verify, prior); + if (primary == ImageReadResult::Unreadable) { held = true; return false; } + if (primary == ImageReadResult::Invalid) { + // The store changed after boot. Do not erase it on an implicit retry. + held = true; + recoverable_corrupt_store = true; + return false; + } + const bool had_primary = primary == ImageReadResult::Valid; image[4] = state.autofetch; image[5] = state.autoinstall; image[6] = state.hops; image[7] = state.allow.count(); storage::writeLE16(image + 8, state.checkpoint); @@ -144,7 +180,7 @@ bool saveCompanionOtaConfig(const OtaConfigState& state) { if (!file) return false; const bool wrote = file.write(image, sizeof(image)) == sizeof(image); file.flush(); file.close(); - if (!wrote || !readImage(Temp, verify, prior) + if (!wrote || readImage(Temp, verify, prior) != ImageReadResult::Valid || memcmp(image, verify, sizeof(image))) return false; if (!removeIfPresent(Backup)) return false; if (had_primary && !settings_fs->rename(Path, Backup)) return false; diff --git a/src/helpers/ota/OtaSpeedConfig.cpp b/src/helpers/ota/OtaSpeedConfig.cpp index 0e8466b86..159729d6c 100644 --- a/src/helpers/ota/OtaSpeedConfig.cpp +++ b/src/helpers/ota/OtaSpeedConfig.cpp @@ -4,38 +4,91 @@ #include #include #include +#if defined(ESP32_PLATFORM) +#include +#include +#endif namespace mesh { namespace ota { namespace { FILESYSTEM* settings_fs = nullptr; float factor = OTA_SPEED_DEFAULT; bool held = false; +bool recoverable_corrupt_store = false; constexpr size_t ImageSize = 12; const char* const Path = "/ota_speed"; const char* const Temp = "/ota_speed.tmp"; const char* const Backup = "/ota_speed.bak"; -bool readImage(const char* path, uint8_t* image) { +enum class ImageReadResult : uint8_t { Missing, Valid, Invalid, Unreadable }; + +// A failed open is not enough to prove a path is absent: some filesystems +// report an I/O error as a failed exists() call. Keep those images protected. +bool pathPresence(const char* path, bool& present) { +#if defined(NRF52_PLATFORM) || defined(STM32_PLATFORM) + struct lfs_info info; + settings_fs->_lockFS(); + const int result = lfs_stat(settings_fs->_getFS(), path, &info); + settings_fs->_unlockFS(); + if (result != 0 && result != LFS_ERR_NOENT) return false; + present = result == 0; +#elif defined(ESP32_PLATFORM) + char vfs_path[48]; + const int length = snprintf(vfs_path, sizeof(vfs_path), "/spiffs%s", path); + if (length < 0 || static_cast(length) >= sizeof(vfs_path)) return false; + struct stat info; + const int result = ::stat(vfs_path, &info); + if (result != 0 && errno != ENOENT) return false; + present = result == 0; +#else + present = settings_fs->exists(path); +#endif + return true; +} + +bool removeIfPresent(const char* path) { + bool present = false; + return pathPresence(path, present) && (!present || settings_fs->remove(path)); +} + +ImageReadResult readImage(const char* path, uint8_t* image) { + bool present = false; + if (!settings_fs || !pathPresence(path, present)) return ImageReadResult::Unreadable; + if (!present) return ImageReadResult::Missing; #if defined(NRF52_PLATFORM) File file(*settings_fs); - if (!file.open(path, FILE_O_READ)) return false; + if (!file.open(path, FILE_O_READ)) return ImageReadResult::Unreadable; #elif defined(STM32_PLATFORM) File file = settings_fs->open(path, FILE_O_READ); #else File file = settings_fs->open(path, "r"); #endif - if (!file) return false; - const bool complete = file.size() == ImageSize && file.read(image, ImageSize) == (int)ImageSize; + if (!file) return ImageReadResult::Unreadable; + const bool right_size = file.size() == ImageSize; + const bool complete = right_size && file.read(image, ImageSize) == (int)ImageSize; file.close(); - if (!complete) return false; + if (!right_size) return ImageReadResult::Invalid; + if (!complete) return ImageReadResult::Unreadable; float value; memcpy(&value, image + 4, sizeof(value)); return !memcmp(image, "OS\1\0", 4) && validSpeed(value) - && storage::readLE32(image + 8) == storage::updateCRC32(0xffffffffU, image, 8); + && storage::readLE32(image + 8) == storage::updateCRC32(0xffffffffU, image, 8) + ? ImageReadResult::Valid : ImageReadResult::Invalid; +} + +bool discardCorruptSavedImagesForWrite() { + if (!held) return true; + if (!settings_fs || !recoverable_corrupt_store) return false; + // This is only reached from an explicit setter after both images were read + // completely and rejected. OTA policy is independent of identity/key data. + if (!removeIfPresent(Path) || !removeIfPresent(Backup)) return false; + held = false; + recoverable_corrupt_store = false; + return true; } bool saveSpeed(float value) { - if (!settings_fs || held) return false; + if (!settings_fs || !validSpeed(value) || !discardCorruptSavedImagesForWrite()) return false; uint8_t image[ImageSize] = {'O', 'S', 1, 0}; memcpy(image + 4, &value, sizeof(value)); storage::writeLE32(image + 8, storage::updateCRC32(0xffffffffU, image, 8)); @@ -52,9 +105,11 @@ bool saveSpeed(float value) { bool ok = file.write(image, sizeof(image)) == sizeof(image); file.flush(); file.close(); uint8_t verify[ImageSize]; - if (!ok || !readImage(Temp, verify) || memcmp(image, verify, sizeof(image))) return false; - if (settings_fs->exists(Backup) && !settings_fs->remove(Backup)) return false; - const bool had_primary = settings_fs->exists(Path); + if (!ok || readImage(Temp, verify) != ImageReadResult::Valid + || memcmp(image, verify, sizeof(image))) return false; + if (!removeIfPresent(Backup)) return false; + bool had_primary = false; + if (!pathPresence(Path, had_primary)) { held = true; return false; } if (had_primary && !settings_fs->rename(Path, Backup)) return false; if (!settings_fs->rename(Temp, Path)) { if (had_primary && !settings_fs->rename(Backup, Path)) held = true; @@ -68,19 +123,35 @@ bool saveSpeed(float value) { void beginSpeedConfig(FILESYSTEM* fs) { settings_fs = fs; factor = OTA_SPEED_DEFAULT; held = false; + recoverable_corrupt_store = false; if (!fs) return; uint8_t image[ImageSize]; - bool loaded = readImage(Path, image); - if (!loaded && readImage(Backup, image)) { + const ImageReadResult primary = readImage(Path, image); + bool loaded = primary == ImageReadResult::Valid; + ImageReadResult backup = ImageReadResult::Missing; + if (!loaded) backup = readImage(Backup, image); + if (!loaded && backup == ImageReadResult::Valid) { // A verified backup is still authoritative when storage cannot repair the // rename gap. Keep its pace instead of silently reverting to the faster 1x. memcpy(&factor, image + 4, sizeof(factor)); - if (fs->exists(Path) && !fs->remove(Path)) { held = true; return; } + if (primary == ImageReadResult::Unreadable + || (primary != ImageReadResult::Missing && !removeIfPresent(Path))) { + held = true; + return; + } if (!fs->rename(Backup, Path)) { held = true; return; } loaded = true; } if (loaded) memcpy(&factor, image + 4, sizeof(factor)); - else held = fs->exists(Path) || fs->exists(Backup); + else { + held = primary != ImageReadResult::Missing || backup != ImageReadResult::Missing; + // Only an explicit later setter may recreate conclusively corrupt OTA + // policy files. An unreadable file stays untouched for a clean reboot. + recoverable_corrupt_store = held + && primary != ImageReadResult::Unreadable + && backup != ImageReadResult::Unreadable + && (primary == ImageReadResult::Invalid || backup == ImageReadResult::Invalid); + } } float speedFactor() { return factor; } diff --git a/test/test_companion_ota_config.py b/test/test_companion_ota_config.py index e4f2e74a7..1b55a5b51 100644 --- a/test/test_companion_ota_config.py +++ b/test/test_companion_ota_config.py @@ -204,8 +204,10 @@ int main() { fs.files["/ota_config"][4]^=1; const auto damaged=fs.files["/ota_config"]; reboot(fs); - command("ota config hops 7", "ERR"); - assert(fs.files["/ota_config"]==damaged); + command("ota config hops 7", "OK"); + assert(fs.files["/ota_config"]!=damaged && context.manager.hops==7); + reboot(fs); + assert(context.manager.hops==7); fs.files.clear(); reboot(fs); acquire_ok=false; command("ota config hops 7", "ERR unavailable"); diff --git a/test/test_ota_speed.py b/test/test_ota_speed.py index 9bc36cf44..4c6202855 100644 --- a/test/test_ota_speed.py +++ b/test/test_ota_speed.py @@ -61,8 +61,10 @@ int main() { fs.files["/ota_speed"][4] ^= 1; // damaged image is not partly adopted beginSpeedConfig(&fs); assert(speedFactor() == 1.0f); - command("set ota.speed 2", "ERR"); - assert(speedFactor() == 1.0f); + command("set ota.speed 2", "OK ota.speed=2x (saved)"); + assert(speedFactor() == 2.0f); + beginSpeedConfig(&fs); + assert(speedFactor() == 2.0f); fs.files.clear(); beginSpeedConfig(&fs); command("ota set speed 1.125", "OK ota.speed=1.125x");