Skip to content

Conversation

@FintasticMan
Copy link
Member

@FintasticMan FintasticMan commented Mar 16, 2023

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.

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
Copy link

Build size and comparison to main:

Section Size Difference
text 406716B 80B
data 940B 0B
bss 53568B 0B

@FintasticMan FintasticMan added the maintenance Background work label Mar 16, 2023
@FintasticMan FintasticMan requested a review from a team March 16, 2023 21:27
@Riksu9000
Copy link
Contributor

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

@FintasticMan
Copy link
Member Author

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
Copy link
Contributor

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.

@FintasticMan FintasticMan marked this pull request as draft June 17, 2023 17:04
Copy link
Contributor

@minacode minacode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. It's simple, clean and more sane. Any more complex refactoring can be done a new PR, if needed.

Haven't tested it, but the code change is so minimal that I would be very surprised if this breaks something.

@NeroBurner NeroBurner added this to the 1.16.0 milestone May 12, 2025
@NeroBurner
Copy link
Contributor

I agree. Minimal code change resulting in a sane memory default. @FintasticMan could you update this PR to see if it is still applicable or if we changed it already in a similar way since then?

@JF002
Copy link
Collaborator

JF002 commented May 16, 2025

I think this PR is still applicable. The implementation of AlertNotificationClient and AlertNotificationServer should ensure that the last character of the array is a \0 but this is still safer to initialize the content of the array correctly.

The conflict that appears when rebasing should be quite easy to fix, it seems to be caused by the order of the fields that changed since this change was originally made.

@JF002 JF002 merged commit 0880b08 into InfiniTimeOrg:main May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Background work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants