InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.64k stars 907 forks source link

notification: Initialise message #1694

Open FintasticMan opened 1 year ago

FintasticMan commented 1 year ago

Prevents reading uninitialised memory if notification gets cut off due to being more than 100 chars. The last character is assumed to be \0, but it is actually uninitialised.

github-actions[bot] commented 1 year ago
Build size and comparison to main: Section Size Difference
text 406716B 80B
data 940B 0B
bss 53568B 0B
Riksu9000 commented 1 year ago

Can you show where and how this issue occurs, so we can verify it?

FintasticMan commented 1 year ago

I don't think that this has ever caused an issue, but if a message gets sent to the watch with more than 100 chars, the first 100 are copied, where the last of those won't be a null-terminator. To solve that, the message array holds 101 chars, so that the last one will be a null-terminator. Except because it's not initialised, it is technically an indeterminate value unless explicitly written to.

This is just a memory safety change, and both AlertNotificationClient and AlertNotificationServer already explicitly add a trailing null-terminator (but I think they don't make use of that extra char stored in the array). We could instead trust that people writing code that sends notifications will remember to add the extra null-terminator if the message is too big.

Riksu9000 commented 1 year ago

It would still be assuming that the struct is being used in the correct way. Someone could still fill the entire array. Maybe the public notification struct should use an std::string, which when pushing the notification would then be converted to a private format which uses a char array in a safe way.