nasa / cFE

The Core Flight System (cFS) Core Flight Executive (cFE)
Apache License 2.0
410 stars 202 forks source link

SCA Finding in CFE_ES_StartAppTask #2457

Open ejtimmon opened 11 months ago

ejtimmon commented 11 months ago

Describe the bug Klockwork static analysis tool flagged the following finding:

File: /ccrs/flight-sw/fsw/cfe/modules/es/fsw/src/cfe_es_apps.c Line: 607 Function: CFE_ES_StartAppTask Finding: Result of function that may return NULL will be dereferenced

To Reproduce Run Klocwork tool

Reporter Info Beth Geist/NASA GSFC

skliper commented 11 months ago

https://github.com/nasa/cFE/blob/03166722cdde3f1b337742088e7137ebacf64734/modules/es/fsw/src/cfe_es_apps.c#L606-L607

jphickey commented 11 months ago

This is common with static analysis, in that it doesn't know context or relationships between things. Specifically in this context the CFE ES task ID is a conversion from the OSAL task ID, and its in a context where the OSAL task ID was checked. Here's a bit more context: https://github.com/nasa/cFE/blob/03166722cdde3f1b337742088e7137ebacf64734/modules/es/fsw/src/cfe_es_apps.c#L598-L607

Although CFE_ES_LocateTaskRecordByID() can theoretically return NULL, it only does so if/when passed in an ID that's not within the valid range. But unless the CFE_ES_TaskId_FromOSAL() is broken, the ID that this returns cannot be out of range.

This is a bit of a design holdover from the days where the OSAL ID was directly used as the CFE task ID. Its no longer used directly, but there is a 1:1 mapping between them, and that mapping is implemented in the CFE_ES_TaskId_FromOSAL function. So as long as that mapping is OK and for every valid OSAL ID it returns a valid CFE ES task ID, there is no real danger here.

That's not to say its 100% robust - changing OSAL or CFE could break this - but in the current configuration it is not possible to get a NULL return fromCFE_ES_LocateTaskRecordByID() here. I am not sure, but its possible that it was checked for NULL at one point but was impossible to reach from coverage testing, thus removed.