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

.github/workflows/run-unit-tests.yml

@@ -20,7 +20,7 @@ jobs:
         uses: ./.github/actions/setup-build-environment
 
       - name: Run Unit Tests
-        run: pio test -e native -e native_kiss_modem -vv
+        run: pio test -e native -e native_kiss_modem -e native_identity_store -vv
 
       - name: Upload Test Results
         # Upload test results even if the test step failed.

platformio.ini

@@ -167,7 +167,9 @@ build_flags = -std=c++17
   -I src
   -I test/mocks
 test_build_src = yes
-test_ignore = test_kiss_modem
+test_ignore =
+  test_kiss_modem
+  test_identity_store
 build_src_filter =
   -<*>
   +<../src/Utils.cpp>
@@ -177,6 +179,21 @@ build_src_filter =
 lib_deps =
   google/googletest @ 1.17.0
 
+[env:native_identity_store]
+platform = native
+test_framework = googletest
+build_flags = -std=c++17
+  -I test/mocks
+  -I src
+  -D NRF52_PLATFORM
+test_build_src = yes
+test_filter = test_identity_store
+build_src_filter =
+  -<*>
+  +<../src/helpers/IdentityStore.cpp>
+lib_deps =
+  google/googletest @ 1.17.0
+
 [env:native_kiss_modem]
 platform = native
 test_framework = googletest

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

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);
 };
 

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;
 }

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]);
 };

test/mocks/Adafruit_LittleFS.h

@@ -0,0 +1,147 @@
+#pragma once
+
+// Mock of the nRF52 filesystem API for native tests. Only what IdentityStore
+// touches is modelled: open, exists, remove, mkdir, and a File that is a
+// Stream.
+//
+// Two failure modes are modelled, because the platforms differ in which one
+// they show.
+//
+// The write budget: a real flash lets a file open and then refuses the write
+// once the medium is full or worn out, so a test needs a file that is valid but
+// whose writes come up short. Print::write() loops over write(uint8_t), so a
+// budget that runs out mid buffer returns a short count, which is how littlefs
+// reports it on nRF52 and STM32.
+//
+// The commit budget: on ESP32 the core opens files fully buffered, so write()
+// reports success for bytes that are still in a stdio buffer, and only close()
+// hands them over. File::close() is void there and drops the error, which
+// leaves a short file behind that no return value ever mentioned. A commit
+// budget below the number of bytes written reproduces exactly that, and
+// corrupt_on_commit covers the worn cell that keeps the length but changes a
+// byte.
+
+#include <cstddef>
+#include <cstdint>
+#include <limits>
+#include <map>
+#include <string>
+#include <vector>
+
+#include "Stream.h"
+
+#define FILE_O_READ   0
+#define FILE_O_WRITE  1
+
+namespace Adafruit_LittleFS_Namespace {
+
+struct FileData {
+  std::vector<uint8_t> bytes;
+  size_t write_budget = std::numeric_limits<size_t>::max();
+  size_t commit_budget = std::numeric_limits<size_t>::max();      // what close() keeps
+  size_t corrupt_on_commit = std::numeric_limits<size_t>::max();  // index close() flips
+};
+
+class File : public Stream {
+  FileData* _data;
+  size_t _read_pos;
+  bool _open;
+  bool _writing;
+
+public:
+  File() : _data(nullptr), _read_pos(0), _open(false), _writing(false) { }
+  explicit File(FileData* data, bool writing = false)
+    : _data(data), _read_pos(0), _open(data != nullptr), _writing(writing) { }
+
+  // The driver signals "could not open" through the bool conversion, not an
+  // exception or a null return.
+  operator bool() const { return _open; }
+
+  // Only a file that was opened for writing commits on close. A read never
+  // changes what is on the medium.
+  void close() {
+    if (_open && _writing && _data != nullptr) {
+      if (_data->bytes.size() > _data->commit_budget) {
+        _data->bytes.resize(_data->commit_budget);
+      }
+      if (_data->corrupt_on_commit < _data->bytes.size()) {
+        _data->bytes[_data->corrupt_on_commit] ^= 0xFF;
+      }
+    }
+    _open = false;
+  }
+
+  // Overriding the single byte write would otherwise hide the buffer overloads
+  // of Print, which is what the callers use.
+  using Print::write;
+
+  size_t write(uint8_t b) override {
+    if (!_open || _data == nullptr || _data->write_budget == 0) return 0;
+    _data->bytes.push_back(b);
+    _data->write_budget--;
+    return 1;
+  }
+
+  int available() override {
+    if (!_open || _data == nullptr) return 0;
+    return (int) (_data->bytes.size() - _read_pos);
+  }
+
+  int read() override {
+    if (available() <= 0) return -1;
+    return _data->bytes[_read_pos++];
+  }
+
+  int peek() override {
+    if (available() <= 0) return -1;
+    return _data->bytes[_read_pos];
+  }
+
+  // The driver's File offers a buffer read next to the Stream interface, and
+  // IdentityStore uses it to pick up the display name.
+  size_t read(uint8_t* buffer, size_t size) {
+    size_t n = 0;
+    while (n < size && available() > 0) buffer[n++] = (uint8_t) read();
+    return n;
+  }
+};
+
+}  // namespace Adafruit_LittleFS_Namespace
+
+class Adafruit_LittleFS {
+  std::map<std::string, Adafruit_LittleFS_Namespace::FileData> _files;
+
+public:
+  // Test knobs.
+  size_t write_budget = std::numeric_limits<size_t>::max();
+  size_t commit_budget = std::numeric_limits<size_t>::max();
+  size_t corrupt_on_commit = std::numeric_limits<size_t>::max();
+  bool fail_open = false;
+
+  bool exists(const char* path) { return _files.count(path) > 0; }
+  bool remove(const char* path) { return _files.erase(path) > 0; }
+  bool mkdir(const char* path) { return true; }
+
+  Adafruit_LittleFS_Namespace::File open(const char* path, uint8_t mode = FILE_O_READ) {
+    if (fail_open) return Adafruit_LittleFS_Namespace::File();
+    if (mode == FILE_O_WRITE) {
+      // std::map keeps its values at a stable address, so handing out a
+      // pointer stays valid while other files are added.
+      Adafruit_LittleFS_Namespace::FileData& data = _files[path];
+      data.bytes.clear();
+      data.write_budget = write_budget;
+      data.commit_budget = commit_budget;
+      data.corrupt_on_commit = corrupt_on_commit;
+      return Adafruit_LittleFS_Namespace::File(&data, true);
+    }
+    auto found = _files.find(path);
+    if (found == _files.end()) return Adafruit_LittleFS_Namespace::File();
+    return Adafruit_LittleFS_Namespace::File(&found->second);
+  }
+
+  // Lets a test see what actually reached the medium.
+  const std::vector<uint8_t>* contents(const char* path) {
+    auto found = _files.find(path);
+    return found == _files.end() ? nullptr : &found->second.bytes;
+  }
+};

test/mocks/Identity.h

@@ -2,6 +2,7 @@
 
 #include <cstdint>
 #include <cstring>
+#include "Stream.h"
 #include "Utils.h"
 
 namespace mesh {
@@ -21,11 +22,25 @@ class Identity {
   bool verify(const uint8_t*, const uint8_t*, int) const {
     return true;
   }
+
+  // Copies of the real serialisation in src/Identity.cpp: a short read or a
+  // short write is a failure, which is what the file based tests rely on.
+  bool readFrom(Stream& s) {
+    return s.readBytes(pub_key, PUB_KEY_SIZE) == PUB_KEY_SIZE;
+  }
+
+  bool writeTo(Stream& s) const {
+    return s.write(pub_key, PUB_KEY_SIZE) == PUB_KEY_SIZE;
+  }
 };
 
 class LocalIdentity : public Identity {
 public:
-  LocalIdentity() : Identity() {}
+  uint8_t prv_key[PRV_KEY_SIZE];
+
+  LocalIdentity() : Identity() {
+    std::memset(prv_key, 0, sizeof(prv_key));
+  }
 
   void sign(uint8_t* sig, const uint8_t*, int) const {
     std::memset(sig, 0x5A, SIGNATURE_SIZE);
@@ -34,6 +49,33 @@ class LocalIdentity : public Identity {
   void calcSharedSecret(uint8_t* secret, const uint8_t*) const {
     std::memset(secret, 0x11, PUB_KEY_SIZE);
   }
+
+  bool readFrom(Stream& s) {
+    bool success = s.readBytes(pub_key, PUB_KEY_SIZE) == PUB_KEY_SIZE;
+    success = success && s.readBytes(prv_key, PRV_KEY_SIZE) == PRV_KEY_SIZE;
+    return success;
+  }
+
+  bool writeTo(Stream& s) const {
+    bool success = s.write(pub_key, PUB_KEY_SIZE) == PUB_KEY_SIZE;
+    success = success && s.write(prv_key, PRV_KEY_SIZE) == PRV_KEY_SIZE;
+    return success;
+  }
+
+  // Copy of the buffer serialisation in src/Identity.cpp, private key first.
+  // IdentityStore compares two identities with it, which keeps it off prv_key,
+  // private in the real class.
+  size_t 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
+      std::memcpy(dest, prv_key, PRV_KEY_SIZE);
+      return PRV_KEY_SIZE;
+    }
+    std::memcpy(dest, prv_key, PRV_KEY_SIZE);
+    std::memcpy(&dest[PRV_KEY_SIZE], pub_key, PUB_KEY_SIZE);
+    return PRV_KEY_SIZE + PUB_KEY_SIZE;
+  }
 };
 
 }

test/mocks/Utils.h

@@ -10,6 +10,11 @@
 #define SIGNATURE_SIZE 64
 #define CIPHER_MAC_SIZE 16
 
+// Stands in for the same no-op definitions in src/MeshCore.h, which is the
+// header the real Utils.h pulls these constants from.
+#define MESH_DEBUG_PRINT(...) {}
+#define MESH_DEBUG_PRINTLN(...) {}
+
 namespace mesh {
 
 class RNG {