From 29e7be7f8e01cc9ec52e3c4d1e578487263d3ec9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 5 Oct 2026 08:31:39 -0700 Subject: [PATCH 1/3] cli: bound `sensor list`'s start index and its pages `sensor list ` parsed start with _atoi(), whose uint32_t goes negative as an int: a start of 4294967295 is -1 and 2147483648 is INT_MIN, both pass the `start >= end` guard, and the loop asks the sensor manager for negative indexes. ZephyrSensorManager returns NULL for those, which went straight into "%s=%s": a remote `sensor list 2147483648` replies "(null)=(null)" ten times and "... next:-2147483638", undefined behaviour that picolibc happens to print. 4294967296 wraps _atoi() itself to 0 and lists from the start. The start is now clamped to the setting count while it is parsed, so any out-of-range start answers "no custom var". A NULL name or value ends the listing. Pages are bounded by replyCap(), the remote buffer less the caller's header, instead of a fixed 134 inside a 256-byte bound, and a page with rows after it keeps room for its "... next:N" marker; a page's first row is written even if truncated, so the marker always advances. The same defect and the same fix as meshcore-dev/MeshCore#3433. Co-Authored-By: Claude Opus 5.5 --- docs/Repeater_CLI_commands.md | 2 +- zephcore/helpers/CommonCLI.cpp | 53 ++++++++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/docs/Repeater_CLI_commands.md b/docs/Repeater_CLI_commands.md index 48bfe58f0..78e82f517 100644 --- a/docs/Repeater_CLI_commands.md +++ b/docs/Repeater_CLI_commands.md @@ -203,7 +203,7 @@ Caveats: the `sys=` tally needs `CONFIG_ZEPHCORE_GPS_SAT_DIAG` (default on for r | Command | Description | |---------|-------------| -| `sensor list []` | List custom sensor settings (paginated at 134 chars) | +| `sensor list []` | List custom sensor settings; a page that does not fit one reply ends with `... next:`, and a start at or past the count replies `no custom var` | | `sensor get ` | Get a custom sensor setting value by key | | `sensor set ` | Set a custom sensor setting value | diff --git a/zephcore/helpers/CommonCLI.cpp b/zephcore/helpers/CommonCLI.cpp index 1a64fd009..7f19a74cd 100644 --- a/zephcore/helpers/CommonCLI.cpp +++ b/zephcore/helpers/CommonCLI.cpp @@ -44,6 +44,24 @@ static uint32_t _atoi(const char* sp) { return n; } +/* A paging start index, clamped to total while it is parsed: _atoi()'s + * uint32_t goes negative as an int, and wraps itself on a long digit string. */ +static int parse_start_index(const char* sp, int total) +{ + uint32_t limit = total > 0 ? (uint32_t)total : 0; + uint32_t value = 0; + + while (*sp >= '0' && *sp <= '9') { + uint32_t digit = (uint32_t)(*sp++ - '0'); + + if (value > limit / 10 || (value == limit / 10 && digit > limit % 10)) { + return total; + } + value = value * 10 + digit; + } + return (int)value; +} + /* ---- "default" keyword + strict numeric parsing for the `set` path ---- * * Bare atoi()/atof() fold every non-numeric string to 0, the word "default" @@ -583,28 +601,45 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch strcpy(reply, "can't find custom var"); } } else if (memcmp(command, "sensor list", 11) == 0) { + /* A page with rows after it keeps room for "... next:N" (9 + 10 + * digits + terminator), or it could not be resumed. */ + const size_t marker = 20; + char* lim = reply + replyCap(sender_timestamp); char* dp = reply; int start = 0; int end = _sensors->getNumSettings(); if (strlen(command) > 11) { - start = _atoi(command + 12); + start = parse_start_index(command + 12, end); } if (start >= end) { strcpy(reply, "no custom var"); } else { - snprintf(dp, CLI_REPLY_SIZE - (dp - reply), "%d vars\n", end); + snprintf(dp, lim - dp, "%d vars\n", end); dp = strchr(dp, 0); int i; - for (i = start; i < end && (dp - reply < 134); i++) { - snprintf(dp, CLI_REPLY_SIZE - (dp - reply), "%s=%s\n", - _sensors->getSettingName(i), - _sensors->getSettingValue(i)); + for (i = start; i < end; i++) { + const char* name = _sensors->getSettingName(i); + const char* value = _sensors->getSettingValue(i); + + if (name == NULL || value == NULL) { + end = i; /* nothing further to list */ + break; + } + size_t space = lim - dp; + size_t avail = (i + 1 < end && space > marker) ? space - marker : space; + + /* A page's first row is written even if truncated, so the + * marker always advances. */ + if (strlen(name) + strlen(value) + 2 >= avail && i > start) { + break; + } + snprintf(dp, avail, "%s=%s\n", name, value); dp = strchr(dp, 0); } if (i < end) { - snprintf(dp, CLI_REPLY_SIZE - (dp - reply), "... next:%d", i); - } else { - *(dp - 1) = 0; // remove last CR + snprintf(dp, lim - dp, "... next:%d", i); + } else if (dp > reply && dp[-1] == '\n') { + dp[-1] = 0; // remove last CR } } } else if (memcmp(command, "gps on", 6) == 0) { From d0fd7d994432a5622216f4a5ef7de03c3b715459 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 5 Oct 2026 08:35:14 -0700 Subject: [PATCH 2/3] cli: cap `sensor list` pages at a remote reply less its prefix Local review: replyCap() is 256 on the local path, but a repeater or room server that strips a typed "xx|" prefix hands handleCommand() a 256-byte buffer 3 bytes in, and a companion frame carries at most 175 bytes of reply. A full 256-byte page could overrun the first by 3 bytes and lose its "... next:N" marker to the second. Pages now stop at CLI_REMOTE_REPLY_SIZE - 3 (158), which fits all three, as MeshCore's fixed 157 does. Neither is reachable with ZephyrSensorManager's single setting. Co-Authored-By: Claude Opus 5.5 --- zephcore/helpers/CommonCLI.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/zephcore/helpers/CommonCLI.cpp b/zephcore/helpers/CommonCLI.cpp index 7f19a74cd..429e3aefa 100644 --- a/zephcore/helpers/CommonCLI.cpp +++ b/zephcore/helpers/CommonCLI.cpp @@ -601,10 +601,14 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch strcpy(reply, "can't find custom var"); } } else if (memcmp(command, "sensor list", 11) == 0) { - /* A page with rows after it keeps room for "... next:N" (9 + 10 - * digits + terminator), or it could not be resumed. */ + /* No page is longer than a remote one less a 3-byte "xx|" prefix, + * which also fits a companion frame and a local buffer the prefix + * took 3 bytes from. A page with rows after it keeps room for + * "... next:N" (9 + 10 digits + terminator), or it could not be + * resumed. */ const size_t marker = 20; - char* lim = reply + replyCap(sender_timestamp); + char* lim = reply + MIN(replyCap(sender_timestamp), + (size_t)CLI_REMOTE_REPLY_SIZE - 3); char* dp = reply; int start = 0; int end = _sensors->getNumSettings(); From 7f4688efc7caf4fc8d5e0a7cd841076832beeefe Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Mon, 5 Oct 2026 12:12:03 -0700 Subject: [PATCH 3/3] cli: reduce the `sensor list` fix to rejecting a negative start The owner asked whether the fix was over-engineered, on liquidraver/ZephCore#100. It was. ZephyrSensorManager has one setting, gps, so the negative start from _atoi()'s uint32_t is the only fault reachable today, and `start < 0 ||` closes it: only index 0 is ever requested, so no NULL reaches "%s". The parser clamp, the page bound and the docs row are reverted to dev. Left as is: the page loop bounds snprintf with CLI_REPLY_SIZE while a remote reply has 161 bytes. A long setting near the end of a page could overrun it, but no such setting exists. Co-Authored-By: Claude Opus 5.5 --- docs/Repeater_CLI_commands.md | 2 +- zephcore/helpers/CommonCLI.cpp | 59 ++++++---------------------------- 2 files changed, 11 insertions(+), 50 deletions(-) diff --git a/docs/Repeater_CLI_commands.md b/docs/Repeater_CLI_commands.md index 78e82f517..48bfe58f0 100644 --- a/docs/Repeater_CLI_commands.md +++ b/docs/Repeater_CLI_commands.md @@ -203,7 +203,7 @@ Caveats: the `sys=` tally needs `CONFIG_ZEPHCORE_GPS_SAT_DIAG` (default on for r | Command | Description | |---------|-------------| -| `sensor list []` | List custom sensor settings; a page that does not fit one reply ends with `... next:`, and a start at or past the count replies `no custom var` | +| `sensor list []` | List custom sensor settings (paginated at 134 chars) | | `sensor get ` | Get a custom sensor setting value by key | | `sensor set ` | Set a custom sensor setting value | diff --git a/zephcore/helpers/CommonCLI.cpp b/zephcore/helpers/CommonCLI.cpp index 429e3aefa..9ea10d610 100644 --- a/zephcore/helpers/CommonCLI.cpp +++ b/zephcore/helpers/CommonCLI.cpp @@ -44,24 +44,6 @@ static uint32_t _atoi(const char* sp) { return n; } -/* A paging start index, clamped to total while it is parsed: _atoi()'s - * uint32_t goes negative as an int, and wraps itself on a long digit string. */ -static int parse_start_index(const char* sp, int total) -{ - uint32_t limit = total > 0 ? (uint32_t)total : 0; - uint32_t value = 0; - - while (*sp >= '0' && *sp <= '9') { - uint32_t digit = (uint32_t)(*sp++ - '0'); - - if (value > limit / 10 || (value == limit / 10 && digit > limit % 10)) { - return total; - } - value = value * 10 + digit; - } - return (int)value; -} - /* ---- "default" keyword + strict numeric parsing for the `set` path ---- * * Bare atoi()/atof() fold every non-numeric string to 0, the word "default" @@ -601,49 +583,28 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, const char* command, ch strcpy(reply, "can't find custom var"); } } else if (memcmp(command, "sensor list", 11) == 0) { - /* No page is longer than a remote one less a 3-byte "xx|" prefix, - * which also fits a companion frame and a local buffer the prefix - * took 3 bytes from. A page with rows after it keeps room for - * "... next:N" (9 + 10 digits + terminator), or it could not be - * resumed. */ - const size_t marker = 20; - char* lim = reply + MIN(replyCap(sender_timestamp), - (size_t)CLI_REMOTE_REPLY_SIZE - 3); char* dp = reply; int start = 0; int end = _sensors->getNumSettings(); if (strlen(command) > 11) { - start = parse_start_index(command + 12, end); + start = _atoi(command + 12); } - if (start >= end) { + if (start < 0 || start >= end) { strcpy(reply, "no custom var"); } else { - snprintf(dp, lim - dp, "%d vars\n", end); + snprintf(dp, CLI_REPLY_SIZE - (dp - reply), "%d vars\n", end); dp = strchr(dp, 0); int i; - for (i = start; i < end; i++) { - const char* name = _sensors->getSettingName(i); - const char* value = _sensors->getSettingValue(i); - - if (name == NULL || value == NULL) { - end = i; /* nothing further to list */ - break; - } - size_t space = lim - dp; - size_t avail = (i + 1 < end && space > marker) ? space - marker : space; - - /* A page's first row is written even if truncated, so the - * marker always advances. */ - if (strlen(name) + strlen(value) + 2 >= avail && i > start) { - break; - } - snprintf(dp, avail, "%s=%s\n", name, value); + for (i = start; i < end && (dp - reply < 134); i++) { + snprintf(dp, CLI_REPLY_SIZE - (dp - reply), "%s=%s\n", + _sensors->getSettingName(i), + _sensors->getSettingValue(i)); dp = strchr(dp, 0); } if (i < end) { - snprintf(dp, lim - dp, "... next:%d", i); - } else if (dp > reply && dp[-1] == '\n') { - dp[-1] = 0; // remove last CR + snprintf(dp, CLI_REPLY_SIZE - (dp - reply), "... next:%d", i); + } else { + *(dp - 1) = 0; // remove last CR } } } else if (memcmp(command, "gps on", 6) == 0) {