[FIX] possible stack smashing

[Yury-MonZon]

This is not safe:

in functions osdGetSystemMessage() and osdGetMultiFunctionMessage() (osd.c)

const char *messages[5];

<SKIPPED>

messages[messageCount++] =   // <--- Multiple times without counting/limiting

[breadoven]

It's not an issue for osdGetMultiFunctionMessage because the messages array size is matched to the maximum number of messages.

It could be an issue for osdGetSystemMessage although looking at that it could do with cleaning up/improving to limit the max number of messages to the messages array size (which could be increased). I'm guessing the maximum number of 5 messages matched the maximum number of messages expected in the past and was never updated as more messages were added. Having said that 5 seems a reasonable maximum number to display to avoid confusion.

[Yury-MonZon]

It's not an issue for osdGetMultiFunctionMessage

For now. Later someone might add more messages expecting it to be already taken care of.

I still believe it is needed to limit the increment to avoid future problems.

[mmosca]

If it is needed or not, the macro defined in random places is not great.

Also, using the modulus operator (%) probably would be better than having that ternary operator.

[Yury-MonZon]

The macro is undefined at the end of the function, no problems here.

Modulo would work but it will roll over and start overwriting old messages. Isn't it better to just stop adding new messages?

[breadoven]

It's not an issue for osdGetMultiFunctionMessage

For now. Later someone might add more messages expecting it to be already taken care of.

Surely it's up to the developer to ensure they've checked that any additional messages added don't cause problems the same as with any other changes.

Modulo would work but it will roll over and start overwriting old messages. Isn't it better to just stop adding new messages?

But that's one of the problems with this. More important messages may end up not being shown because the queue is full. Surely it's better to configure the messages so no more than the max allowed can be selected with more important messages prioritised. Admittedly osdGetSystemMessage has become a bit complicated which doesn't make it easy trying to work out the max number of messages that could be selected at the same time.

[MrD-RC]

My 2 Cents. The if the message array is full. No new messages should be added. The problem is then prioritising the order so that important messages are added first.

[sensei-hacker]

If it is needed or not, the macro defined in random places is not great.

Also, using the modulus operator (%) probably would be better than having that ternary operator.

I tested and found modulo produces somewhat larger code. Neither option gets 10 points for code clarity.

[sensei-hacker]

Surely it's up to the developer to ensure

While there is some truth to that, IMHO this code / style is a trap. A trap that has already caught us - there are already (many) more messages than the array size. IMHO we should not knowingly add traps to the code when those traps can be avoided.
Would any INAV developer fall into this trap? We already have about ten times, adding different messages in different commits long after there may have been five previous messages.

We could try to stop falling into this trap over and over, or we could remove the trap.

[sensei-hacker]

IMHO we kinda got lost in the weeds on this.

We discussed:
If it tries to write past the end of the buffer, should it just stop or should it wrap around?

Well, it definitely should NOT just keep writing past the end of the buffer, crashing the flight controller. So either way is an improvement over what we have now.

Yes, I'd rather change this:

messages[nextMsg()] = OSD_MESSAGE_STR(OSD_MSG_LANDED);

To:
addMessage(OSD_MSG_LANDED);

That would be cleaner. But to me, this PR isn't bad as it stands; it's not that it makes a big mess of things. It's certainly an improvement over overwriting the return pointer and likely crashing the FC and/or the aircraft.

[breadoven]

I did try and improve the system messaging code by making it easier (via comments) to understand the total number of messages that will be displayed to avoid problems with overflow. However, that fell by the wayside pretty quickly because the comments haven't been updated after new messages were added.

I think the total number of messages needs to limited (probably to around 5) to avoid user overload but in a clever way so more important messages are prioritised.

I suppose the other possibility is to just have a larger buffer or would this causes performance issues ?

[sensei-hacker]

I suppose the other possibility is to just have a larger buffer or would this causes performance issues ?

Seen that done a few times. Larger buffer ends up just being a larger problem. Because over time we'll keep adding messages and the problem will be completely forgotten about by time time it triggers again.

Unless you set the buffer to four times the size of of what anyone could ever use in the future. Kinda like with the FAT file system they made the fields large enough to handle far larger partitions than anyone would ever actually have - up to 2GB. Or how they defined the IPs to 32 bits so it would be able to handle millions of computers, when of course there will never be more than several hundred computers on the Internet.

So you do a million times what you'll need to be safe but then you're just wasting a lot of RAM.

[sensei-hacker]

If anyone sees a way to improve this in the future, such as by tracking which messages are more important, you are more than welcome to PR an improvement. Until then, I think fixing the issue is better than not fixing it.

[sensei-hacker]

Superseded by #11365 for merge conflicts and slight syntax tweak.