Skip to content

Conversation

@DavisNT
Copy link
Contributor

@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.
@github-actions
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
Contributor 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
@NeroBurner NeroBurner added this to the 1.16.0 milestone Sep 3, 2025
@NeroBurner NeroBurner requested a review from mark9064 September 3, 2025 18:53
Copy link
Member

@mark9064 mark9064 left a comment

Choose a reason for hiding this comment

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

At a glance seems fine to me - haven't looked at the code around this though

Refactoring this problem away would be better but that can be done another time

@DavisNT
Copy link
Contributor Author

DavisNT commented Sep 4, 2025

@mark9064, @NeroBurner Should I do the refactoring (implement a constructor for NotificationManager::Notification that checks for buffer overflows)?

@mark9064
Copy link
Member

mark9064 commented Sep 7, 2025

That would be great :)

@DavisNT
Copy link
Contributor Author

DavisNT commented Oct 2, 2025

I have made a new PR with the refactoring: #2347
It also requires corresponding changes in InfiniSim: InfiniTimeOrg/InfiniSim#181

@NeroBurner NeroBurner merged commit 74cf69b into InfiniTimeOrg:main Nov 4, 2025
7 checks passed
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