fix(freertos): Limit idle task name length copy operation and ensure null-termination of the idle task name string

[sudeep-mohanty]

This PR:
- Limits the idle task name length copy operation to prevent Out-of-bounds memory access warnings from static code analyzers.
- Fixes a bug where in the idle task name could be non null-terminated string for SMP configuration.

Description

• In the function prvCreateIdleTasks(), we have the operation -

       for( xIdleTaskNameIndex = ( BaseType_t ) 0; xIdleTaskNameIndex < ( BaseType_t ) configMAX_TASK_NAME_LEN; xIdleTaskNameIndex++ )
       {
          cIdleName[ xIdleTaskNameIndex ] = configIDLE_TASK_NAME[ xIdleTaskNameIndex ];
  
          /* Don't copy all configMAX_TASK_NAME_LEN if the string is shorter than
           * configMAX_TASK_NAME_LEN characters just in case the memory after the
           * string is not accessible (extremely unlikely). */
          if( cIdleName[ xIdleTaskNameIndex ] == ( char ) 0x00 )
          {
             break;
          }
          else
          {
             mtCOVERAGE_TEST_MARKER();
          }     
       }

• Static code analyzers could flag this copy operation as an Out-of-bounds memory access because they see configIDLE_TASK_NAME as an array of 5 bytes (I, D, L, E, \0) but the loop running for configMAX_TASK_NAME_LEN iterations which could be more than 5.
• They also do not assume that a \0 character is present in the configIDLE_TASK_NAME array and hence can not predict that the loop will break before an Out-of-bounds memory access is made.
• Therefore, this PR limits the copy operation to the minimum of either configIDLE_TASK_NAME or configMAX_TASK_NAME_LEN. This ensures that the copy operation runs for exactly the required number of iterations to copy the idle task name, 5 by default.
• The PR also fixes a bug with null-termination of the IDLE task name if strlen(configIDLE_TASK_NAME) = configMAX_TASK_NAME_LEN - 1 when SMP configuration is enabled. The current code would append the core ID to the task name and in the process overwrites the null terminator. It then exists the loop as there is no more space to add a null terminator.

Test Steps

• Run a static code analysis of tasks.c on a tool like Coverity.

Checklist:

• I have tested my changes. No regression in existing tests.

Related Issue

None

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[jasonpcarroll]

Doing this means you no longer need the logic within the for loop that limits how many characters are copied into the buffer. you might as well remove that logic as well. Also, since you are already using strlen why not just replace all of this code with strncpy? https://cplusplus.com/reference/cstring/strncpy/ with n set to to the max length?

[sudeep-mohanty]

@jasonpcarroll I've removed the break-ing logic inside the loop and updated it to use standard string operations.

On a second note, just discovered that there is a potential problem in handling the null-termination of the IDLE task name in case of SMP configuration. The current logic would not null-terminate the idle task array if strlen(configIDLE_TASK_NAME) = configMAX_TASK_NAME_LEN - 1. The logic would append the xCoreID, in the process overwriting the null character and then not adding a null-character any more.

I've updated the logic to use strncat instead.

[sudeep-mohanty]

Quality Gate failed

Failed conditions 1 Security Hotspot

See analysis details on SonarQube Cloud

@jasonpcarroll Looks like I would need an additional review on SonarQube for using strncpy. Thanks.

[sudeep-mohanty]

@jasonpcarroll Could you let me know if this PR is reviewable as is or do I need to make some changes for the checks to pass. Thanks.

[aggarg]

We try to avoid snprintf class of functions because it might bloat the code size and use a lot of stack. Only stats formatting functions use snprintf which is more of a debugging aid. I'd like to avoid snprintf or string manipulation functions for IDLE task name which is also a debugging aid. What do you think about the following patch - suggestion.patch?

[sudeep-mohanty]

Thanks for the inputs @aggarg! I've incorporated your suggestions but since it did not address the out-of-bound mem copy operation warnings flagged by static code analyzers, I have added a fix for it as well. PTAL. Thanks.

[sudeep-mohanty]

Updated corresponding unit test in FreeRTOS PR #1314.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud