Compile-time-enforced bounds for fixed-layout MSP SET handlers

[sensei-hacker]

Summary

MSP_SET_RC_TUNING, MSP2_INAV_SET_RATE_PROFILE, and MSP2_COMMON_SET_MSP_RC_INFO each relied on a hand-written dataSize guard with no compiler-enforced link to what the handler body actually reads — a future field addition could silently drift the guard out of sync with the read sequence. This converts all three to a packed wire struct + STATIC_ASSERT(sizeof(...) == N, ...) + a single sbufReadDataSafe call, moving constrain()/clamp logic to the assignment step. No intended behavior change for well-formed input.

Changes

• Add mspSetRcTuning_t, mspSetRateProfile_t, mspSetMspRcInfo_t packed structs with STATIC_ASSERT-pinned sizes
• Replace each handler's imperative reads with guard + sbufReadDataSafe + struct-field assignment
• MSP_SET_RC_TUNING's legacy two-length wire format (10 or 11 bytes) is handled with the fixed 10-byte struct plus a conditional second read for the trailing rcYawExpo8 byte
• MSP2_COMMON_SET_MSP_RC_INFO keeps its original dataSize >= 15 (not narrowed to ==) to preserve existing lenient-trailing-bytes behavior
• Remove mspReadRates(), dead code after both its callers were converted to inline constrain() calls
• Fixes a latent bug this surfaced: sbufReadData/sbufReadDataSafe don't advance the buffer's read pointer (unlike sbufReadU8/U16/U32), so MSP_SET_RC_TUNING's second read for its optional 11th byte was silently re-reading byte 0 instead of byte 10, corrupting rcYawExpo8 on any 11-byte legacy frame. Fixed with an explicit sbufAdvance() between the two reads — this idiom is already used elsewhere in this file and in telemetry/msp_shared.c.

Testing

• Built SITL successfully, no compiler warnings
• Field-by-field review of all three handlers against the original imperative read sequence (byte offsets, and which of MIN() vs constrain() vs no-clamp applies to dynPID in each handler — deliberately different between MSP_SET_RC_TUNING and MSP2_INAV_SET_RATE_PROFILE)
• SITL round-trip test (44/44 checks): sent distinct values for every field of both MSP_SET_RC_TUNING (both 10- and 11-byte variants) and MSP2_INAV_SET_RATE_PROFILE, read back via the corresponding GET commands, confirmed exact match including clamp behavior — this test is what caught the sbufAdvance bug above before it shipped
• Independent inav-code-review pass: approved, no correctness issues; re-derived all three struct layouts from scratch against the pre-diff code and confirmed a full-tree grep found no other instance of the double-sbufReadDataSafe-without-advance pattern

Related Issues

Related to #11672

Sibling of #11822 (fix-msp-sensor-command-bounds-check, already merged/open)

[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

Enforce fixed MSP SET payload bounds at compile time

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

AI Description

• Enforces fixed MSP payload layouts with packed structs and compile-time size assertions.
• Preserves legacy lengths and clamping while safely mapping fields into runtime state.
• Advances legacy RC tuning reads correctly, preventing optional yaw expo corruption.

Diagram

graph TD
    A["MSP SET payload"] --> B{"Size valid?"} --> C["Packed wire struct"] --> D["Safe buffer copy"] --> E["Clamp fields"] --> F["Runtime state"]

Loading

High-Level Assessment

The packed wire-struct approach is appropriate because it directly couples accepted fixed payload sizes to compiler-checked layouts while preserving existing protocol behavior. Field-by-field safe reads would retain duplicated layout knowledge, and a generated decoder would add disproportionate complexity for three local handlers.

Files changed (1) +93 / -44

Bug fix (1) +93 / -44

fc_msp.cHarden fixed-layout MSP SET command decoding +93/-44

Harden fixed-layout MSP SET command decoding

• Adds packed, statically size-checked wire structs for RC tuning, rate profile, and MSP RC information payloads, then maps safely copied fields into runtime configuration with existing constraints preserved. Explicitly advances the RC tuning buffer before reading its optional legacy yaw-expo byte, fixing corruption caused by rereading byte zero, and removes the now-unused rate-reading helper.

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]

Test firmware build ready — commit c4ac91b

Download firmware for PR #11823

247 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.

[github-actions]

RAM / Flash usage vs. base branch — commit c4ac91b

No size baseline is available yet for this PR's base commit (no per-commit baseline has been published for it). This comment will show deltas once one exists — rebasing the PR refreshes its base commit.

Target	Flash Δ	RAM Δ
MATEKF405	704751 B (no baseline)	149520 B (no baseline)
MATEKF722	465539 B (no baseline)	125260 B (no baseline)
MATEKF765	736379 B (no baseline)	165432 B (no baseline)
MATEKH743	773839 B (no baseline)	167944 B (no baseline)

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

[sensei-hacker]

@xznhj8129 you are very familiar with the MSP handling code. Does this look good to you, for robustness in case of data corruption or sender and receiver have different version or similar?

these two are similar concepts:
#11672
#11822

[xznhj8129]

@xznhj8129 you are very familiar with the MSP handling code. Does this look good to you, for robustness in case of data corruption or sender and receiver have different version or similar?

these two are similar concepts: #11672 #11822

Oh yeah this looks much better, having the defined struct like this instead of reading it piecemeal makes it a lot easier to handle. Size from sizeof, safe reads, constrain, the packed struct; basically all the MSP handlers should be like this. Those typedef structs are the kind of thing who would belong in an msp definition header as the main message library.

[sensei-hacker]

Thanks!

Note to self - ensure these all allow reading a message with a different length than expected, from a different version of the MSP message.

[sensei-hacker]

Phase 4: lenient gates per forward-compat policy — implemented + SITL-verified.

Following the 2026-08-27 policy decision (MSP payloads only ever gain fields at the END; a newer configurator's longer message must be accepted by older firmware — apply known fields, ignore trailing bytes), the gates are now deliberately lenient:

Handler	Gate	Longer (newer)	Shorter (older)
MSP_SET_RC_TUNING	dataSize >= sizeof(mspSetRcTuning_t) (10)	ACCEPT: 10-byte struct read, rcYawExpo8 read only when dataSize > 10, trailing ignored	REJECT (MSP_RESULT_ERROR)
MSP2_INAV_SET_RATE_PROFILE	dataSize >= sizeof(mspSetRateProfile_t) (15)	ACCEPT: 15-byte struct read, trailing ignored	REJECT
MSP2_COMMON_SET_MSP_RC_INFO	dataSize >= sizeof(mspSetMspRcInfo_t) (15) — unchanged	ACCEPT (already lenient)	REJECT

Implementation: commit c4ac91b changes only the two == gates to >=; the rcYawExpo8 read stays conditional on dataSize > sizeof(mspSetRcTuning_t); MSP_RC_INFO is untouched (was already >= 15). sbufReadDataSafe + sbufAdvance pairing already guarantees clean rejection of shorter-than-struct payloads (no over-read, no partial state mutation).

SITL round-trip verification (extended test_rc_tuning_rate_profile_roundtrip.py, 92/92 checks pass, 0 failures):

• Longer: RC_TUNING 12-byte frame (10-byte struct + rcYawExpo8 + 1 trailing byte) accepted, all fields applied, trailing byte ignored; RATE_PROFILE 17-byte frame (15-byte struct + 2 trailing bytes) accepted, all fields applied, trailing ignored.
• Exact: existing 10/11-byte RC_TUNING and 15-byte RATE_PROFILE round-trips still pass (clamps verified).
• Shorter: RC_TUNING 9-byte frame and RATE_PROFILE 14-byte frame rejected cleanly — state verified unchanged after each (no partial mutation), and the FC remains responsive (well-formed frame accepted immediately after each rejection).

Note: MSP2_COMMON_SET_MSP_RC_INFO is compiled out of the SITL target (USE_RX_MSP not enabled), so it is verified by inspection + its STATIC_ASSERT(sizeof == 15) rather than SITL; its gate and read path are unchanged by this commit.