Fix potential buffer overflows when calling sprintf

[szsam]

No description provided.

[github-actions]

Build size and comparison to main:

Section	Size	Difference
text	377632B	16B
data	940B	0B
bss	63492B	0B

[minacode]

Welcome to InfiniTime! 😊 I think it would be helpful if you write a little description about the motivations of the changes you made.

[Avamander]

These aren't overflows, truncations at best and modulo is relatively quite expensive.

The other parts, would be very nice if you would explain them.

[szsam]

A recent commit introduces a real one-byte buffer overflow.

InfiniTime/src/displayapp/screens/StopWatch.cpp

Lines 201 to 206 in 40f7e1c

	char buffer[16];
	if (times.hours == 0) {
	sprintf(buffer, "#%2d %2d:%02d.%02d\n", i + 1, times.mins, times.secs, times.hundredths);
	} else {
	sprintf(buffer, "#%2d %2d:%02d:%02d.%02d\n", i + 1, times.hours, times.mins, times.secs, times.hundredths);
	}

The format str "#%2d %2d:%02d:%02d.%02d\n" requires 17 bytes, but the buffer length is only 16. This is fixed in this PR.

[FintasticMan]

I don't think that replacing sprintf with snprintf is necessarily a good idea. I believe that because it adds an overhead and that, with the current configuration, I don't think it does anything if it detects an overflow, we shouldn't use it.

[szsam]

I don't think that replacing sprintf with snprintf is necessarily a good idea. I believe that because it adds an overhead and that, with the current configuration, I don't think it does anything if it detects an overflow, we shouldn't use it.

I agree that snprintf adds overhead. However, when snprintf(buffer, bufsz, fmt, ...) detects an overflow, it will write at most bufsz-1 characters, effectively preventing buffer overflow.

That said, if you insist on using sprintf, I will change the code and use it.

[FintasticMan]

After looking into the overhead that snprintf adds, I've come to the conclusion that it is fine to use it over sprintf as an added layer of safety. We should of course still make sure that our values can never be higher than what we expect and that our buffers are big enough.

For some reason changing the format specifier macro to PRIu16 fixes the issue I mentioned before. Once that is fixed, I'm willing to get this merged.

[szsam]

@FintasticMan Rebase done.

[NeroBurner]

LGTM

[Machiry]

Hello @FintasticMan, @Avamander, @NeroBurner ,

My name is Aravind Machiry, Assistant Professor at Purdue's ECE Department.

Thank you for considering this pull request. This pull request was the result of our on-going research work (along with @szsam) to improve the security and quality of open-source embedded projects.

In addition to scanning codebases with CodeQL, we are also doing a short (~4 minutes) survey to understand the use of static analysis tools like gcc -Wall and CodeQL in embedded software projects.

It would greatly benefit our research if you could fill this anonymous survey: https://purdue.ca1.qualtrics.com/jfe/form/SV_0OnXfr5plPe1QCa

Thank you,
Aravind