Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImmediateAlertService: fix latent bug #2159

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DavisNT
Copy link

@DavisNT DavisNT commented Nov 3, 2024

Include null terminator in the bytes copied.
Set notif.size like it is done in AlertNotificationService.cpp and AlertNotificationClient.cpp.

Include null terminator in the bytes copied.
Set notif.size as it is done in AlertNotificationService.cpp and
AlertNotificationClient.cpp.
Copy link

github-actions bot commented Nov 3, 2024

Build size and comparison to main:

Section Size Difference
text 374512B -16B
data 948B 0B
bss 63488B 0B

Copy link
Member

@FintasticMan FintasticMan left a comment

Choose a reason for hiding this comment

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

I'm fine with this implementation, but I think that it might be good to also use std::min to avoid overflowing the buffer. (Of course that isn't possible with the current notification strings, but it makes the code more clear if we do that)

Because there are some inconsistencies and incorrect handlings of the notification buffer elsewhere in the code, I'm thinking of adding a constructor to the struct that makes sure it's written correctly. In the meantime, this is a good fix.

@DavisNT
Copy link
Author

DavisNT commented Nov 5, 2024

@FintasticMan Thanks for the review!
In this particular case the alertString is one of string constants defined in lines 20-26 of ImmediateAlertService.cpp and is never longer than 12 characters (unless someone would modify this code).

In general constructor for NotificationManager::Notification that checks for buffer overflows sounds like a very good idea. 👍

P.S. Would you like me to implement this constructor?

@FintasticMan
Copy link
Member

I was planning on doing it, but if you want to, go ahead!

@mark9064 mark9064 added the maintenance Background work label Nov 18, 2024
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.

3 participants