Mismatched Variable Types in Data Structures

[dmknutsen]

Describe the bug
While auditing cFE tlm packets for 64 bit alignment issues, I noticed that we have instances of mismatched variable types in data structures. This results in data being truncated/corrupted.

Example:
In cfe_es.h::CFE_ES_AppInfo_t
There are multiple addresses (StartAddress, CodeAddress, etc) declared as uint32. They should be declared as cpuaddr variables - similar to CFE_ES_AppStartParams_t:StartAddress and OS_module_address_t:code_address.

Another example is the priority, stackSize, and Exception Action variables. See below for declaration trace:

CFE_ES_ParseFileEntry:
unsigned int Priority
unsigned int StackSize
unsigned int ExceptionAction

CFE_ES_AppCreate:
uint32 Priority
uint32 StackSize
uint32 ExceptionAction

CFE_ES_AppStartParams_t:
uint16 ExceptionAction
uint16 Priority
uint32 StackSize

CFE_ES_AppInfo_t:
uint16 Priority
uint16 ExceptionAction
uint32 StackSize

Reporter Info
Dan Knutsen
NASA/Goddard

[jphickey]

Part of this was actually a design decision at the time 64 bit support was initially added. There was a desire to not change the TLM/CMD formats at that time, so these fields remained fixed 32 bit in size and the AddressesAreValid should be set FALSE to basically just tell the ground system they are all wrong and not to pay attention to them.

That being said, there might have still been one place (in the mempool stuff?) where telemetry did change size.

[jphickey]

The other aspect is that CFE doesn't ever actually use data structures that are bigger than 32-bit sizes so the size fields remaining as 32-bit was OK. The emphasis was to build and run on the newer architectures, not necessarily to actually use or manipulate huge data structures with sizes > 4GB.

[dmknutsen]

Thanks for the info/feedback! I just double checked and identified the following tlm/cmd that contain 64 bit variables:
CFE_SB_HousekeepingTlm_Payload
CFE_ES_PoolStatsTlm_Payload
CFE_TBL_HouseKeepingTlm_Payload_t
CFE_TBL_TblRegPacket_Payload_t
CFE_ES_SendMemPoolStatsCmd_Payload_t

I think it might be possible that these were updated at some point following the original decision to not modify and CFE_ES_AppInfo_t was missed because it is contained in cfe_es.h - as opposed to cfe_es_msg.h.

[jphickey]

Yeah, those are likely all pointers / memory addresses right? That should be a separate design discussion / steering committee topic on the solution to these. Firstly it should be clarified whether or not the addresses are even needed in the TLM stream in the first place (i.e. what does the ground system actually use that for).

Obviously, receiving a memory address in a CMD (as in CFE_ES_SendMemPoolStatsCmd) and proceeding to dereference it is very dangerous because CFE cannot validate the pointer value wasn't corrupted at some point. I think there is a separate issue ticket about this?

For this we should consider using a more abstract "handle" instead, that the CFE can validate and get the real address of. It can stay 32 bits which will be backward compatible with a 32-bit memory address.

Any other CMD/TLM interactions that deal with direct memory addresses should be relegated to an app (e.g. MM, which already does peeks and pokes) due to the inherent risk involved.

[skliper]

Add to list of issues that need design discussion.

[skliper]

Topics touched on - possibly mission define typedef for memory address sizes (could be 64 or 32...), discussed config vs just going to 64 bit everywhere...
GetMemPoolStats - this is an offender for taking memory addresses.

[astrogeco]

CCB 2020-08-05 See comment above and minutes

[skliper]

Note CFE_TBL_TblRegPacket_Payload_t is unique in that it actually uses cpuaddr. Not good.

[jphickey]

In places where CFE is using these values to call OSAL (e.g. stack pointer and priority at least), OSAL should be identifying the right type to use. Submitted nasa/osal#635 to add a typedef for those items. Will then update CFE to use the typedef.

[jphickey]

My other recommendation is to start migrating toward using cpusize (or perhaps size_t, because it is a standard C type) for internal sizes - as it will match the type resulting from use of e.g. sizeof() operator and thereby avoid any conversions.

Types in commands/telemetry will still need to be standardized by CFE to be fixed width - cannot use cpusize here. But #964 already added a separate typedef for those. So the remaining issue is primarily how it is stored internally and that we do many conversions across APIs.

[jphickey]

After reviewing the code a bit more I'm leaning more toward using the standard size_t internally and using the CFE_ES_MemOffset_t for sizes cmd/tlm messages - perhaps renaming it to CFE_ES_MemOffset_Atom_t to clarify it is intended for this purpose.

[skliper]

... using the standard size_t internally and using the CFE_ES_MemOffset_t for sizes cmd/tlm messages ...

Sounds good to me.

[jphickey]

Another potential compromise to move in that direction without jumping all-in would be to add another typedef in ES for this, i.e.

typedef size_t CFE_ES_Size_t;

Then update all CFE APIs to use this typedef for sizes. Mem Pool code was already done this way in my previous commit, but this can be generalized to the rest of the API.

Upside: Gives users an easy escape if their apps don't compile nicely with 64-bit sizes - they can patch it back to uint32.
Downside: Yet another typedef.....

[skliper]

I vote against another typedef, I'd rather jump all-in.