Repository navigation
cli: reject a negative sensor list start index - #100
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
No unresolved review issues remain.
Review effort: Lite
Findings: None
What changed in this PR
This pull request hardens sensor list against invalid indices, unsafe NULL formatting, and oversized replies.
Changes:
- Safely parses and bounds start indices.
- Handles NULL sensor entries and bounds pagination.
- Documents paging and out-of-range behavior.
| File | Summary |
|---|---|
zephcore/helpers/CommonCLI.cpp |
Implements safe parsing, NULL handling, and bounded paging. |
docs/Repeater_CLI_commands.md |
Documents updated sensor-list behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Owner
|
isn't this a little over-engineered? if (start < 0 || start >= end) { |
ptr727
added a commit
to ptr727/liquidraver-ZephCore
that referenced
this pull request
Oct 5, 2026
The owner asked whether the fix was over-engineered, on liquidraver#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 <noreply@anthropic.com>
`sensor list <start>` parsed start with _atoi(), whose uint32_t goes negative as an int: a start of 4294967295 is -1 and 2147483648 is INT_MIN. Both passed the `start >= end` guard, so the loop asked the sensor manager for a negative index and passed the NULL it returned to "%s=%s", undefined behaviour that picolibc happens to print as "(null)". Rejecting a negative start closes it: ZephyrSensorManager has one setting, so only index 0 is ever requested. The same defect as meshcore-dev/MeshCore#3433. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ptr727
force-pushed
the
sensor-list-bounds
branch
from
October 5, 2026 19:14
426dc96 to
8190b40
Compare
ptr727
changed the title
sensor list's start index and its pagessensor list start index
Contributor
Author
|
Fair point, yes. The rest was hardening for a sensor manager with more or longer settings. One latent issue worth knowing about: the page loop bounds Cut down to your line in 8190b40, and the description is updated to match. |
This was referenced Oct 5, 2026
ptr727
added a commit
to ptr727/meshcore-dev-MeshCore
that referenced
this pull request
Oct 5, 2026
`sensor list <n>` passes two NULL pointers to sprintf("%s=%s\n", ...)
for any n in [2147483648, 4294967295]. On the C libraries these targets
link against, that faults rather than printing "(null)".
_atoi() returns uint32_t, and its result goes straight into an int
start, so `sensor list 4294967295` gives start == -1. The guard
`start >= end` can't catch that: -1 >= end is false for every setting
count, end == 0 included. The loop then runs from a negative index,
where getSettingName() and getSettingValue() return NULL.
The branch sits behind no #if, and the base SensorManager accessors
always return NULL, so a plain repeater with no sensors is affected
too. The command also works over remote admin. The effect depends on
the target: an ESP32-S3 panics (LoadProhibited at address 0) and
reboots, while an nRF52840 hangs until someone resets it by hand.
Add `start < 0` to the guard, so a wrapped start gets the existing
"no custom var" reply. This is the same line as liquidraver/ZephCore#100.
Every sensor manager on dev exposes at most one setting, `gps`, so the
paging loop's fixed 134-byte bound can't overrun today. That bound is
left as it is.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ptr727
added a commit
to ptr727/meshcore-dev-MeshCore
that referenced
this pull request
Oct 5, 2026
Every sensor manager on dev exposes at most one setting, so the paging bound and the start-index clamp guard nothing reachable. Restore dev's parser, paging loop and docs, and keep only the check that stops a wrapped negative start reaching the accessors. Matches liquidraver/ZephCore#100 as merged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 5, 2026
ptr727
added a commit
to ptr727/liquidraver-ZephCore
that referenced
this pull request
Oct 6, 2026
dev brought liquidraver#99's RTC identification (rtc_identify() and its verdicts), liquidraver#98's RAK12002 and RV3028 EEPROM configuration, liquidraver#100, and the rename of docs/Repeater_CLI_commands.md to docs/CLI_commands.md. The hw docs moved with the rename without conflict. ZephyrRTCDiscover.c conflicted where hw recorded a report state at the inline reads liquidraver#99 replaced. dev's logic is taken whole, and each verdict now maps to a report state: an address NACK or RTC_NOT_THIS is absent, RTC_FOUND and RTC_FOUND_GARBLED are present. rtc_identify() gains RTC_UNREAD for a bus that is not ready or a read failing other than with -EIO, which keeps hw's "unprobed" for those; discovery skips it exactly as it skips RTC_ABSENT. liquidraver#99's all-0xFF skip is neither absent nor present, so it gets its own state, rendered "all 0xff". It counts with unprobed toward the summary, now worded "unsettled", since neither settles whether an RTC is fitted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The MeshCore counterpart is meshcore-dev/MeshCore#3433.
The defect
sensor list <start>parsesstartwith_atoi(), which returns auint32_t, into anint. A start of 4294967295 becomes -1, and 2147483648 becomes INT_MIN. Both pass thestart >= endguard, so the loop asks the sensor manager for a negative index, gets NULL back, and passes it to"%s=%s\n". That is undefined behaviour; picolibc happens to print(null).sensor listis an admin command, so a remote admin can reach this.The fix
if (start < 0 || start >= end), as suggested in review.ZephyrSensorManagerhas one setting,gps, so only index 0 is ever requested and no NULL reaches%s.Not changed: the page loop bounds
snprintfwithCLI_REPLY_SIZE(256), while a remote reply has 161 bytes. A long setting near the end of a page could overrun it, but no sensor manager here has one.Testing
rak4631repeater, companion and room server build with no compiler warnings.Fork issue: ptr727#43. Iteration history and review: ptr727#44.
🤖 Generated with Claude Code