From 4efc8675ce465c9e1574fad9d4096f50018ffd13 Mon Sep 17 00:00:00 2001 From: Marcel Verdult Date: Thu, 22 Oct 2026 09:42:59 +0000 Subject: [PATCH] fix(identity): report a failed identity write and verify it after close What changes - IdentityStore::save() returns the result of writing the identity instead of always returning true. Both overloads were affected: the two argument form computed the result, logged it and then discarded it, and the form that also stores a display name ignored the result of both writes. - In that second form the display name write is now part of the result, so a medium that fills up between the keypair and the name is a failure, and it gets the debug line the other form already had. - Both overloads read the file back after closing it and compare what the medium returns: all 96 bytes of the keypair, and the 32 bytes of the display name in the overload that stores one. A save reports success only once the identity can be read back. - New private helpers verifySaved(), and a file local sameIdentity() that compares two identities through the buffer serialisation LocalIdentity already offers, since prv_key is private. - LocalIdentity::writeTo(uint8_t*, size_t) becomes const. It only reads, and the comparison would otherwise need a copy of both identities, which costs 192 bytes of stack on a path that has little to spare. Why - Every role generates a keypair when the load at boot finds no file and then saves it: simple_repeater/main.cpp:94, simple_room_server/main.cpp:75, simple_sensor/main.cpp:101, kiss_modem/main.cpp:56, simple_secure_chat/main.cpp:327, companion_radio/MyMesh.cpp:955. When that write fails the node keeps running under an identity that never reached the medium, and the next boot finds no identity and generates a new random one. - One caller already acts on the result and is written for an honest one: CMD_IMPORT_PRIVATE_KEY in companion_radio/MyMesh.cpp:1544 adopts the imported key and answers OK when save() reports success, and answers ERR_CODE_FILE_IO_ERROR when it does not. While save() always reported success, a failed import was answered with OK. - A write that reports success is not proof that the medium took the data, and File::close() is void in every platform branch of IdentityStore.cpp and drops the error it is handed. Measured with the 96 bytes of an identity against both filesystem stacks this file talks to: - littlefs, nRF52 with prog_size 128 and STM32 with prog_size 2048: a full filesystem and a failing erase are reported by lfs_file_write() itself, as LFS_ERR_NOSPC and LFS_ERR_IO. A failing program is not. The two writes return 32 and 64, and the error turns up in lfs_file_close(), whose result Adafruit_LittleFS' void close() drops. - ESP32, which uses SPIFFS in this repository: nothing reaches the caller at write time at all. The core opens files fully buffered, setvbuf(_IOFBF, 4096) in libraries/FS/src/vfs_api.cpp, and VFSFileImpl::write() is fwrite(), so the 96 bytes stay in that buffer until close() calls fclose() and throws its result away. On a full filesystem fwrite() returns 32 and then 64 and fclose() returns -1. SPIFFSImpl derives from VFSImpl, as does LittleFSImpl, so this is a property of the ESP32 file layer rather than of SPIFFS. - Checking the write results alone would close the gap on two of the four platform branches and only for two of the three failure modes. The read back closes it on all of them. - A read back that cannot be done at all counts as a failed save. That is deliberate: an identity nobody can read is of no use to the next boot. Compatibility - The success path is unchanged. A medium that accepts the write hands the identity back on the read. - All nine call sites were checked for what a false now triggers. Eight discard the result and are unchanged; three of those reach save() through void wrappers (simple_repeater and simple_room_server MyMesh::saveIdentity, SensorMesh::saveIdentity) that cannot propagate it anyway. The only consumer is CMD_IMPORT_PRIVATE_KEY above, which already has its error path. No boot path reads the result, so a failing write adds no error cascade at startup. - The overload that stores a display name has no caller in this repository. - The extra work is one open and one read of 96 or 128 bytes, on a path that runs when a node first generates its identity, imports a private key, or takes a new one over set prv.key. save() is never called in a loop. - Stack: about 500 bytes more per save() on each platform branch, measured from the objects of the builds below. The deepest caller is set prv.key on the nRF52 loop task, whose stack is 4096 bytes; the measured frames there come to about 2.4 KB, and the filesystem chain below them is estimated, not measured. - Flash: 112 bytes more on the two tightest targets in the tree. wio-e5-mini_repeater goes from 203552 to 203664 bytes, 88.7 to 88.8 per cent, with 25712 bytes still free, and wio-e5_repeater from 196360 to 196472. - set prv.key on the server roles still answers "OK, reboot to apply!" whatever the result is, because ICommonCLICallbacks::saveIdentity() is void. Lifting that to bool is a separate change. Evidence - Built for one environment per platform branch: wio_wm1110_repeater (nRF52), wio-e5_repeater and wio-e5-mini_repeater (STM32), PicoW_repeater (RP2040), Heltec_v3_repeater and Heltec_v3_companion_radio_usb (ESP32). All six build, with no new warning in the changed files. --- src/Identity.cpp | 2 +- src/Identity.h | 2 +- src/helpers/IdentityStore.cpp | 68 ++++++++++++++++++++++++++++++++--- src/helpers/IdentityStore.h | 7 ++++ 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/src/Identity.cpp b/src/Identity.cpp index 51a01ae71d8..ab54929143d 100644 --- a/src/Identity.cpp +++ b/src/Identity.cpp @@ -125,7 +125,7 @@ void LocalIdentity::printTo(Stream& s) const { s.print("prv_key: "); Utils::printHex(s, prv_key, PRV_KEY_SIZE); s.println(); } -size_t LocalIdentity::writeTo(uint8_t* dest, size_t max_len) { +size_t LocalIdentity::writeTo(uint8_t* dest, size_t max_len) const { if (max_len < PRV_KEY_SIZE) return 0; // not big enough if (max_len < PRV_KEY_SIZE + PUB_KEY_SIZE) { // only room for prv_key diff --git a/src/Identity.h b/src/Identity.h index 008f7b5bf63..7f2b1b8c885 100644 --- a/src/Identity.h +++ b/src/Identity.h @@ -90,7 +90,7 @@ class LocalIdentity : public Identity { bool readFrom(Stream& s); bool writeTo(Stream& s) const; void printTo(Stream& s) const; - size_t writeTo(uint8_t* dest, size_t max_len); + size_t writeTo(uint8_t* dest, size_t max_len) const; void readFrom(const uint8_t* src, size_t len); }; diff --git a/src/helpers/IdentityStore.cpp b/src/helpers/IdentityStore.cpp index dc85d69cddc..42b84069e64 100644 --- a/src/helpers/IdentityStore.cpp +++ b/src/helpers/IdentityStore.cpp @@ -42,6 +42,56 @@ bool IdentityStore::load(const char *name, mesh::LocalIdentity& id, char display return loaded; } +// Compares two identities over all 96 bytes. prv_key is private, so the +// comparison runs over the buffer serialisation that LocalIdentity offers +// publicly. Its layout does not have to match the file, it only has to be the +// same on both sides. +static bool sameIdentity(const mesh::LocalIdentity& a, const mesh::LocalIdentity& b) { + uint8_t ab[PRV_KEY_SIZE + PUB_KEY_SIZE]; + uint8_t bb[PRV_KEY_SIZE + PUB_KEY_SIZE]; + return a.writeTo(ab, sizeof(ab)) == sizeof(ab) + && b.writeTo(bb, sizeof(bb)) == sizeof(bb) + && memcmp(ab, bb, sizeof(ab)) == 0; +} + +// A write that reports success is not proof that the medium took the data, so +// both overloads read the file back after closing it. File::close() is void in +// every platform branch of this file and drops the error it is handed, which +// is where the gap is. +// +// Measured against the two filesystem stacks this file talks to, with the 96 +// bytes of an identity: +// +// littlefs, on nRF52 with prog_size 128 and on STM32 with prog_size 2048: a +// full filesystem and a failing erase are reported by lfs_file_write() itself, +// as LFS_ERR_NOSPC and LFS_ERR_IO. A failing program is not. The two writes +// return 32 and 64 and the error turns up in lfs_file_close(), whose result +// Adafruit_LittleFS' void close() drops. +// +// ESP32: nothing reaches the caller at write time at all. The core opens files +// fully buffered, setvbuf(_IOFBF, 4096) in libraries/FS/src/vfs_api.cpp, and +// VFSFileImpl::write() is fwrite(), so the 96 bytes stay in that buffer until +// close() calls fclose() and throws its result away. On a full filesystem +// fwrite() returns 32 and then 64 and fclose() returns -1. Both ESP32 +// filesystems are affected, LittleFSImpl and SPIFFSImpl derive from VFSImpl. +// +// A read back that cannot be done at all counts as a failed save. That is +// deliberate: an identity nobody can read is of no use to the next boot. +bool IdentityStore::verifySaved(const char *name, const mesh::LocalIdentity& id) { + mesh::LocalIdentity stored; + if (!load(name, stored)) return false; // file gone, empty or too short + return sameIdentity(id, stored); +} + +bool IdentityStore::verifySaved(const char *name, const mesh::LocalIdentity& id, const uint8_t expected_name[32]) { + mesh::LocalIdentity stored; + char stored_name[32]; + memset(stored_name, 0, sizeof(stored_name)); // load() fills at most 32 bytes + if (!load(name, stored, stored_name, (int) sizeof(stored_name))) return false; + if (!sameIdentity(id, stored)) return false; + return memcmp(stored_name, expected_name, sizeof(stored_name)) == 0; +} + bool IdentityStore::save(const char *name, const mesh::LocalIdentity& id) { char filename[40]; sprintf(filename, "%s/%s.id", _dir, name); @@ -58,7 +108,11 @@ bool IdentityStore::save(const char *name, const mesh::LocalIdentity& id) { bool success = id.writeTo(file); file.close(); MESH_DEBUG_PRINTLN("IdentityStore::save() write - %s", success ? "OK" : "Err"); - return true; + if (success && !verifySaved(name, id)) { + MESH_DEBUG_PRINTLN("IdentityStore::save() verify - Err"); + success = false; + } + return success; } MESH_DEBUG_PRINTLN("IdentityStore::save() failed"); return false; @@ -77,17 +131,23 @@ bool IdentityStore::save(const char *name, const mesh::LocalIdentity& id, const File file = _fs->open(filename, "w", true); #endif if (file) { - id.writeTo(file); + bool success = id.writeTo(file); uint8_t tmp[32]; memset(tmp, 0, sizeof(tmp)); int n = strlen(display_name); if (n > sizeof(tmp)-1) n = sizeof(tmp)-1; memcpy(tmp, display_name, n); - file.write(tmp, sizeof(tmp)); + success = success && (file.write(tmp, sizeof(tmp)) == sizeof(tmp)); file.close(); - return true; + MESH_DEBUG_PRINTLN("IdentityStore::save() write - %s", success ? "OK" : "Err"); + if (success && !verifySaved(name, id, tmp)) { + MESH_DEBUG_PRINTLN("IdentityStore::save() verify - Err"); + success = false; + } + return success; } + MESH_DEBUG_PRINTLN("IdentityStore::save() failed"); return false; } diff --git a/src/helpers/IdentityStore.h b/src/helpers/IdentityStore.h index d0d7ee457ef..29d404f8c69 100644 --- a/src/helpers/IdentityStore.h +++ b/src/helpers/IdentityStore.h @@ -23,4 +23,11 @@ class IdentityStore { bool load(const char *name, mesh::LocalIdentity& id, char display_name[], int max_name_sz); bool save(const char *name, const mesh::LocalIdentity& id); bool save(const char *name, const mesh::LocalIdentity& id, const char display_name[]); + +private: + // Reads back what save() just wrote and compares it. The comment above the + // definition in IdentityStore.cpp says why the return value of write() is + // not enough on its own. + bool verifySaved(const char *name, const mesh::LocalIdentity& id); + bool verifySaved(const char *name, const mesh::LocalIdentity& id, const uint8_t expected_name[32]); };