fix: CRC im Settings-Record dort schreiben, wo der Leser sie erwartet
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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user