Reject a negative sensor list start index 🤖🤖

[ptr727]

Summary

sensor list <n> passes two NULL pointers to sprintf("%s=%s\n", ...) for any n in
[2147483648, 4294967295]. The command works over remote admin as well as the serial console,
and the branch sits behind no #if, so a plain repeater with no sensors is affected too. This PR
is a one-line guard.

Scope reduced on 2026-10-05. The earlier revision also bounded the paging loop against the
reply buffer, clamped the start index while parsing it, and documented the reply format. The
ZephCore owner asked whether the same change was over-engineered there, and it was cut down to
this one line in liquidraver/ZephCore#100,
which has since merged. The same argument holds here. Every sensor manager on dev exposes at
most one setting, gps, so the paging bound guards nothing that can currently happen. That part
is left out, and the "Not fixed here" section below explains it.

The bug

Links are to dev at 3e3150c8.

1. _atoi()
   returns uint32_t, and the result
   goes straight into int start. So sensor list 4294967295 gives start == -1, and any value
   from 2^31 up wraps negative.
2. The guard is if (start >= end). -1 >= end is false for every setting count, end == 0
   included, so the guard never fires on a wrapped value.
3. The loop then starts at a negative i. getSettingName(i) and getSettingValue(i) return
   NULL there: the base SensorManager always returns NULL, and every override returns a
   name only for a matching index.
4. Both NULLs go into %s, which is undefined behaviour. On the C libraries these targets link
   against, it faults rather than printing (null).

What that does on hardware (stock build of the earlier base cdd04077, serial CLI):

• ESP32-S3: Guru Meditation Error: Core 1 panic'ed (LoadProhibited), EXCVADDR: 0x00000000,
  then a reboot. Any admin client can repeat that remote reset as fast as it can send the command.
• nRF52840: no reboot. The firmware hangs permanently. USB CDC stays enumerated but answers
  nothing, and the 1200-baud DFU touch isn't serviced either. Someone has to reset the board by
  hand.

The fix

if (start < 0 || start >= end).
A wrapped start now answers no custom var, the existing reply for a start past the end. It is the
same line as liquidraver/ZephCore#100.

Not fixed here

• The paging bound. The loop guard dp-reply < 134 reserves a fixed 26 bytes for the
  ... next:N marker. A long row that starts near byte 133 could run past the reply buffer,
  which is 157 bytes once a wrapper has stripped its 3-byte xx| prefix. The longest row today is
  gps=1, so nothing reaches that. If a sensor manager ever lists longer settings, the bound will
  need the change the earlier revision of this PR had.
• A digit string past 2^32 wraps inside _atoi() itself, so sensor list 4294967296 lists
  from index 0. That's harmless (it returns the first page), and _atoi() is shared with the
  timestamp commands, so it is left alone.

Verification

Build	Board	Commands	Result
stock, on the earlier base cdd04077	ESP32-S3, nRF52840 (GPS detected, one setting)	sensor list 2147483648, sensor list 4294967295	panic (ESP32-S3), hang (nRF52840)
this PR, built from ec05125f (same tree as bbacf47a)	nRF52840, RAK4631 (GPS not detected at boot, zero settings)	sensor list 2147483648, 4294967295	no custom var, and the board kept answering
same	same	sensor list, 0, 1, 2, 4294967296, 18446744073709551616, 99999999999, -1, abc	no custom var for all of them, as on dev with zero settings

The board runs a simple_repeater build, driven over the serial CLI. On the boots tested, its GPS
sent nothing during boot detection, so these runs cover the zero-setting path, which is what a
plain repeater has. The negative-start guard doesn't depend on the setting count. The
one-setting listing itself is unchanged from dev.

Builds: RAK_4631_repeater and heltec_v4_repeater succeed.

Iteration history, including the earlier revision's off-target harness and hardware tables:
ptr727/meshcore-dev-MeshCore#8.

[Copilot]

🟡 Changes recommended

CLI_REPLY_MAX currently ignores the 3-byte reply-prefix reservation done by some callers before delegating to CommonCLI::handleCommand(), which can reintroduce a small out-of-bounds write.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens the sensor list [start] CLI command against integer wraparound and reply-buffer overruns that can lead to crashes/reboots (ESP32-S3) or hangs (nRF52), including when triggered via remote admin. It adds bounded parsing for the paging start index, switches the listing output to bounded writes, and documents the paging reply shape.

Changes:

• Add parseStartIndex() to clamp start during parsing (avoids _atoi() wrap and negative int start values).
• Make sensor list output safer via snprintf, pre-flight space checks per row, and NULL-guarding accessor results.
• Update CLI documentation to describe the header/continuation marker behavior and the out-of-range reply.

File summaries

File	Description
src/helpers/CommonCLI.cpp	Adds bounded parsing and safer paging/formatting for sensor list, plus explicit reply-size assumptions.
docs/cli_commands.md	Documents sensor list reply shape, paging marker, and out-of-range behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

The changes directly address the described crash/hang path by preventing index wrap, bounding formatting/writes, and documenting the resulting output behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Note

Copilot was unable to run its full agentic suite in this review.

Copilot review overview

Review effort: Lite
Findings: 1

Open (1)

• The start < 0 check is only meaningful if start is a signed type. Since _atoi() returns… · New