Add sticky overrun tracking to sbuf_t for MSP variable-layout reads

[sensei-hacker]

Summary

Third and final piece of the MSP payload bounds hardening effort tracked under #11672. MSP2_INAV_SET_AUX_RC and MSP_OSD_CHAR_WRITE have runtime-determined payload layouts (length depends on a resolution mode / addressing mode encoded in the payload itself), so they can't use the compile-time struct pattern applied to the fixed-layout SET handlers in the prior PR. Both are already correctly bounded today by their own dataSize checks (confirmed by an earlier audit), so this is defense-in-depth, not a bug fix.

Changes

• Add a sticky overrun bool to sbuf_t (common/streambuf.h), appended as the trailing field so ptr stays first (sbuf_t* is used interchangeably with uint8_t** elsewhere in the tree).
• sbufReadU8 now checks buffer bounds before dereferencing; on an out-of-bounds read it sets overrun and returns 0 instead of reading past end. sbufReadU16/sbufReadU32 are built on sbufReadU8 and inherit the check automatically. sbufReadI8 was rewritten to delegate to sbufReadU8 (was a separate, unchecked implementation) — same result, one bounds check instead of two.
• sbufInit clears overrun on construction; sbufSwitchToReader clears it when a write buffer flips to read mode.
• MSP2_INAV_SET_AUX_RC and MSP_OSD_CHAR_WRITE each check src->overrun once after their read sequence and reject the command (MSP_RESULT_ERROR) if it tripped.
• sbufReadData/sbufReadDataSafe are deliberately untouched — out of scope, neither handler calls them, and sbufReadDataSafe already has its own independent length check.

This keeps every existing call site's shape unchanged (field = sbufReadU8(src);), avoiding a wider mechanical rewrite across the ~40 other sbufRead* call sites in the tree, while giving these two variable-layout handlers the same guarantee the fixed-layout ones already have.

Testing

• Clean SITL build, no new warnings.
• SITL round-trip test covering MSP2_INAV_SET_AUX_RC: all 4 resolution modes (2/4/8/16-bit) verified against baseline behavior (well-formed frames apply the expected channel values, including the raw=0 skip semantics and 16-bit clamping), plus truncated/edge-case frames (all cleanly rejected by the handler's existing dataSize checks; FC stayed responsive, no crash/hang). Confirmed it isn't possible to construct a frame that passes the handler's own bounds checks yet still trips overrun — this is genuinely defense-in-depth, matching the prior audit's conclusion.
• MSP_OSD_CHAR_WRITE exercised end-to-end in SITL (no OSD hardware needed — osdGetDisplayPort() safely returns NULL when nothing is attached) and passed.
• Audited all 16 sbuf_t construction sites tree-wide: 3 use designated initializers (implicitly zero-init overrun per C99), 4 go through sbufInit (now explicit), and the remaining 9 are write-only telemetry/OSD/RC-device frame builders whose overrun field is never read.
• Independent code review: approve.

Addresses #11672.

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Add sticky stream-buffer overrun protection for variable MSP reads

✨ Enhancement 🕐 20-40 Minutes

AI Description

• Track sticky read overruns without changing existing stream-buffer call sites.
• Reject truncated variable-layout AUX RC and OSD character payloads after decoding.
• Reset overrun state when buffers initialize or switch into reader mode.

Diagram

graph TD
    A["MSP Payload"] --> B["MSP Dispatcher"] --> C["Read Values"] --> D{"Overrun Set?"}
    D -- "Yes" --> E["MSP Error"]
    D -- "No" --> F["Apply AUX or OSD"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use safe read APIs in each handler

• ➕ Makes every potentially failing read explicit at its call site.
• ➕ Avoids adding state to sbuf_t.
• ➖ Requires repetitive branching throughout variable decoding loops.
• ➖ Encourages a wider mechanical rewrite and risks missed return-value checks.

2. Rely only on payload-size prevalidation

• ➕ Preserves the existing stream-buffer representation and read behavior.
• ➕ Avoids per-read bounds checks.
• ➖ Keeps byte-count calculations manually coupled to evolving decode logic.
• ➖ Provides no safety net if future layout changes desynchronize validation and reads.

Recommendation: Keep the sticky overrun approach: it adds centralized defense-in-depth while preserving established read expressions and pointer-first sbuf_t compatibility. Explicit safe reads are preferable when callers need immediate failure handling, but a single post-decode check is better suited to these loop-driven variable layouts.

Files changed (3) +17 / -1

Enhancement (3) +17 / -1

streambuf.cBound scalar reads and manage sticky overrun state +7/-1

Bound scalar reads and manage sticky overrun state

• Initializes and resets the new overrun flag when buffers are constructed or switched to reader mode. Bounds-checks byte reads, returns zero without advancing on exhaustion, and routes signed byte reads through the checked primitive so U16 and U32 reads inherit protection.

src/main/common/streambuf.c

streambuf.hAdd sticky overrun state to sbuf_t +1/-0

Add sticky overrun state to sbuf_t

• Appends an overrun boolean to the stream-buffer structure while preserving the pointer as its first field for existing pointer-aliasing usage. The field records scalar reads attempted past the configured end.

src/main/common/streambuf.h

fc_msp.cReject variable-layout MSP commands after read overruns +9/-0

Reject variable-layout MSP commands after read overruns

• Checks sticky stream-buffer status after decoding MSP2_INAV_SET_AUX_RC and MSP_OSD_CHAR_WRITE payloads. Either handler now returns MSP_RESULT_ERROR before completing downstream processing when its scalar read sequence exceeds the payload.

src/main/fc/fc_msp.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[github-actions]

RAM / Flash usage vs. base branch — commit df30651

Target	Flash Δ	RAM Δ
MATEKF405 ⚠️	+8368 B (+1.27%)	+24 B (+0.02%)
MATEKF722 ⚠️	-48 B (-0.01%)	+16 B (+0.01%)
MATEKF765 ⚠️	+9492 B (+1.39%)	+20 B (+0.01%)
MATEKH743 ⚠️	+9336 B (+1.30%)	-32 B (-0.02%)

See RAM/flash optimization guide for techniques to reduce usage.

[github-actions]

Test firmware build ready — commit df30651

Download firmware for PR #11824

246 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.