feat: replace sprintf with snprintf #746

araujo88 · 2023-08-10T03:35:13Z

Description

Replaces sprintf with snprintf.

Checklist:

I have tested my changes. No regression in existing tests.
I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

ydhuang28 · 2023-08-14T16:34:20Z

@araujo88 Thank you for the PR. As a convention, we put config macros in FreeRTOSConfig.h. Could you modify your PR to have the new defined macro in FreeRTOSConfig.h?

araujo88 · 2023-08-15T00:12:31Z

@araujo88 Thank you for the PR. As a convention, we put config macros in FreeRTOSConfig.h. Could you modify your PR to have the new defined macro in FreeRTOSConfig.h?

@ydhuang28 You meant FreeRTOS.h? The file FreeRTOSConfig.h doesn't exist.

htibosch · 2023-08-15T04:13:30Z

@araujo88 wrote:

You meant FreeRTOS.h? The file FreeRTOSConfig.h doesn't exist.

FreeRTOS.h is part of the kernel sources.
FreeRTOSConfig.h is to be created and maintained by you, it is part of your your project.

For instance, configTICK_RATE_HZ has a default of 1000, but you can change it to for instance:

#define configTICK_RATE_HZ    100U

FreeRTOSConfig.h is often placed in a special include directory called config, together with e.g. FreeRTOSIPConfig.h

htibosch · 2023-08-15T04:34:50Z

Unfortunately, the function snprintf() is often deprecated, as you see in the lint messages.

It seems safe to use it because it limits the number of bytes written to a buffer.

But a problem occurs when it doesn't fit, I have seen different behaviours:

snprintf() returns the number of bytes that would be written to s had n been sufficiently large ..
snprintf() returns the number of bytes that are written in the array, not counting the ending null character.
If it returns a negative value, then either the maxlen character limit was reached or some other error, such as an invalid format specification, has occurred.

In embedded applications, I use printf-stdarg.c, which has a safe implementation of the [v]s[n]printf family. It will not overwrite the buffer, and it makes sure that a terminating nul is written, unless n equals zero.

When sprintf() is used in the kernel, it has been made sure the buffers can not be overwritten.

ydhuang28 · 2023-08-15T17:40:52Z

@htibosch I agree with limiting the use of snprintf (or any stdio.h functions, for that matter). However, we do still want to provide a simple way to output human-readable text for task information for non-production uses, which is what this is.

I seem to be missing the context of

When sprintf() is used in the kernel, it has been made sure the buffers can not be overwritten

As I understand the code in its current state, I don't see a protection on writing to the buffer? Perhaps I'm missing something?

ydhuang28 · 2023-08-15T17:45:09Z

@araujo88 Correct, like @htibosch has mentioned, FreeRTOSConfig.h exists in FreeRTOS/FreeRTOS. The correct way is to raise another PR to the "classic" FreeRTOS repo with the change to FreeRTOSConfig.h, since we do default back to the original sprintf() if the config macro is not defined.

htibosch · 2023-08-17T06:58:10Z

@araujo88 wrote:

we do still want to provide a simple way to output human-readable text for task information for non-production uses, which is what this is.

Now I've got your point, same thing in FreeRTOS+TCP: you will also see that snprintf() is only used for debugging and logging. It is included when ipconfigHAS_PRINTF or ipconfigHAS_DEBUG_PRINTF is defined.

When sprintf() is used in the kernel, it has been made sure the buffers can not be overwritten

Sorry, my mistake: the only case where sprintf() is used to write to pcWriteBuffer, which is user provided.

jefftenney · 2023-08-17T23:34:03Z

tasks.c

+                }
+                #else
+                {
+                    snprintf( pcWriteBuffer, configTASK_WRITE_BUFFER_LENGTH, "\t%c\t%u\t%u\t%u\r\n", cStatus, ( unsigned int ) pxTaskStatusArray[ x ].uxCurrentPriority, ( unsigned int ) pxTaskStatusArray[ x ].usStackHighWaterMark, ( unsigned int ) pxTaskStatusArray[ x ].xTaskNumber ); /*lint !e586 snprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */


The use of configTASK_WRITE_BUFFER_LENGTH here should instead be a local size_t variable that is initialized to configTASK_WRITE_BUFFER_LENGTH, and it should shrink with each loop iteration wherever pcWriteBuffer is advanced. The two variables should stay in sync.

jefftenney · 2023-08-17T23:42:04Z

tasks.c

@@ -6615,8 +6615,16 @@ static void prvResetNextTaskUnblockTime( void )
                pcWriteBuffer = prvWriteNameToBuffer( pcWriteBuffer, pxTaskStatusArray[ x ].pcTaskName );

                /* Write the rest of the string. */
-                sprintf( pcWriteBuffer, "\t%c\t%u\t%u\t%u\r\n", cStatus, ( unsigned int ) pxTaskStatusArray[ x ].uxCurrentPriority, ( unsigned int ) pxTaskStatusArray[ x ].usStackHighWaterMark, ( unsigned int ) pxTaskStatusArray[ x ].xTaskNumber ); /*lint !e586 sprintf() allowed as this is compiled with many compilers and this is a utility function only - not part of the core kernel implementation. */
-                pcWriteBuffer += strlen( pcWriteBuffer );                                                                                                                                                                                                /*lint !e9016 Pointer arithmetic ok on char pointers especially as in this case where it best denotes the intent of the code. */
+                #ifndef configTASK_WRITE_BUFFER_LENGTH


To reduce conditional code, configTASK_WRITE_BUFFER_LENGTH could have a default value of SIZE_MAX if the developer doesn't define it. Actually, it seems SIZE_MAX was introduced in C99, so that symbol would need to be defined conditionally first, e.g., ( (size_t) -1 ). This change would then eliminate all the calls to sprintf() as they would all be calls to snprintf().

As a (better?) alternative, if configUSE_STATS_FORMATTING_FUNCTIONS > 0 then we could generate a compile-time error if configTASK_WRITE_BUFFER_LENGTH is not defined. This is not backward compatible but perhaps is justified. Would need direction from the FreeRTOS team on this question. EDIT: See next comment.

If the FreeRTOS team thinks this improvement warrants breaking backward compatibility for vTaskList() vTaskGetRunTimeStats(), then the simpler, direct fix is to add a second formal parameter, eg, bufferSize, to these two functions, and not to add configTASK_WRITE_BUFFER_LENGTH at all. I think most developers would appreciate being "forced" into this improvement during a FreeRTOS upgrade. Otherwise, they likely wouldn't benefit from this improvement during the upgrade (and likely wouldn't even know about it). These are peripheral functions to the kernel, thus not being backward compatible might be OK.

The current thought is that we don't want to break backwards compatibility (demo code maybe using this function and we want to minimize the impact to that), so I think adding just a config for this is fine.

codecov · 2023-08-19T03:41:38Z

Codecov Report

Patch coverage has no change and project coverage change: +0.20% 🎉

Comparison is base (2f94b18) 94.35% compared to head (22b33da) 94.55%.
Report is 12 commits behind head on main.

❗ Current head 22b33da differs from pull request most recent head 2a19f7d. Consider uploading reports for the commit 2a19f7d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #746      +/-   ##
==========================================
+ Coverage   94.35%   94.55%   +0.20%     
==========================================
  Files           6        6              
  Lines        2446     4392    +1946     
  Branches      598     1161     +563     
==========================================
+ Hits         2308     4153    +1845     
- Misses         85      154      +69     
- Partials       53       85      +32

Flag	Coverage Δ
unittests	`94.55% <ø> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
tasks.c	`95.01% <ø> (+0.18%)`	⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ydhuang28 · 2023-09-12T17:11:14Z

Linked PR (adds the config macro definition): FreeRTOS/FreeRTOS#1071

chinglee-iot · 2023-09-25T03:54:56Z

This PR is covered by #802 and therefore will be closed. Thanks for creating this PR.

chinglee-iot · 2023-09-25T03:56:36Z

Sorry, my mistake. Once PR #802 is merged. We will close this PR.

aggarg · 2023-09-26T17:42:23Z

Closing this in favor of #802.

feat: replace sprintf with snprintf

b530791

araujo88 requested a review from a team as a code owner August 10, 2023 03:35

araujo88 and others added 2 commits August 17, 2023 01:54

Merge branch 'main' into feat/replace-sprintf-with-snprintf

c2746bf

fix: remove macro

0e4f0c6

araujo88 mentioned this pull request Aug 17, 2023

added macro configTASK_WRITE_BUFFER_LENGTH FreeRTOS/FreeRTOS#1071

Closed

2 tasks

amazonKamath added the First Code Review In-Progress label Aug 17, 2023

jefftenney suggested changes Aug 17, 2023

View reviewed changes

Merge branch 'main' into feat/replace-sprintf-with-snprintf

842b0ac

araujo88 and others added 2 commits August 22, 2023 22:49

Merge branch 'main' into feat/replace-sprintf-with-snprintf

e08494f

Merge branch 'main' into feat/replace-sprintf-with-snprintf

6295112

Merge branch 'main' into feat/replace-sprintf-with-snprintf

22b33da

aggarg mentioned this pull request Sep 21, 2023

Replace sprintf with snprintf #802

Merged

2 tasks

chinglee-iot closed this Sep 25, 2023

chinglee-iot reopened this Sep 25, 2023

Uncrustify: triggered by comment.

2a19f7d

aggarg closed this Sep 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace sprintf with snprintf #746

feat: replace sprintf with snprintf #746

araujo88 commented Aug 10, 2023

ydhuang28 commented Aug 14, 2023

araujo88 commented Aug 15, 2023

htibosch commented Aug 15, 2023

htibosch commented Aug 15, 2023

ydhuang28 commented Aug 15, 2023

ydhuang28 commented Aug 15, 2023

htibosch commented Aug 17, 2023

jefftenney Aug 17, 2023

jefftenney Aug 17, 2023 •

edited

Loading

jefftenney Aug 18, 2023

ydhuang28 Sep 12, 2023

codecov bot commented Aug 19, 2023 •

edited

Loading

ydhuang28 commented Sep 12, 2023

chinglee-iot commented Sep 25, 2023

chinglee-iot commented Sep 25, 2023

aggarg commented Sep 26, 2023

feat: replace sprintf with snprintf #746

feat: replace sprintf with snprintf #746

Conversation

araujo88 commented Aug 10, 2023

Description

Checklist:

Related Issue

ydhuang28 commented Aug 14, 2023

araujo88 commented Aug 15, 2023

htibosch commented Aug 15, 2023

htibosch commented Aug 15, 2023

ydhuang28 commented Aug 15, 2023

ydhuang28 commented Aug 15, 2023

htibosch commented Aug 17, 2023

jefftenney Aug 17, 2023

Choose a reason for hiding this comment

jefftenney Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

jefftenney Aug 18, 2023

Choose a reason for hiding this comment

ydhuang28 Sep 12, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 19, 2023 • edited Loading

Codecov Report

ydhuang28 commented Sep 12, 2023

chinglee-iot commented Sep 25, 2023

chinglee-iot commented Sep 25, 2023

aggarg commented Sep 26, 2023

jefftenney Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 19, 2023 •

edited

Loading