diff --git a/src/config/nl_settings.c b/src/config/nl_settings.c index 38138c3..d2947fd 100644 --- a/src/config/nl_settings.c +++ b/src/config/nl_settings.c @@ -44,14 +44,35 @@ void nl_settings_init(uint32_t addr_a, uint32_t addr_b) s_addr[1] = addr_b; } +/* Reads a record and validates it. `r->data` is filled from the on-flash payload, + * zero-extended when an older/shorter layout is found (hdr.length < sizeof(nl_settings_t)): + * the CRC only covers the bytes actually written, so it is checked over the stored length, + * not over the current struct size. A record whose length exceeds the current struct + * (an unknown, newer layout) is rejected outright. */ static bool read_rec(int slot, rec_t *r) { - if (nl_flash_read(s_addr[slot], r, sizeof *r) != 0) + rec_hdr_t hdr; + uint32_t crc, stored_crc; + + if (nl_flash_read(s_addr[slot], &hdr, sizeof hdr) != 0) return false; - if (r->hdr.magic != NL_SETTINGS_MAGIC || r->hdr.version != NL_SETTINGS_VERSION || - r->hdr.length != sizeof(nl_settings_t)) + if (hdr.magic != NL_SETTINGS_MAGIC || hdr.version != NL_SETTINGS_VERSION || + hdr.length > sizeof(nl_settings_t)) return false; - return nl_crc32(0, r, offsetof(rec_t, crc)) == r->crc; + + memset(r, 0, sizeof *r); + r->hdr = hdr; + 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) + return false; + + crc = nl_crc32(0, &hdr, sizeof hdr); + crc = nl_crc32(crc, &r->data, hdr.length); + if (crc != stored_crc) + return false; + r->crc = stored_crc; + return true; } bool nl_settings_load(nl_settings_t *out) @@ -71,6 +92,12 @@ bool nl_settings_load(nl_settings_t *out) out->wifi_pass[NL_PASS_LEN - 1] = '\0'; out->token[NL_TOKEN_LEN - 1] = '\0'; out->name[NL_NAME_LEN - 1] = '\0'; + /* A record so severely truncated that fade_in_ms/fade_out_ms/token all read back as + * zero carries no usable data; treat it as if nothing was stored at all. */ + if (out->fade_in_ms == 0 && out->fade_out_ms == 0 && out->token[0] == '\0') { + nl_settings_defaults(out); + return false; + } return true; } diff --git a/src/config/nl_settings.h b/src/config/nl_settings.h index 7c58b5e..5b5f5ba 100644 --- a/src/config/nl_settings.h +++ b/src/config/nl_settings.h @@ -18,6 +18,7 @@ typedef struct { uint8_t power_on_behaviour; /* nl_pob_t */ uint16_t fade_in_ms; uint16_t fade_out_ms; + uint8_t ota_pending; /* set before an OTA; the next boot restores the last state */ } nl_settings_t; void nl_settings_defaults(nl_settings_t *s); diff --git a/test/test_settings.c b/test/test_settings.c index 7ccab69..e596e41 100644 --- a/test/test_settings.c +++ b/test/test_settings.c @@ -4,6 +4,7 @@ #include "config/nl_flash_layout.h" #include "config/nl_settings.h" #include "light/nl_light_types.h" +#include "util/nl_crc.h" void setUp(void) { @@ -84,6 +85,51 @@ static void test_corrupt_newest_falls_back(void) TEST_ASSERT_EQUAL_STRING("first", out.name); } +static void test_load_accepts_shorter_record(void) +{ + nl_settings_t in, out; + uint32_t crc; + /* simulate a record written by an older firmware: length = sizeof - 4, payload truncated */ + in = sample("alt"); + nl_settings_save(&in); + { + uint16_t shorter = (uint16_t)(sizeof(nl_settings_t) - 4); + memcpy(&fake_flash_mem[NL_FL_SETTINGS_A + 6], &shorter, 2); /* header.length */ + crc = nl_crc32(0, &fake_flash_mem[NL_FL_SETTINGS_A], 12 + shorter); + memcpy(&fake_flash_mem[NL_FL_SETTINGS_A + 12 + shorter], &crc, 4); + } + TEST_ASSERT_TRUE(nl_settings_load(&out)); + TEST_ASSERT_EQUAL_STRING("alt", out.name); + TEST_ASSERT_EQUAL_STRING("ninHome", out.wifi_ssid); +} + +static void test_load_rejects_longer_record(void) +{ + nl_settings_t in = sample("x"), out; + uint16_t longer = (uint16_t)(sizeof(nl_settings_t) + 1); + uint32_t crc; + nl_settings_save(&in); + memcpy(&fake_flash_mem[NL_FL_SETTINGS_A + 6], &longer, 2); /* header.length */ + /* recompute the CRC over the (bogus, longer) length so the length check itself is + * what rejects the record, not a CRC mismatch left over from the original length */ + crc = nl_crc32(0, &fake_flash_mem[NL_FL_SETTINGS_A], 12 + longer); + memcpy(&fake_flash_mem[NL_FL_SETTINGS_A + 12 + longer], &crc, 4); + TEST_ASSERT_FALSE(nl_settings_load(&out)); /* unknown newer layout -> defaults */ +} + +static void test_ota_pending_default_and_roundtrip(void) +{ + nl_settings_t in, out; + TEST_ASSERT_FALSE(nl_settings_load(&out)); + TEST_ASSERT_EQUAL_UINT8(0, out.ota_pending); + + in = sample("ota"); + in.ota_pending = 1; + TEST_ASSERT_EQUAL_INT(0, nl_settings_save(&in)); + TEST_ASSERT_TRUE(nl_settings_load(&out)); + TEST_ASSERT_EQUAL_UINT8(1, out.ota_pending); +} + static void test_erase(void) { nl_settings_t s = sample("x"), out; @@ -100,6 +146,9 @@ int main(void) RUN_TEST(test_alternates_slots); RUN_TEST(test_power_loss_keeps_previous); RUN_TEST(test_corrupt_newest_falls_back); + 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_erase); return UNITY_END(); }