Bound the sensor list start index and its paging reserve 🤖🤖

[ptr727]

Summary

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

The failure is worse on nRF52 than on ESP32-S3, and that is the part worth reading first.

• ESP32-S3. Guru Meditation Error: Core 1 panic'ed (LoadProhibited) with
  EXCVADDR: 0x00000000 — a null dereference, the predicted path rather than a coincidence — and
  then a reboot. The node comes back on its own, so the effect is a remote reset, repeatable
  as fast as an admin client can send the command.
• nRF52840. No reboot. The firmware hangs permanently: the USB CDC endpoint stays
  enumerated, but nothing is answered, and the 1200-baud DFU trigger is not serviced either. The
  board had to be physically reset to recover. On this target the bug is not a remote reset, it is
  a remote hard stop that needs someone to walk to the device.

Both observed on hardware, stock dev build against this branch.

The bug

In src/helpers/CommonCLI.cpp:

1. _atoi() returns uint32_t. The result was assigned straight to int start, so
   sensor list 4294967295 gives start == -1.
2. The existing guard is if (start >= end). -1 >= end is false for every possible setting
   count, including end == 0, so the guard never fires on a wrapped value.
3. The loop therefore runs from i == -1, and getSettingName(-1) / getSettingValue(-1) both
   return NULL — EnvironmentSensorManager's only non-NULL return is a guarded
   i == settings++ match, and the base SensorManager accessors return NULL unconditionally.
4. Both NULLs go into the sprintf. NULL into %s is undefined behaviour, and on the C
   libraries these targets link against it faults rather than printing (null).

2147483648 faulting alongside 4294967295 is the boundary: everything at or above 2^31 wraps
negative, so it is most of the argument space rather than one magic value.

Two things about the reach:

• The sensor list branch is not behind any #if, and the base SensorManager is what a
  plain repeater gets. A node with no sensors at all faults on the same input — this is not
  limited to sensor-equipped builds.
• The command arrives over remote admin as well as the serial console
  (examples/simple_repeater/MyMesh.cpp feeds a CLI packet straight into handleCommand), so any
  client that can send a CLI command can trigger it.

Second, latent flaw in the same branch

The loop guard dp-reply < 134 reserves a fixed 26 bytes for the ... next:N marker and so
assumes a maximum row width. A row that starts at byte 133 can run past the end of the reply
buffer. Not reachable with today's setting names — gps=1 is 6 bytes — but it is a fixed reserve
standing in for a bound.

Both flaws are in the one else if, so they are fixed together.

How big is the reply buffer, actually

This matters for the second flaw and it is not 160.

• The serial CLI passes char reply[160] (examples/*/main.cpp).
• Remote admin passes 161 — the reply is built at offset 5 of a uint8_t[166].
• But every wrapper that reaches CommonCLI::handleCommand() — simple_repeater,
  simple_room_server and simple_sensor — first reflects an optional 3-byte xx|
  companion-radio prefix back into the reply and does reply += 3 before delegating. So the
  pointer this function receives can be 3 bytes short of the buffer it was cut from.

The smallest usable size is therefore 157, and that is what CLI_REPLY_MAX is set to.
(companion_radio strips the same prefix but has its own handleCommand and never reaches this
one.) The old dp-reply < 134 was conservative enough that the prefix never mattered; an exact
bound is not, so it has to be accounted for.

The fix

• parseStartIndex() parses the paging argument and clamps it to the setting count, so an
  out-of-range start answers through the ordinary no custom var path instead of wrapping
  negative. It walks the digits itself rather than clamping _atoi()'s result, because _atoi()
  wraps its own uint32_t on a long enough digit string — 4294967296 comes back as 0, and a
  clamp applied afterwards cannot see that the wrap happened. The comparison is written as
  value > limit/10 || (value == limit/10 && digit > limit%10) so neither side can overflow.
  _atoi() itself is left alone: it is shared with the timestamp and interval commands, and
  narrowing it under them belongs in its own change.
• The accessors are null-checked before formatting. With the clamp in place this is unreachable,
  and it is kept as defence in depth: it is the check that actually stops a NULL reaching %s
  if an index guard is ever bypassed again, and it costs two lines. Happy to drop it if you would
  rather the fix stay strictly to the index.
• Each row is measured against the remaining space before it is written, holding back room for
  the continuation marker, and the writes are snprintf-bounded. The first row of a page is never
  held back entirely, because that would emit ... next:<start> and never advance; an over-long
  first row is written truncated into what is left instead.
• CLI_REPLY_MAX (157) and CLI_MARKER_RESERVE (20) replace the bare 134, with the derivation
  in a comment. handleCommand() is not told its buffer size, so the smallest size any caller
  passes has to be written down somewhere; this makes the existing assumption explicit rather than
  adding a new one.
• docs/cli_commands.md documented only the row format. It now describes the reply shape: the
  <count> vars header, the ... next:<index> continuation marker, and the bare no custom var
  that an out-of-range start returns.

Verification

Reproduced and checked off-target first, with the parsing and paging logic transcribed into a host
program and run against a stand-in SensorManager, comparing the current and proposed logic over
the same inputs. The reply lives in a sentinel-filled arena so the "bytes written" column below is
measured rather than reasoned about. Reconstructed values throughout.

Start argument, one setting, 157-byte reply:

command	before	after
sensor list	lists the settings	unchanged
sensor list 0	lists the settings	unchanged
sensor list 1	no custom var	unchanged
sensor list 2	no custom var	unchanged
sensor list 2147483648	NULL into %s	no custom var
sensor list 4294967295	NULL into %s	no custom var
sensor list 4294967296	lists from index 0	no custom var
sensor list 18446744073709551616	lists from index 0	no custom var
sensor list 99999999999	no custom var	unchanged
sensor list -1	lists from index 0	unchanged
sensor list abc	lists from index 0	unchanged

With the base SensorManager (zero settings, i.e. a plain repeater), sensor list 4294967295 and
sensor list 2147483648 both reach the NULL before the fix and both return no custom var
after it.

Row width, eight settings, first page, 157-byte reply — bytes actually written:

row width	before	after
10 B	88 B	88 B
22 B	150 B	128 B
24 B	162 B — overruns	138 B
30 B	168 B — overruns	138 B
40 B	178 B — overruns	138 B
60 B	198 B — overruns	138 B

Sweeping every row width from 10 B to 60 B, the worst case after the fix is 152 B at width 18,
inside 157. Paging runs to completion for eight 40-byte rows — ... next:3 → ... next:6 → final
page — with no page repeating its own start.

On hardware

Two boards, an ESP32-S3 and an nRF52840, each flashed first with a stock build of this branch's
base and then with the branch, driven over the serial CLI. Every case agrees with the off-target
harness on both targets.

command	stock base	this branch
sensor list	lists the settings	unchanged
sensor list 0	lists the settings	unchanged
sensor list 1	no custom var	unchanged
sensor list 2	no custom var	unchanged
sensor list 2147483648	panic (ESP32-S3) / hang (nRF52840)	no custom var
sensor list 4294967295	panic (ESP32-S3) / hang (nRF52840)	no custom var
sensor list 4294967296	lists from index 0	no custom var
sensor list 18446744073709551616	lists from index 0	no custom var
sensor list 99999999999	no custom var	unchanged
sensor list -1	lists the settings	unchanged
sensor list abc	lists the settings	unchanged

Both boards happen to detect a GPS, so they exercise the one-setting path. The zero-setting path —
a plain repeater, where getNumSettings() is 0 and -1 >= 0 is still false — is covered
off-target only; it is the same branch with the same accessors, and the base SensorManager
returns NULL unconditionally rather than conditionally.

Builds: RAK_4631_repeater, heltec_v4_r8_repeater and Heltec_v2_repeater all succeed on this
branch's base.

[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