fix: fehlende Felder in gekürzten Settings-Records auf echte Defaults setzen
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.
This commit is contained in:
+10
-12
@@ -44,11 +44,15 @@ void nl_settings_init(uint32_t addr_a, uint32_t addr_b)
|
|||||||
s_addr[1] = addr_b;
|
s_addr[1] = addr_b;
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Reads a record and validates it. `r->data` is filled from the on-flash payload,
|
/* Reads a record and validates it. `r->data` is pre-filled with nl_settings_defaults()
|
||||||
* zero-extended when an older/shorter layout is found (hdr.length < sizeof(nl_settings_t)):
|
* and only the bytes actually on flash (hdr.length, possibly less than sizeof(nl_settings_t)
|
||||||
* the CRC only covers the bytes actually written, so it is checked over the stored length,
|
* for a record written by an older firmware) are copied over that. Fields a shorter record
|
||||||
* not over the current struct size. A record whose length exceeds the current struct
|
* never had (e.g. fade_in_ms/fade_out_ms/token added later) therefore keep their real
|
||||||
* (an unknown, newer layout) is rejected outright. */
|
* 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)
|
static bool read_rec(int slot, rec_t *r)
|
||||||
{
|
{
|
||||||
rec_hdr_t hdr;
|
rec_hdr_t hdr;
|
||||||
@@ -60,8 +64,8 @@ static bool read_rec(int slot, rec_t *r)
|
|||||||
hdr.length > sizeof(nl_settings_t))
|
hdr.length > sizeof(nl_settings_t))
|
||||||
return false;
|
return false;
|
||||||
|
|
||||||
memset(r, 0, sizeof *r);
|
|
||||||
r->hdr = hdr;
|
r->hdr = hdr;
|
||||||
|
nl_settings_defaults(&r->data);
|
||||||
if (nl_flash_read(s_addr[slot] + sizeof hdr, &r->data, hdr.length) != 0)
|
if (nl_flash_read(s_addr[slot] + sizeof hdr, &r->data, hdr.length) != 0)
|
||||||
return false;
|
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] + 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->wifi_pass[NL_PASS_LEN - 1] = '\0';
|
||||||
out->token[NL_TOKEN_LEN - 1] = '\0';
|
out->token[NL_TOKEN_LEN - 1] = '\0';
|
||||||
out->name[NL_NAME_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;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+12
-7
@@ -1,3 +1,4 @@
|
|||||||
|
#include <stddef.h>
|
||||||
#include <string.h>
|
#include <string.h>
|
||||||
#include "unity.h"
|
#include "unity.h"
|
||||||
#include "fake_flash.h"
|
#include "fake_flash.h"
|
||||||
@@ -89,11 +90,14 @@ static void test_load_accepts_shorter_record(void)
|
|||||||
{
|
{
|
||||||
nl_settings_t in, out;
|
nl_settings_t in, out;
|
||||||
uint32_t crc;
|
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");
|
in = sample("alt");
|
||||||
nl_settings_save(&in);
|
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 */
|
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);
|
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);
|
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_TRUE(nl_settings_load(&out));
|
||||||
TEST_ASSERT_EQUAL_STRING("alt", out.name);
|
TEST_ASSERT_EQUAL_STRING("alt", out.name);
|
||||||
TEST_ASSERT_EQUAL_STRING("ninHome", out.wifi_ssid);
|
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)
|
static void test_load_rejects_longer_record(void)
|
||||||
{
|
{
|
||||||
nl_settings_t in = sample("x"), out;
|
nl_settings_t in = sample("x"), out;
|
||||||
uint16_t longer = (uint16_t)(sizeof(nl_settings_t) + 1);
|
uint16_t longer = (uint16_t)(sizeof(nl_settings_t) + 1);
|
||||||
uint32_t crc;
|
|
||||||
nl_settings_save(&in);
|
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 */
|
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 */
|
TEST_ASSERT_FALSE(nl_settings_load(&out)); /* unknown newer layout -> defaults */
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user