From c96e0442c0a9e5a485c9b939b9867ba0aba37a52 Mon Sep 17 00:00:00 2001 From: Michael Sauer Date: Sat, 12 Sep 2026 01:11:18 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20fehlende=20Felder=20in=20gek=C3=BCrzten?= =?UTF-8?q?=20Settings-Records=20auf=20echte=20Defaults=20setzen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_rec() füllte den Puffer bisher mit memset(0) statt mit nl_settings_defaults() vor, bevor die tatsächlich vorhandenen hdr.length Bytes darübergeschrieben werden. Ein gekürzter Record, dessen Länge vor fade_in_ms/fade_out_ms endet, aber token noch enthält, lief damit mit fade_in_ms=0/fade_out_ms=0 statt der spezifizierten 500 ms - genau der Fall, für den die Längentoleranz gedacht war. Die davor eingebaute "leerer Record" Sonderbehandlung in nl_settings_load() (fade_in_ms==0 && fade_out_ms==0 && token[0]=='\0' -> volle Defaults) ist dadurch überflüssig geworden und entfernt. test_load_accepts_shorter_record kürzt jetzt bis vor fade_in_ms und prüft explizit auf die echten Defaults (500/500/0) statt nur auf Felder vor dem gekürzten Bereich. test_load_rejects_longer_record verzichtet auf die tote CRC-Neuberechnung, da read_rec() zu lange Records schon anhand der Länge verwirft, bevor Payload oder CRC gelesen werden. --- src/config/nl_settings.c | 22 ++++++++++------------ test/test_settings.c | 19 ++++++++++++------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/config/nl_settings.c b/src/config/nl_settings.c index d2947fd..aa41d47 100644 --- a/src/config/nl_settings.c +++ b/src/config/nl_settings.c @@ -44,11 +44,15 @@ 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. */ +/* Reads a record and validates it. `r->data` is pre-filled with nl_settings_defaults() + * and only the bytes actually on flash (hdr.length, possibly less than sizeof(nl_settings_t) + * for a record written by an older firmware) are copied over that. Fields a shorter record + * never had (e.g. fade_in_ms/fade_out_ms/token added later) therefore keep their real + * default (500 ms, NINLIGHT_TOKEN_DEFAULT, ...), not zero - a zeroed fade time or an empty + * token would be a behaviour change, not just "unset". 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, before either payload or CRC is even read. */ static bool read_rec(int slot, rec_t *r) { rec_hdr_t hdr; @@ -60,8 +64,8 @@ static bool read_rec(int slot, rec_t *r) hdr.length > sizeof(nl_settings_t)) return false; - memset(r, 0, sizeof *r); r->hdr = hdr; + 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) @@ -92,12 +96,6 @@ 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/test/test_settings.c b/test/test_settings.c index e596e41..67a6949 100644 --- a/test/test_settings.c +++ b/test/test_settings.c @@ -1,3 +1,4 @@ +#include #include #include "unity.h" #include "fake_flash.h" @@ -89,11 +90,14 @@ 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 */ + /* simulate a record written by a firmware that predates fade_in_ms/fade_out_ms/ + * ota_pending: length ends right before fade_in_ms. Those fields must come back as + * their real defaults (500 ms / 0), not as zero just because the record didn't carry + * them - that's the whole point of the length tolerance. */ in = sample("alt"); nl_settings_save(&in); { - uint16_t shorter = (uint16_t)(sizeof(nl_settings_t) - 4); + uint16_t shorter = (uint16_t)offsetof(nl_settings_t, fade_in_ms); 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); @@ -101,19 +105,20 @@ static void test_load_accepts_shorter_record(void) TEST_ASSERT_TRUE(nl_settings_load(&out)); TEST_ASSERT_EQUAL_STRING("alt", out.name); TEST_ASSERT_EQUAL_STRING("ninHome", out.wifi_ssid); + TEST_ASSERT_EQUAL_UINT16(500, out.fade_in_ms); + TEST_ASSERT_EQUAL_UINT16(500, out.fade_out_ms); + TEST_ASSERT_EQUAL_UINT8(0, out.ota_pending); } 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); + /* read_rec() rejects hdr.length > sizeof(nl_settings_t) outright, before it even reads + * the payload or the CRC - so the stored CRC is irrelevant here and deliberately left + * as whatever nl_settings_save() wrote for the original (shorter) length. */ 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 */ }