nasa / CF

The Core Flight System (cFS) CFDP application.
Apache License 2.0
79 stars 45 forks source link

CF method CF_CFDP_MsgOutGet returns 0x10 when it should be NULL #28

Closed jphickey closed 2 years ago

jphickey commented 2 years ago

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1726] CF method CF_CFDP_MsgOutGet returns 0x10 when it should be NULL Originally submitted by: Gibson, Alan S. (GSFC-5870) on Fri Sep 10 14:28:59 2021

Original Description: CF_CFDP_MsgOutGet has a path where it will return a 0x10 if the CF_AppData.engine.out.msg is NULL. There are two if blocks after the msg NULL verification that if neither come back true the assignment on line 382 of cf_cfdp.c:

ret = &((pdu_s_msg_t*)CF_AppData.engine.out.msg)->ph;

will make ret == 0x10, not NULL.
The description of the return statement shows that this is not correct:

\retstmt Pointer to a pdu_header_t within a software bus buffer on success. Otherwise NULL.

jphickey commented 2 years ago

Imported comment by internal user on Mon Sep 27 10:06:10 2021

(link removed)

This line does not check the return result from CFE_MSG_Init and continues on as though the value put into &CF_AppData.engine.out.msg->Msg is good even if it failed. If it comes back NULL, it will return a value of 0x10 because ph is 0x10 away from the base pointer in the message type, 0x10 from 0x00.

It is unknown if this situation can happen or not, but the unit tests ran into it.

jphickey commented 2 years ago

I noted this as well in my testing, it appears this can happen any time the "frozen" or "suspended" flags are set true. This causes it to skip the actual msg allocation, so the pointer is NULL, but it still executes this line:

https://github.com/nasa/CF/blob/a894069439316ef89ad7751f3a03036930158a07/fsw/src/cf_cfdp.c#L422

This means the function will return something that is not NULL but also not valid (0x10 is likely when using the default MSG implementation where telemetry header size is 16 bytes).

jphickey commented 2 years ago

Should definitely be fixed....!