Compile-time-enforced bounds for fixed-layout MSP SET handlers - #11823
Conversation
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 actually reads, so a future field addition could silently drift the two out of sync. Replace the imperative field-by-field reads with a packed wire struct per message, a STATIC_ASSERT pinning its size, and a single sbufReadDataSafe call, moving constrain()/clamp logic to the assignment step. Also fixes a latent bug this surfaced: sbufReadData doesn't advance the buffer pointer, so RC_TUNING's second read for its optional 11th byte was silently re-reading byte 0 instead of byte 10.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoEnforce fixed MSP SET payload bounds at compile time
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
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 reviewTip of the day💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt |
|
Test firmware build ready — commit Download firmware for PR #11823 246 targets built. Find your board's
|
|
RAM / Flash usage vs. base branch — commit
See RAM/flash optimization guide for techniques to reduce usage. |
1 participant
Summary
MSP_SET_RC_TUNING,MSP2_INAV_SET_RATE_PROFILE, andMSP2_COMMON_SET_MSP_RC_INFOeach relied on a hand-writtendataSizeguard 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 singlesbufReadDataSafecall, movingconstrain()/clamp logic to the assignment step. No intended behavior change for well-formed input.Changes
mspSetRcTuning_t,mspSetRateProfile_t,mspSetMspRcInfo_tpacked structs withSTATIC_ASSERT-pinned sizessbufReadDataSafe+ struct-field assignmentMSP_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 trailingrcYawExpo8byteMSP2_COMMON_SET_MSP_RC_INFOkeeps its originaldataSize >= 15(not narrowed to==) to preserve existing lenient-trailing-bytes behaviormspReadRates(), dead code after both its callers were converted to inlineconstrain()callssbufReadData/sbufReadDataSafedon't advance the buffer's read pointer (unlikesbufReadU8/U16/U32), soMSP_SET_RC_TUNING's second read for its optional 11th byte was silently re-reading byte 0 instead of byte 10, corruptingrcYawExpo8on any 11-byte legacy frame. Fixed with an explicitsbufAdvance()between the two reads — this idiom is already used elsewhere in this file and intelemetry/msp_shared.c.Testing
MIN()vsconstrain()vs no-clamp applies todynPIDin each handler — deliberately different betweenMSP_SET_RC_TUNINGandMSP2_INAV_SET_RATE_PROFILE)MSP_SET_RC_TUNING(both 10- and 11-byte variants) andMSP2_INAV_SET_RATE_PROFILE, read back via the corresponding GET commands, confirmed exact match including clamp behavior — this test is what caught thesbufAdvancebug above before it shippedinav-code-reviewpass: 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 patternRelated Issues
Related to #11672
Sibling of #11822 (fix-msp-sensor-command-bounds-check, already merged/open)