Copy only persisted Companion preference values during loading

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

@@ -126,6 +126,7 @@ jobs:
           python3 -B test/test_companion_discovery_correlation.py -v
           python3 -B test/test_pico_ble_queues.py -v
           python3 -B test/test_companion_preferences_transaction.py -v
+          python3 -B test/test_companion_prefs_values.py -v
           python3 -B test/test_companion_prefs_tail.py -v
           python3 -B test/test_companion_radio_settings_transaction.py -v
           python3 -B test/test_companion_prefs_transactions.py -v

examples/companion_radio/DataStore.cpp

@@ -998,7 +998,11 @@ bool DataStore::loadPrefsInt(const char *filename,
                              double& node_lon) {
   File file = openRead(_fs, filename);
   if (file) {
-    CompanionNodePrefs loaded_prefs = _prefs;
+    CompanionNodePrefs loaded_prefs;
+    if (!loaded_prefs.copyPersistedValuesFrom(_prefs)) {
+      file.close();
+      return false;
+    }
     // Files written before this tail existed always started Bluetooth at boot.
     // Do not inherit a runtime off value when loading one of those images.
     loaded_prefs.bluetooth_enabled = 1;
@@ -1230,7 +1234,7 @@ bool DataStore::loadPrefsInt(const char *filename,
     file.close();
     if (!success) return false;
     loaded_prefs.usb_debug_enabled = loaded_prefs.usb_debug_enabled == 1 ? 1 : 0;
-    _prefs = loaded_prefs;
+    if (!_prefs.copyPersistedValuesFrom(loaded_prefs)) return false;
     node_lat = loaded_lat;
     node_lon = loaded_lon;
     return true;

examples/companion_radio/NodePrefs.h

@@ -247,6 +247,44 @@ private:
 public:
   CompanionNodePrefs() : radio(this), custom(&radio) { }
 
+  // Only the scalar/array value prefix is a snapshot. Copying the complete
+  // object also copies adapter owner/fallback pointers and costs two large
+  // memberwise copies in the transactional loader. Keep both live adapters
+  // untouched, and leave a normally constructed snapshot's adapters local.
+  bool copyPersistedValuesFrom(const CompanionNodePrefs& source) {
+    if (&source == this) return true;
+    static_assert(sizeof(usb_debug_enabled) == sizeof(uint8_t),
+                  "The published USB debug preference is one byte");
+    const uintptr_t first = reinterpret_cast<uintptr_t>(&airtime_factor);
+    const uintptr_t last = reinterpret_cast<uintptr_t>(&usb_debug_enabled);
+    if (last < first
+        || last - first > sizeof(*this) - sizeof(usb_debug_enabled)) {
+      return false;
+    }
+    const size_t size = static_cast<size_t>(last - first)
+        + sizeof(usb_debug_enabled);
+    const auto bounded = [size](const CompanionNodePrefs& values) -> bool {
+      const uintptr_t object = reinterpret_cast<uintptr_t>(&values);
+      const uintptr_t begin =
+          reinterpret_cast<uintptr_t>(&values.airtime_factor);
+      const uintptr_t end =
+          reinterpret_cast<uintptr_t>(&values.usb_debug_enabled);
+      const uintptr_t radio_begin = reinterpret_cast<uintptr_t>(&values.radio);
+      const uintptr_t custom_begin = reinterpret_cast<uintptr_t>(&values.custom);
+      // Subtractions happen only after the corresponding ordering checks.
+      // Never include either runtime adapter, even if this layout is changed.
+      return begin >= object && begin - object <= sizeof(values)
+          && size <= sizeof(values) - static_cast<size_t>(begin - object)
+          && end >= begin
+          && end - begin == size - sizeof(values.usb_debug_enabled)
+          && radio_begin >= begin && size <= radio_begin - begin
+          && custom_begin >= begin && size <= custom_begin - begin;
+    };
+    if (!bounded(*this) || !bounded(source)) return false;
+    memcpy(&airtime_factor, &source.airtime_factor, size);
+    return true;
+  }
+
   bool isRepeatEn() const { return client_repeat != 0; }
   void setRepeatEn(bool enabled) { client_repeat = enabled ? 1 : 0; }
 

test/test_companion_prefs_values.py

@@ -0,0 +1,240 @@
+"""Exercise value-only snapshots using the real adapter-bearing preferences."""
+from pathlib import Path
+import os
+import re
+import subprocess
+import tempfile
+import unittest
+
+from test_companion_preferences_transaction import HARNESS, esp_recovery_helpers
+from test_radio_receive_contract import method
+
+ROOT = Path(__file__).resolve().parents[1]
+
+TESTS = r'''
+#include <type_traits>
+#include <vector>
+@FIELD_CHECKS@
+
+static void seedValues(CompanionNodePrefs& prefs, uint32_t& random) {
+  auto fill = [&](void* destination, size_t size) {
+    auto* bytes = static_cast<uint8_t*>(destination);
+    for (size_t i = 0; i < size; ++i) {
+      random = random * 1664525u + 1013904223u;
+      bytes[i] = static_cast<uint8_t>(random >> 24);
+    }
+  };
+  @SEED_FIELDS@
+}
+
+static void equalValues(const CompanionNodePrefs& a,
+                        const CompanionNodePrefs& b) {
+  @COMPARE_FIELDS@
+}
+
+static size_t adapterOffset(const CompanionNodePrefs& prefs) {
+  // The value helper does not copy the adapters or their inter-field padding.
+  // Use the complete object's byte representation for bounded observation.
+  const uintptr_t first = reinterpret_cast<uintptr_t>(&prefs);
+  const uintptr_t last = reinterpret_cast<uintptr_t>(&prefs.usb_debug_enabled);
+  assert(last >= first && last - first < sizeof(prefs));
+  return static_cast<size_t>(last - first) + sizeof(prefs.usb_debug_enabled);
+}
+
+static std::vector<uint8_t> adapterBytes(const CompanionNodePrefs& prefs) {
+  const auto* bytes = reinterpret_cast<const uint8_t*>(&prefs);
+  return {bytes + adapterOffset(prefs), bytes + sizeof(prefs)};
+}
+
+static void expectTag(CompanionNodePrefs& prefs, const char* expected) {
+  char value[32] = {};
+  assert(prefs.getCustom()->getByKey("snapshot_tag", value, sizeof(value)));
+  assert(!strcmp(value, expected));
+}
+
+int main() {
+  uint32_t random = 0x19283746;
+  for (unsigned trial = 0; trial < 64; ++trial) {
+    CompanionNodePrefs source, snapshot;
+    seedValues(source, random);
+    source.getRadioPrefs()->setFreq(915.25f);
+    assert(source.getCustom()->setByKey("snapshot_tag", "source"));
+    assert(snapshot.getCustom()->setByKey("snapshot_tag", "snapshot"));
+    snapshot.clearDirty();
+    assert(source.isDirty() && !snapshot.isDirty());
+    auto source_adapters = adapterBytes(source);
+    auto snapshot_adapters = adapterBytes(snapshot);
+    assert(snapshot.copyPersistedValuesFrom(source));
+    equalValues(source, snapshot);
+    assert(adapterBytes(source) == source_adapters);
+    assert(adapterBytes(snapshot) == snapshot_adapters);
+    assert(source.isDirty() && !snapshot.isDirty());
+    expectTag(source, "source"); expectTag(snapshot, "snapshot");
+    const auto* bytes = reinterpret_cast<const uint8_t*>(&snapshot);
+    std::vector<uint8_t> before_self(bytes, bytes + sizeof(snapshot));
+    assert(snapshot.copyPersistedValuesFrom(snapshot));
+    assert(!memcmp(&snapshot, before_self.data(), before_self.size()));
+    // A normally constructed scratch adapter must still own the scratch
+    // object, including its dynamic-config fallback after copying values.
+    snapshot.getRadioPrefs()->setFreq(910.5f);
+    assert(snapshot.freq == 910.5f && source.freq == 915.25f);
+    source.radio_fem_txgain = 0;
+    assert(snapshot.getCustom()->setByKey("fem_txgain", "1"));
+    assert(snapshot.radio_fem_txgain == 1 && source.radio_fem_txgain == 0);
+    source.getRadioPrefs()->setFreq(920.5f);
+    assert(source.freq == 920.5f && snapshot.freq == 910.5f);
+  }
+
+  unsigned comparisons = 0;
+  for (unsigned trial = 0; trial < 3; ++trial) {
+    CompanionNodePrefs saved;
+    seedValues(saved, random);
+    saved.bluetooth_enabled = static_cast<uint8_t>(trial);
+    DataStore image_store;
+    assert(image_store.savePrefs(saved, 47.125, -122.25));
+    const auto image = image_store.fs.files["/new_prefs"];
+    // Test every possible old/torn image length, including every officially
+    // supported append boundary and unknown lengths beyond the current tail.
+    for (size_t length = 0; length <= image.size() + 5; ++length) {
+      auto disk = image;
+      disk.resize(length);
+      for (size_t quota = 0; quota <= length; ++quota) {
+        DataStore actual_store, legacy_store;
+        actual_store.fs.files["/new_prefs"] = disk;
+        legacy_store.fs.files["/new_prefs"] = disk;
+        actual_store.fs.fail_read_after = static_cast<int>(quota);
+        legacy_store.fs.fail_read_after = static_cast<int>(quota);
+        CompanionNodePrefs actual, legacy;
+        seedValues(actual, random);
+        assert(legacy.copyPersistedValuesFrom(actual));
+        actual.getRadioPrefs()->setFreq(915.25f);
+        legacy.getRadioPrefs()->setFreq(915.25f);
+        assert(actual.getCustom()->setByKey("snapshot_tag", "actual"));
+        assert(legacy.getCustom()->setByKey("snapshot_tag", "legacy"));
+        auto actual_adapters = adapterBytes(actual);
+        auto legacy_adapters = adapterBytes(legacy);
+        double lat = 1, lon = 2, legacy_lat = 1, legacy_lon = 2;
+        const bool expected = legacy_store.loadPrefsLegacy(
+            "/new_prefs", legacy, legacy_lat, legacy_lon);
+        const bool got = actual_store.loadPrefsInt(
+            "/new_prefs", actual, lat, lon);
+        assert(got == expected);
+        equalValues(actual, legacy);
+        assert(!memcmp(&lat, &legacy_lat, sizeof(lat)));
+        assert(!memcmp(&lon, &legacy_lon, sizeof(lon)));
+        assert(actual_store.fs.files["/new_prefs"] == disk);
+        assert(legacy_store.fs.files["/new_prefs"] == disk);
+        assert(adapterBytes(actual) == actual_adapters);
+        assert(adapterBytes(legacy) == legacy_adapters);
+        assert(actual.isDirty() && legacy.isDirty());
+        expectTag(actual, "actual"); expectTag(legacy, "legacy");
+        auto legacy_txgain = legacy.radio_fem_txgain;
+        assert(actual.getCustom()->setByKey("fem_txgain", "1"));
+        assert(actual.radio_fem_txgain == 1
+            && legacy.radio_fem_txgain == legacy_txgain);
+        ++comparisons;
+      }
+    }
+  }
+  printf("PASS: %u actual-loader value/adapter/failure comparisons\n", comparisons);
+}
+'''
+
+
+class CompanionPrefsValuesTests(unittest.TestCase):
+    def test_value_only_copy_and_full_loader_match_legacy(self):
+        source = (ROOT / 'examples/companion_radio/DataStore.cpp').read_text()
+        loader = method(source, 'bool DataStore::loadPrefsInt(')
+        legacy = loader.replace('DataStore::loadPrefsInt(', 'DataStore::loadPrefsLegacy(', 1)
+        legacy = legacy.replace('''    CompanionNodePrefs loaded_prefs;
+    if (!loaded_prefs.copyPersistedValuesFrom(_prefs)) {
+      file.close();
+      return false;
+    }''', '    CompanionNodePrefs loaded_prefs = _prefs;')
+        legacy = legacy.replace(
+            '    if (!_prefs.copyPersistedValuesFrom(loaded_prefs)) return false;',
+            '    _prefs = loaded_prefs;')
+        self.assertNotIn('copyPersistedValuesFrom', legacy)
+        methods = esp_recovery_helpers(source) + '\n'.join((loader, legacy,
+            method(source, 'bool DataStore::savePrefs(')))
+        code = HARNESS.split('int main(')[0].replace('@METHODS@', methods)
+        code = code.replace(' bool loadPrefsInt(const char*, CompanionNodePrefs&, double&, double&);',
+                            ' bool loadPrefsInt(const char*, CompanionNodePrefs&, double&, double&);\n'
+                            ' bool loadPrefsLegacy(const char*, CompanionNodePrefs&, double&, double&);')
+        prefs = (ROOT / 'examples/companion_radio/NodePrefs.h').read_text()
+        prefix = prefs[prefs.index('class CompanionNodePrefs {'):prefs.index('\nprivate:')]
+        declarations = re.findall(
+            r'^\s*(?:float|char|u?int(?:8|16|32)_t)\s+([A-Za-z_][A-Za-z_0-9]*)',
+            prefix, re.M)
+        self.assertEqual(declarations[0], 'airtime_factor')
+        self.assertEqual(declarations[-1], 'usb_debug_enabled')
+        optional = {
+            'fan_mode': 'TBEAM_1W', 'fan_lo': 'TBEAM_1W', 'fan_hi': 'TBEAM_1W',
+            'wifi_ssid': 'defined(RP2040_PLATFORM) && defined(ENABLE_WIFI_INTERFACE)',
+            'wifi_pwd': 'defined(RP2040_PLATFORM) && defined(ENABLE_WIFI_INTERFACE)',
+        }
+
+        def fields(template):
+            rows = []
+            for name in declarations:
+                row = template.format(name=name)
+                if name in optional:
+                    condition = optional[name]
+                    if not condition.startswith('defined('):
+                        condition = 'defined(' + condition + ')'
+                    row = '#if ' + condition + '\n' + row + '\n#endif'
+                rows.append(row)
+            return '\n'.join(rows)
+
+        tests = TESTS.replace('@FIELD_CHECKS@', fields(
+            'static_assert(std::is_trivially_copyable<decltype(CompanionNodePrefs::{name})>::value,\n'
+            '              "A persisted snapshot member must remain a value");'))
+        tests = tests.replace('@SEED_FIELDS@', fields('fill(&prefs.{name}, sizeof(prefs.{name}));'))
+        tests = tests.replace('@COMPARE_FIELDS@', fields(
+            'assert(!memcmp(&a.{name}, &b.{name}, sizeof(a.{name})));'))
+        with tempfile.TemporaryDirectory(prefix='meshcore-prefs-values-') as directory:
+            work = Path(directory)
+            (work / 'Utils.h').write_text('''#pragma once
+#include <Arduino.h>
+#include <cassert>
+namespace mesh { struct Utils {
+ static void printHex(Stream&,const uint8_t*,size_t){assert(false);}
+ static void fromHex(uint8_t*,size_t,const char*){assert(false);}
+}; }
+''')
+            (work / 'platform_shim.h').write_text('''#include <cstdlib>
+#include <cstdio>
+inline char* utoa(unsigned int value,char* output,int base){
+ if(base!=10)abort();sprintf(output,"%u",value);return output;
+}
+''')
+            transaction = (ROOT / 'src/helpers/ContactFileTransaction.h').read_text()
+            (work / 'ContactFileTransaction.h').write_text(transaction.replace(
+                '#include "IdentityStore.h"', '#include <helpers/IdentityStore.h>'))
+            cpp = work / 'test.cpp'
+            cpp.write_text(code + tests)
+            for platform in ('STM32_PLATFORM', 'NRF52_PLATFORM', 'ESP32_PLATFORM', 'RP2040_PLATFORM',
+                             'ESP32_PLATFORM,TBEAM_1W', 'RP2040_PLATFORM,ENABLE_WIFI_INTERFACE'):
+                with self.subTest(platform=platform):
+                    binary = work / 'test'
+                    sanitizers = [] if os.name == 'nt' else [
+                        '-fsanitize=address,undefined', '-fno-sanitize-recover=all',
+                        '-fno-pie', '-no-pie']
+                    built = subprocess.run([os.environ.get('CXX', 'g++'), '-std=c++17',
+                        *['-D' + flag + '=1' for flag in platform.split(',')],
+                        *sanitizers, '-include', str(work / 'platform_shim.h'),
+                        '-I', str(work), '-I', str(ROOT / 'test/fixtures/radio_profiles/mocks'),
+                        '-I', str(ROOT / 'test/mocks'), '-I', str(ROOT / 'src'),
+                        '-I', str(ROOT / 'src/helpers'), '-I', str(ROOT), str(cpp),
+                        *[str(ROOT / 'src/helpers' / name) for name in (
+                            'ConfigSerializer.cpp', 'DynamicConfigSerializer.cpp',
+                            'CommonRadioPrefs.cpp', 'TxtDataHelpers.cpp')],
+                        '-o', str(binary)], capture_output=True, text=True)
+                    self.assertEqual(built.returncode, 0, built.stdout + built.stderr)
+                    self.assertNotIn('offsetof', built.stderr)
+                    ran = subprocess.run([str(binary)], capture_output=True, text=True, timeout=60)
+                    self.assertEqual(ran.returncode, 0, ran.stdout + ran.stderr)
+
+
+if __name__ == '__main__':
+    unittest.main()

test/test_identity_and_settings_recovery.py

@@ -14,21 +14,39 @@ def fixture_prefix():
     return text[:text.index('struct Host {')]
 
 
-def compile_run(program, platform):
+def compile_run(program, platform, actual_node_prefs=False):
     with tempfile.TemporaryDirectory(prefix='mesh-storage-recovery-') as directory:
         directory = Path(directory)
         (directory / 'FS.h').write_text('#pragma once\nnamespace fs { using FS = FakeFilesystem; }\n')
         (directory / 'Adafruit_LittleFS.h').write_text(
             '#pragma once\nusing Adafruit_LittleFS = FakeFilesystem;\n'
             'namespace Adafruit_LittleFS_Namespace {}\n')
+        helper_sources = []
+        if actual_node_prefs:
+            # Keep the actual owner-bearing adapters in the preferences
+            # recovery test, rather than replacing the new snapshot API.
+            (directory / 'Arduino.h').write_text(
+                '#include "' + str(ROOT / 'test/mocks/Arduino.h') + '"\n')
+            utils = (ROOT / 'test/fixtures/contact_cache/mocks/Utils.h').read_text()
+            utils = utils.replace('public:', '''public:
+  static void printHex(Stream&, const uint8_t*, size_t) { assert(false); }
+  static void fromHex(uint8_t*, size_t, const char*) { assert(false); }
+''', 1)
+            (directory / 'Utils.h').write_text('#include <cassert>\n' + utils)
+            helper_sources = [str(ROOT / 'src/helpers' / name) for name in (
+                'ConfigSerializer.cpp', 'DynamicConfigSerializer.cpp',
+                'CommonRadioPrefs.cpp', 'TxtDataHelpers.cpp')]
         cpp, exe = directory / 'test.cpp', directory / 'test'
         cpp.write_text(program)
         compiled = subprocess.run([
             'c++', '-std=c++17', '-O1', '-g', '-fsanitize=address,undefined',
-            '-fno-pie', '-no-pie', '-DMESH_CONTACT_CACHE=0', '-D' + platform + '=1',
+            '-fno-sanitize-recover=all', '-fno-pie', '-no-pie',
+            '-DMESH_CONTACT_CACHE=0', '-D' + platform + '=1',
             '-I', str(directory), '-I', str(ROOT / 'test/fixtures/contact_cache/mocks'),
+            '-I', str(ROOT / 'test/mocks'),
             '-I', str(ROOT / 'src'), '-I', str(ROOT / 'lib/ed25519'),
-            '-I', str(ROOT / 'examples/companion_radio'), str(cpp), '-o', str(exe),
+            '-I', str(ROOT / 'examples/companion_radio'), str(cpp), *helper_sources,
+            '-o', str(exe),
         ], capture_output=True, text=True, timeout=60)
         if compiled.returncode:
             raise AssertionError(compiled.stdout + compiled.stderr)
@@ -233,12 +251,8 @@ int main() {
 
     def test_esp32_preferences_and_channels_recover_or_reset(self):
         store = (ROOT / 'examples/companion_radio/DataStore.cpp').read_text()
-        prefs = (ROOT / 'examples/companion_radio/NodePrefs.h').read_text()
-        fields = prefs[prefs.index('class CompanionNodePrefs {'):prefs.index('\nprivate:')]
         program = fixture_prefix() + r'''
-#include <helpers/BluetoothMac.h>
-#include "BluetoothName.h"
-''' + fields + '\n};\n' + r'''
+#include "NodePrefs.h"
 struct ChannelDetails { struct { uint8_t secret[32] = {}; uint8_t tx_radio = 0; } channel; char name[32] = {}; };
 #define MAX_GROUP_CHANNELS 40
 struct DataStoreHost {
@@ -330,7 +344,7 @@ int main() {
   }
 }
 '''
-        compile_run(program, 'ESP32_PLATFORM')
+        compile_run(program, 'ESP32_PLATFORM', actual_node_prefs=True)
 
 
 if __name__ == '__main__':