fix(companion): channel save/lookup hardening + trail count guard

Address the open items from the FEATURES.md audit backlog:
- saveChannels() now skips unused slots (all-zero secret) instead of
  writing all 40 every time, so /channels2 holds only configured channels
  (was always ~2.7 KB) and wears the flash less. loadChannels() already
  compacted empties on read, so the loaded result is unchanged.
- findChannelIdx() returns -1 for an all-zero secret, so a corrupted/empty
  channel can't match an unused all-zero slot and misroute messages.
- TrailStore gains a static_assert that CAPACITY fits the uint16_t save
  header count, failing the build instead of silently truncating.

Also re-classify two audit items verified to be non-issues in current
code: the bot strstr "199-char" truncation (BOT_SCRATCH=200 >= MAX_TEXT_LEN
=160, unreachable) and the MSG_PICK reply title (rlen clamped to 20, so
title[24] never overflows).

Verified: WioTrackerL1_companion_solo_dual builds clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
MarekZegare4
2026-06-21 23:25:11 +02:00
parent a2af1a7409
commit 29deaaad44
4 changed files with 32 additions and 23 deletions

View File

@@ -768,23 +768,17 @@ Defensive `if (ch_idx >= MAX_GROUP_CHANNELS) return;` at function entry — prev
## High ## High
### 📋 `findChannelIdx` scans all-zero secret in uninitialised slots ### `findChannelIdx` scans all-zero secret in uninitialised slots
[`BaseChatMesh.cpp:892-897`](src/helpers/BaseChatMesh.cpp#L892-L897) [`BaseChatMesh.cpp:908`](src/helpers/BaseChatMesh.cpp#L908)
```cpp Fixed (local override): `findChannelIdx()` now returns `-1` immediately when the queried secret is all-zero, so a corrupted/empty channel can't match an unused all-zero slot. Complements the load-side skip already in `loadChannels()`.
for (int i = 0; i < MAX_GROUP_CHANNELS; i++) { // full range, not num_channels
if (memcmp(ch.secret, channels[i].channel.secret, 32) == 0) return i;
}
```
If `ch.secret` is all-zero (uninitialised or corrupted) and an unused slot is also all-zero, the function returns that unused slot as a "match". Upstream code — needs upstream fix or local override. ### ✅ `saveChannels` writes all 40 slots to `/channels2`
### 📋 `saveChannels` writes all 40 slots to `/channels2` [`DataStore.cpp:687`](examples/companion_radio/DataStore.cpp#L687)
[`DataStore.cpp:563-580`](examples/companion_radio/DataStore.cpp#L563-L580) + [`BaseChatMesh.cpp:871-877`](src/helpers/BaseChatMesh.cpp#L871-L877) Fixed: the save loop now skips unused slots (all-zero secret) instead of writing every slot up to `MAX_GROUP_CHANNELS`, so the file holds only the channels actually configured (was always ~2.7 KB). `loadChannels()` already compacted empty entries on read, so the loaded result is unchanged — only on-flash size and write wear drop.
`saveChannels` calls `getChannelForSave(idx, ch)` in a loop; `BaseChatMesh::getChannel(idx)` returns `true` for any `idx < MAX_GROUP_CHANNELS` (including uninitialised slots). Result: file is always ~2.7 KB (40 × 68 B). The `loadChannels` sanity check (skip empty secret) papers over the symptom but the root cause is in save.
### 📋 `msgRead(0)` wipes the whole DM unread table ### 📋 `msgRead(0)` wipes the whole DM unread table
@@ -862,17 +856,17 @@ Point (0, 0) is a legitimate location (Gulf of Guinea). Corner case but a logic
Scan detail view (`SNR: %.1f dB`, `Rem: %.1f dB`) and the ping popup keep the 0.25 dB resolution. (After the one-list refactor the scan list cards show **RSSI** in the right column, not SNR.) Scan detail view (`SNR: %.1f dB`, `Rem: %.1f dB`) and the ping popup keep the 0.25 dB resolution. (After the one-list refactor the scan list cards show **RSSI** in the right column, not SNR.)
### 📋 Trail `_count` cast to `uint16_t` ### Trail `_count` cast to `uint16_t`
[`Trail.h:176`](examples/companion_radio/Trail.h#L176) [`Trail.h:27`](examples/companion_radio/Trail.h#L27)
Safe today (CAPACITY=512 fits), fragile if CAPACITY grows past 65535. A `static_assert(CAPACITY <= 0xFFFF, …)` next to the CAPACITY definition now fails the build if it is ever grown past what the uint16_t save-header count can hold, instead of silently truncating. Safe today (CAPACITY=512).
### 📋 Bot `strstr` on truncated 199-char buffer ### Bot `strstr` on truncated 199-char buffer — not reachable
[`MyMeshBot.h:22-25`](examples/companion_radio/MyMeshBot.h#L22-L25) [`MyMeshBot.h:11`](examples/companion_radio/MyMeshBot.h#L11)
Trigger word near the end of a >199-character message will not match. Matching is now centralised in `botTriggerMatches()` (single `BOT_SCRATCH`-sized buffer), so a fix would land in one place. Re-checked: `BOT_SCRATCH` is 200 and `MAX_TEXT_LEN` is 160, so an incoming message never reaches the 199-char truncation point — the scratch buffer (used by the centralised `botTriggerMatches()`) always holds the whole message. No fix needed.
### ✅ `strncpy("?", buf, sizeof(buf))` replaced with `strcpy` ### ✅ `strncpy("?", buf, sizeof(buf))` replaced with `strcpy`
@@ -880,11 +874,11 @@ Trigger word near the end of a >199-character message will not match. Matching i
Two fallback "?" sender names now use a plain `strcpy` so we don't memset 21 unused bytes for a one-character string. Two fallback "?" sender names now use a plain `strcpy` so we don't memset 21 unused bytes for a one-character string.
### 📋 Title truncation in MSG_PICK reply mode ### Title truncation in MSG_PICK reply mode — not an overflow
[`QuickMsgScreen.h:937`](examples/companion_radio/ui-new/QuickMsgScreen.h#L937) [`QuickMsgScreen.h:1279-1287`](examples/companion_radio/ui-new/QuickMsgScreen.h#L1279)
`char title[24]` for `"RE:" + nick[32]` truncates long nicks silently. Re-checked: `rlen` is clamped to 20 before building `"RE:" + nick`, so the title is ≤23 chars and fits `title[24]` with no overflow. A nick longer than 20 chars is shown truncated, but that's an intentional fit-to-header limit (the OLED header only fits ~21 chars anyway), not a bug.
## Priority for merge ## Priority for merge

View File

@@ -685,12 +685,19 @@ void DataStore::saveChannels(DataStoreHost* host) {
memset(unused, 0, 4); memset(unused, 0, 4);
while (host->getChannelForSave(channel_idx, ch)) { while (host->getChannelForSave(channel_idx, ch)) {
channel_idx++;
// getChannelForSave() returns every slot up to MAX_GROUP_CHANNELS, so skip
// the unused ones (all-zero secret) rather than writing all 40 — otherwise
// the file is always ~2.7 KB and wears the flash needlessly. loadChannels()
// already compacts empty entries on read, so the loaded result is identical.
bool empty = true;
for (int b = 0; b < 32; b++) if (ch.channel.secret[b]) { empty = false; break; }
if (empty) continue;
bool success = (file.write(unused, 4) == 4); bool success = (file.write(unused, 4) == 4);
success = success && (file.write((uint8_t *)ch.name, 32) == 32); success = success && (file.write((uint8_t *)ch.name, 32) == 32);
success = success && (file.write((uint8_t *)ch.channel.secret, 32) == 32); success = success && (file.write((uint8_t *)ch.channel.secret, 32) == 32);
if (!success) break; // write failed if (!success) break; // write failed
channel_idx++;
} }
file.close(); file.close();
} }

View File

@@ -25,6 +25,9 @@ static const uint8_t TRAIL_FLAG_SEG_START = 0x01;
class TrailStore { class TrailStore {
public: public:
static const int CAPACITY = 512; static const int CAPACITY = 512;
// _count is serialised as uint16_t in the save header — fail the build loudly
// if CAPACITY is ever grown past what that can hold, rather than truncating.
static_assert(CAPACITY <= 0xFFFF, "TrailStore::CAPACITY must fit in the uint16_t save-header count");
// Fixed sampling cadence — matches the sensor manager's default GPS update // Fixed sampling cadence — matches the sensor manager's default GPS update
// rate (1 s). Density is controlled by the min-delta gate (settings) rather // rate (1 s). Density is controlled by the min-delta gate (settings) rather

View File

@@ -906,6 +906,11 @@ bool BaseChatMesh::setChannel(int idx, const ChannelDetails& src) {
return false; return false;
} }
int BaseChatMesh::findChannelIdx(const mesh::GroupChannel& ch) { int BaseChatMesh::findChannelIdx(const mesh::GroupChannel& ch) {
// An all-zero secret is never a real channel key; without this guard it would
// match the first uninitialised (all-zero) slot and misroute incoming messages.
bool empty = true;
for (int b = 0; b < (int)sizeof(ch.secret); b++) if (ch.secret[b]) { empty = false; break; }
if (empty) return -1;
for (int i = 0; i < MAX_GROUP_CHANNELS; i++) { for (int i = 0; i < MAX_GROUP_CHANNELS; i++) {
if (memcmp(ch.secret, channels[i].channel.secret, sizeof(ch.secret)) == 0) return i; if (memcmp(ch.secret, channels[i].channel.secret, sizeof(ch.secret)) == 0) return i;
} }