From a400cc1649179eae2234834dfa670b2ad02c890f Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Sat, 12 Sep 2026 03:14:31 +0200 Subject: [PATCH] fix: CRC im Settings-Record dort schreiben, wo der Leser sie erwartet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit nl_settings_save() rechnete die CRC über [0, offsetof(rec_t, crc)) und legte sie dort ab, read_rec() liest sie bei sizeof(rec_hdr_t) + hdr.length. Beide Offsets stimmen nur überein, solange sizeof(nl_settings_t) ein Vielfaches von 4 ist (aktuell 204). Ein zusätzliches uint16_t - in Plan 4 (Migration) sehr wahrscheinlich - macht 206, und jeder gespeicherte Record fiele beim nächsten Boot still durch die CRC: WLAN, Token und Name wären auf allen Lampen weg. Geschrieben wird jetzt explizit bei NL_SETTINGS_CRC_OFF(length), also exakt an der Leseposition; dazu eine Übersetzungszeit-Zusicherung, dass die Nutzdaten direkt hinter dem Header beginnen (die CRC läuft auf beiden Seiten über einen zusammenhängenden Block). Test: test_crc_offset_matches_reader prüft die Symmetrie unabhängig von der Strukturgröße. Gegenprobe mit einem zusätzlichen uint16_t im Struct: mit der alten Schreibposition fallen vier Settings-Tests, mit der neuen keiner. --- src/config/nl_settings.c | 24 +++++++++++++++++++----- test/test_settings.c | 25 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/config/nl_settings.c b/src/config/nl_settings.c index aa41d47..5376a16 100644 --- a/src/config/nl_settings.c +++ b/src/config/nl_settings.c @@ -24,9 +24,20 @@ typedef struct { typedef struct { rec_hdr_t hdr; nl_settings_t data; - uint32_t crc; + uint32_t crc; /* NB: only a size reserve - the CRC is written at NL_SETTINGS_CRC_OFF() */ } rec_t; +/* Where read_rec() expects the CRC, and therefore where nl_settings_save() must put it: directly + * behind the `length` payload bytes, not at offsetof(rec_t, crc). Both offsets are only equal + * while sizeof(nl_settings_t) is a multiple of 4 (204 today). Adding a single uint16_t would move + * offsetof(rec_t, crc) two bytes further than the reader looks and every stored record would + * silently fail its CRC on the next boot - Wi-Fi, token and name gone on every lamp. */ +#define NL_SETTINGS_CRC_OFF(len) (sizeof(rec_hdr_t) + (uint32_t)(len)) + +/* The CRC runs over header and payload as one contiguous block on both sides, so the payload + * really has to start right behind the header (no padding). Compile-time, gnu99-compatible. */ +typedef char nl_settings_layout_check[offsetof(rec_t, data) == sizeof(rec_hdr_t) ? 1 : -1]; + static uint32_t s_addr[2]; void nl_settings_defaults(nl_settings_t *s) @@ -68,7 +79,7 @@ static bool read_rec(int slot, rec_t *r) nl_settings_defaults(&r->data); if (nl_flash_read(s_addr[slot] + sizeof hdr, &r->data, hdr.length) != 0) return false; - if (nl_flash_read(s_addr[slot] + sizeof hdr + hdr.length, &stored_crc, sizeof stored_crc) != 0) + if (nl_flash_read(s_addr[slot] + NL_SETTINGS_CRC_OFF(hdr.length), &stored_crc, sizeof stored_crc) != 0) return false; crc = nl_crc32(0, &hdr, sizeof hdr); @@ -104,7 +115,7 @@ int nl_settings_save(const nl_settings_t *s) static rec_t a, b, r; /* keep ~700 bytes off the task stack */ bool va = read_rec(0, &a); bool vb = read_rec(1, &b); - uint32_t seq; + uint32_t seq, crc, crc_off; int slot; if (va && vb) { @@ -127,11 +138,14 @@ int nl_settings_save(const nl_settings_t *s) r.hdr.length = sizeof(nl_settings_t); r.hdr.seq = seq; r.data = *s; - r.crc = nl_crc32(0, &r, offsetof(rec_t, crc)); + /* Write the CRC exactly where read_rec() reads it (see NL_SETTINGS_CRC_OFF). */ + crc_off = NL_SETTINGS_CRC_OFF(r.hdr.length); + crc = nl_crc32(0, &r, crc_off); + memcpy((uint8_t *)&r + crc_off, &crc, sizeof crc); if (nl_flash_erase_sector(s_addr[slot]) != 0) return -1; - return nl_flash_write(s_addr[slot], &r, sizeof r); + return nl_flash_write(s_addr[slot], &r, crc_off + sizeof crc); } int nl_settings_erase(void) diff --git a/test/test_settings.c b/test/test_settings.c index 67a6949..4ccbbfe 100644 --- a/test/test_settings.c +++ b/test/test_settings.c @@ -135,6 +135,30 @@ static void test_ota_pending_default_and_roundtrip(void) TEST_ASSERT_EQUAL_UINT8(1, out.ota_pending); } +/* I6: the writer must place the CRC exactly where the reader looks for it - directly behind + * `length` payload bytes. Both sides only agreed by accident while sizeof(nl_settings_t) is a + * multiple of 4; one extra uint16_t in the struct used to move the written CRC two bytes past + * the read offset, which would have failed every stored record's CRC after an update. This test + * nails the invariant independently of the struct size. */ +static void test_crc_offset_matches_reader(void) +{ + nl_settings_t in = sample("crc"), out; + uint16_t length; + uint32_t crc, stored; + const uint8_t *rec = &fake_flash_mem[NL_FL_SETTINGS_A]; + + TEST_ASSERT_EQUAL_INT(0, nl_settings_save(&in)); + memcpy(&length, rec + 6, 2); /* header.length */ + TEST_ASSERT_EQUAL_UINT16(sizeof(nl_settings_t), length); + + crc = nl_crc32(0, rec, 12u + length); /* header + exactly `length` payload bytes */ + memcpy(&stored, rec + 12u + length, 4); /* where read_rec() takes the CRC from */ + TEST_ASSERT_EQUAL_HEX32(crc, stored); + + TEST_ASSERT_TRUE(nl_settings_load(&out)); /* and the record really does read back */ + TEST_ASSERT_EQUAL_MEMORY(&in, &out, sizeof in); +} + static void test_erase(void) { nl_settings_t s = sample("x"), out; @@ -154,6 +178,7 @@ int main(void) RUN_TEST(test_load_accepts_shorter_record); RUN_TEST(test_load_rejects_longer_record); RUN_TEST(test_ota_pending_default_and_roundtrip); + RUN_TEST(test_crc_offset_matches_reader); RUN_TEST(test_erase); return UNITY_END(); }