Skip to content

Fix IdentityStore::save() reporting success when the write failed - #3635

Open
marcelverdult wants to merge 1 commit into
meshcore-dev:devfrom
marcelverdult:pr/identity-store-save-result
Open

marcelverdult wants to merge 1 commit into
meshcore-dev:devfrom
marcelverdult:pr/identity-store-save-result

Conversation

@marcelverdult

@marcelverdult marcelverdult commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

IdentityStore::save() always returned true once the file had opened. The two argument form computed the result of writeTo(), 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_KEY in companion_radio/MyMesh.cpp:1544 adopts the imported key and answers OK when save() reports success, and ERR_CODE_FILE_IO_ERROR when 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() is void in 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:

  • 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 the void close() of Adafruit_LittleFS 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 and so does LittleFSImpl, 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_key is private, so the comparison goes through the buffer serialisation LocalIdentity already offers; its layout does not have to match the file, it only has to be the same on both sides. That serialisation becomes const, 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.key on the server roles answers "OK, reboot to apply!" whatever the result is, because ICommonCLICallbacks::saveIdentity() is void. Lifting that to bool is a separate change.

A new native environment, native_identity_store, runs IdentityStore against 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_repeater and wio-e5-mini_repeater (STM32), PicoW_repeater (RP2040), and Heltec_v3_repeater plus Heltec_v3_companion_radio_usb (ESP32), the last one being the environment that also compiles the CMD_IMPORT_PRIVATE_KEY caller. All six build. The read back costs 112 bytes of flash on the two tightest targets, wio-e5-mini_repeater going 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.key goes through save() 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.

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
marcelverdult force-pushed the pr/identity-store-save-result branch from 7e27a98 to 4efc867 Compare October 22, 2026 09:43

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant