Skip to content

ImmediateAlertService: fix latent bug#2159

Merged
NeroBurner merged 1 commit into
InfiniTimeOrg:mainfrom
DavisNT:fixias
Nov 4, 2025
Merged

ImmediateAlertService: fix latent bug#2159
NeroBurner merged 1 commit into
InfiniTimeOrg:mainfrom
DavisNT:fixias

Conversation

@DavisNT

@DavisNT DavisNT commented Nov 3, 2024

Copy link
Copy Markdown
Contributor

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

github-actions Bot commented Nov 3, 2024

Copy link
Copy Markdown

Build size and comparison to main:

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

@FintasticMan FintasticMan left a comment

Copy link
Copy Markdown
Member

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

DavisNT commented Nov 5, 2024

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

@mark9064 mark9064 left a comment

Copy link
Copy Markdown
Member

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

DavisNT commented Sep 4, 2025

Copy link
Copy Markdown
Contributor Author

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

@mark9064

mark9064 commented Sep 7, 2025

Copy link
Copy Markdown
Member

That would be great :)

@DavisNT

DavisNT commented Oct 2, 2025

Copy link
Copy Markdown
Contributor Author

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
metaphys pushed a commit to metaphys/InfiniTime that referenced this pull request Mar 30, 2026
Include null terminator in the bytes copied.
Set notif.size as it is done in AlertNotificationService.cpp and
AlertNotificationClient.cpp.
iakat pushed a commit to iakat/InfiniTime that referenced this pull request Jun 8, 2026
Include null terminator in the bytes copied.
Set notif.size as it is done in AlertNotificationService.cpp and
AlertNotificationClient.cpp.
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