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