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