Repository navigation
Fix IdentityStore::save() reporting success when the write failed - #3635
Open
marcelverdult wants to merge 1 commit into
Open
marcelverdult wants to merge 1 commit into
marcelverdult wants to merge 1 commit into
Conversation
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.
marcelverdult
force-pushed
the
pr/identity-store-save-result
branch
from
October 22, 2026 09:43
7e27a98 to
4efc867
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IdentityStore::save()always returnedtrueonce the file had opened. The two argument form computed the result ofwriteTo(), logged it and then threw it away. The form that also stores a display name ignored the result of both writes.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 does not reach the medium, the node keeps running under an identity it never stored, and comes back with a different public key after the next boot.One caller already acts on the result.
CMD_IMPORT_PRIVATE_KEYincompanion_radio/MyMesh.cpp:1544adopts the imported key and answers OK whensave()reports success, andERR_CODE_FILE_IO_ERRORwhen it does not. Until now a failed import was answered with OK although the identity had not been stored. The old key does not come back either: every platform branch removes or truncates the file before writing, so the next boot finds no identity and generates a new random one.Both overloads now return the result of the writes. In the second form the display name write counts towards the result as well, so a medium that fills up between the keypair and the name is a failure, and that form gets the debug line the other one already had.
That alone covers only part of it.
File::close()isvoidin every platform branch of this file and drops the error it is handed, so a failure that only surfaces when the file is closed never reaches the caller. Measured with the 96 bytes of an identity, against both filesystem stacks this file talks to:prog_size128 and on STM32 withprog_size2048: a full filesystem and a failing erase are reported bylfs_file_write()itself, asLFS_ERR_NOSPCandLFS_ERR_IO. A failing program is not. The two writes return 32 and 64, and the error turns up inlfs_file_close(), whose result thevoid close()of Adafruit_LittleFS drops.setvbuf(_IOFBF, 4096)inlibraries/FS/src/vfs_api.cpp, andVFSFileImpl::write()isfwrite(), so the 96 bytes stay in that buffer untilclose()callsfclose()and throws its result away. On a full filesystemfwrite()returns 32 and then 64, andfclose()returns -1.SPIFFSImplderives fromVFSImpland so doesLittleFSImpl, so this is a property of the ESP32 file layer rather than of SPIFFS.So both overloads read the file back after closing it and compare all 96 bytes of the keypair, plus the 32 bytes of the display name in the overload that stores one.
prv_keyis private, so the comparison goes through the buffer serialisationLocalIdentityalready offers; its layout does not have to match the file, it only has to be the same on both sides. That serialisation becomesconst, since 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. A read back that cannot be done at all counts as a failed save: an identity nobody can read is of no use to the next boot.The success path is unchanged. Of the nine call sites, eight discard the result, and three of those reach
save()through void wrappers that could not propagate it anyway. No boot path reads the result, so a failing write adds no new error handling at startup. The overload that stores a display name has no caller in this repository. One gap is left on purpose:set prv.keyon the server roles answers "OK, reboot to apply!" whatever the result is, becauseICommonCLICallbacks::saveIdentity()isvoid. Lifting that toboolis a separate change.A new native environment,
native_identity_store, runsIdentityStoreagainst a mock filesystem. Next to its write budget the mock has a commit budget, for the medium that takes every write and loses the data when the file is closed, and a knob that returns one byte differently while keeping the file length. 19 cases, two of which walk every byte of the keypair and of the name, so a comparison that is one byte short cannot pass. The unit test workflow runs the environment next to the existing ones.Three mutation probes, each caught by the suite: on code that only returns the write result, 8 of the 19 cases fail; shortening the keypair comparison by one byte fails the case that walks the keypair; shortening the name comparison to 8 bytes fails the case that walks the name.
Verified with
pio test -e native -e native_kiss_modem -e native_identity_store, 73 of 73 pass.Built for one environment per platform branch in the file:
wio_wm1110_repeater(nRF52),wio-e5_repeaterandwio-e5-mini_repeater(STM32),PicoW_repeater(RP2040), andHeltec_v3_repeaterplusHeltec_v3_companion_radio_usb(ESP32), the last one being the environment that also compiles theCMD_IMPORT_PRIVATE_KEYcaller. All six build. The read back costs 112 bytes of flash on the two tightest targets,wio-e5-mini_repeatergoing from 203552 to 203664 bytes with 25712 bytes still free, and about 500 bytes of stack per save.Checked on hardware as well, on a LilyGo T-Echo (nRF52840) running a build from this branch: the stored identity loads, writing it back over
set prv.keygoes throughsave()and the read back, and the node reboots afterwards. The write reports OK, the read back raises no error, and the public key is unchanged after the reboot. The ESP32 side has not been run on a device.