[FIX] Prevent stack smashing via unbounded OSD message array writes#11365
[FIX] Prevent stack smashing via unbounded OSD message array writes#11365sensei-hacker wants to merge 5 commits into
Conversation
Both osdGetSystemMessage() and osdGetMultiFunctionMessage() accumulate
message pointers into a fixed-size array with no bounds checking. With
recent additions (geozone avoidance, pitot validation, etc.) the number
of potential writes exceeds the array size, causing stack corruption.
Introduce a scoped ADD_MSG() macro that guards each write:
#define ADD_MSG(msg) if (messageCount < ARRAYLEN(messages)) \
messages[messageCount++] = (msg)
Excess messages are silently dropped rather than written past the end
of the array. The macro is #undef'd at the end of each function.
Fixes: iNavFlight#10048 (Yury-MonZon)
|
That fix was made in a macro way in order to keep the original code intact as much as possible. At current state(after 2 major versions) I suggest re-writing it as a function with counter which guards the limits of the buffer. Easier to read - less problems later. |
|
I started to do it as a function, but then you have to pass messages and a pointer to the count in each of 57 calls. 🫤 I think your original approach of using a macro was best way. Just maybe reads cleaner with ADD_MSG(msg) rather than messages[nextMsg()] = msg. |
No, only pass the message. The function can read/write the message counter by itself. |
I'm sleepy today and not 100% on my game, so maybe I'm missing something but - |
|
Problem with this is it still doesn't deal the the issue of message priority, more important messages never appear if the queue gets clogged with less important messages. |
Sure, prioritizing eight or more concurrent system messages is not a goal of this PR. With the current code, if you have too many messages it crashes the flight controller. Preventing that crash is the goal here. If someone wants to make a PR that prioritizes messages, that would be cool. Though frankly, if you have eight or more system messages going off at once, the priority should probably be -- |
…changes Correctly resolve the incomplete merge of PR iNavFlight#11308 (multifunction battery voltage warnings) into fix/pr10048-add-msg: - Replace old getBatteryState() voltage check with checkBatteryVoltageState() and update messages to "VBATT LAND"/"VBATT LOW " to match PR iNavFlight#11308 - Fix capacity warning block to use ADD_MSG() instead of raw array write - Remove stale &warningsCount argument from osdCheckWarning() calls for RTH sanity, altitude sanity, magnetometer, pitot, and ground test mode
|
Test firmware build ready — commit Download firmware for PR #11365 225 targets built. Find your board's
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Pull request overview
This PR prevents stack corruption in OSD message assembly by replacing unchecked fixed-array writes with bounded ADD_MSG() insertion in the affected OSD message functions.
Changes:
- Adds bounds-checked message insertion for system OSD messages.
- Adds the same protection for multi-function warning messages.
- Keeps message selection behavior otherwise unchanged, with excess messages ignored once capacity is reached.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t#11365 Conflict resolution in src/main/io/osd.c: - Kept upstream's messages[8] (larger array for new geozone messages) - Applied ADD_MSG() bounds-safe macro to upstream's relocated geozone block - Took upstream's expanded NAV_ALTHOLD_MODE block with terrain follow messages (OSD_MSG_SURFACE_OK/BAD), using ADD_MSG() for all writes - Removed osdCheckWarning/osdGetMultiFunctionMessage from osd.c (functions moved to fc/multifunction.c by upstream) Applied ADD_MSG() protection to src/main/fc/multifunction.c where upstream relocated the warning functions from osd.c. Note: build_hw/ excluded — accidentally tracked upstream artifact, see iNavFlight#11583 (fix pending for maintenance-9.x).
Summary
Both
osdGetSystemMessage()andosdGetMultiFunctionMessage()accumulate message pointers into fixed-size arrays with no bounds checking. With additions since this was originally reported (geozone avoidance, pitot validation, etc.) the number of potential writes can exceed the array size, causing stack corruption during active flight.This is a rebase/rework of #10048 by @Yury-MonZon, adapted to current maintenance-9.x.
Some folks didn't care for the syntax:
So I adjusted it to:
This replaces the unsafe:
Related
Fixes #10048