Skip to content

Commit

Permalink
Fix nasa#113, replace acknack count union
Browse files Browse the repository at this point in the history 
Using a union for the ack/nak counts is somewhat dangerous because
undefined behavior will occur if accessed improperly.  In this case
there is no need to have separate representations of the counter,
they are both limited to the "unit8" range, so use a uint8.
  • Loading branch information
@jphickey
jphickey committed Dec 10, 2021
1 parent a894069 commit f9c00c6
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 60 deletions.
12 changes: 3 additions & 9 deletions fsw/src/cf_cfdp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,10 @@ typedef struct CF_Poll
bool timer_set;
} CF_Poll_t;

typedef union CF_RxTxCounters
{
unsigned ack;
uint8 nak;
} CF_RxTxCounters_t;

typedef struct CF_TxS2_Data
{
uint8 fin_cc; /* remember the cc in the received fin pdu to echo in eof-fin */
CF_RxTxCounters_t counter;
uint8 fin_cc; /* remember the cc in the received fin pdu to echo in eof-fin */
uint8 acknak_count;
} CF_TxS2_Data_t;

typedef struct CF_TxState_Data
Expand All @@ -162,7 +156,7 @@ typedef struct CF_RxS2_Data
CF_CFDP_FinDeliveryCode_t dc;
CF_CFDP_FinFileStatus_t fs;
uint8 eof_cc; /* remember the cc in the received eof pdu to echo in eof-ack */
CF_RxTxCounters_t counter;
uint8 acknak_count;
} CF_RxS2_Data_t;

typedef struct CF_RxState_Data
Expand Down
14 changes: 7 additions & 7 deletions fsw/src/cf_cfdp_r.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,15 +181,15 @@ static void CF_CFDP_R2_Complete(CF_Transaction_t *t, int ok_to_send_nak)

if (send_nak && ok_to_send_nak)
{
if (++t->state_data.r.r2.counter.nak == CF_AppData.config_table->nak_limit)
if (++t->state_data.r.r2.acknak_count >= CF_AppData.config_table->nak_limit)
{
CFE_EVS_SendEvent(CF_EID_ERR_CFDP_R_NAK_LIMIT, CFE_EVS_EventType_ERROR, "CF R%d(%u:%u): nak limited reach",
(t->state == CF_TxnState_R2), t->history->src_eid, t->history->seq_num);
send_fin = 1;
++CF_AppData.hk.channel_hk[t->chan_num].counters.fault.nak_limit;
t->history->cc = CF_CFDP_ConditionCode_NAK_LIMIT_REACHED; /* don't use CF_CFDP_R2_SetCc because many places
in this function set send_fin */
t->state_data.r.r2.counter.nak = 0; /* reset for fin/ack */
t->state_data.r.r2.acknak_count = 0; /* reset for fin/ack */
}
else
{
Expand Down Expand Up @@ -493,7 +493,7 @@ static void CF_CFDP_R2_SubstateRecvFileData(CF_Transaction_t *t, const CF_CFDP_P
CF_CFDP_ArmAckTimer(t); /* re-arm ack timer, since we got data */
}

t->state_data.r.r2.counter.nak = 0;
t->state_data.r.r2.acknak_count = 0;

return;

Expand Down Expand Up @@ -916,9 +916,9 @@ static void CF_CFDP_R2_RecvMd(CF_Transaction_t *t, const CF_CFDP_PduHeader_t *ph
}
}

t->flags.rx.md_recv = 1;
t->state_data.r.r2.counter.nak = 0; /* in case part of nak */
CF_CFDP_R2_Complete(t, 1); /* check for completion now that md is received */
t->flags.rx.md_recv = 1;
t->state_data.r.r2.acknak_count = 0; /* in case part of nak */
CF_CFDP_R2_Complete(t, 1); /* check for completion now that md is received */
}
else
{
Expand Down Expand Up @@ -1147,7 +1147,7 @@ void CF_CFDP_R_Tick(CF_Transaction_t *t, int *cont /* unused */)
}
else if (t->state_data.r.sub_state == CF_RxSubState_WAIT_FOR_FIN_ACK)
{
if (++t->state_data.r.r2.counter.ack == CF_AppData.config_table->ack_limit)
if (++t->state_data.r.r2.acknak_count >= CF_AppData.config_table->ack_limit)
{
CFE_EVS_SendEvent(CF_EID_ERR_CFDP_R_ACK_LIMIT, CFE_EVS_EventType_ERROR,
"CF R2(%u:%u): ack limit reached, no fin-ack", t->history->src_eid,
Expand Down
2 changes: 1 addition & 1 deletion fsw/src/cf_cfdp_s.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ void CF_CFDP_S_Tick(CF_Transaction_t *t, int *cont /* unused */)
{
if (t->state_data.s.sub_state == CF_TxSubState_WAIT_FOR_EOF_ACK)
{
if (++t->state_data.s.s2.counter.ack == CF_AppData.config_table->ack_limit)
if (++t->state_data.s.s2.acknak_count >= CF_AppData.config_table->ack_limit)
{
CFE_EVS_SendEvent(CF_EID_ERR_CFDP_S_ACK_LIMIT, CFE_EVS_EventType_ERROR,
"CF S2(%u:%u), ack limit reached, no eof-ack", t->history->src_eid,
Expand Down
71 changes: 34 additions & 37 deletions unit-test/cf_cfdp_r_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,9 +433,9 @@ void Test_CF_CFDP_R2_Complete_Given_t_Sets_send_nak_To_1_Given_ok_to_send_nak_Is
arg_t->state_data.r.sub_state = initial_sub_state;
arg_t->flags.rx.md_recv = 0;

CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_Except(0);
arg_t->state_data.r.r2.counter.nak = CF_AppData.config_table->nak_limit - 1;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_Except(0);
arg_t->state_data.r.r2.acknak_count = CF_AppData.config_table->nak_limit - 1;

/* Act */
CF_CFDP_R2_Complete(arg_t, arg_ok_to_send_nak);
Expand Down Expand Up @@ -470,9 +470,9 @@ void Test_CF_CFDP_R2_Complete_Given_t_Sets_send_nak_To_1_Given_ok_to_send_nak_Is
arg_t->flags.rx.md_recv = 0;
arg_t->flags.rx.send_nak = 0;

CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.counter.nak = CF_AppData.config_table->nak_limit - 2;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.acknak_count = CF_AppData.config_table->nak_limit - 2;

/* Act */
CF_CFDP_R2_Complete(arg_t, arg_ok_to_send_nak);
Expand Down Expand Up @@ -511,9 +511,9 @@ void Test_CF_CFDP_R2_Complete_Calls_CF_Chunks_ComputeGaps_Returns_non0_Set_send_

UT_SetDefaultReturnValue(UT_KEY(CF_ChunkList_ComputeGaps), Any_int_Except(0));

CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.counter.nak = CF_AppData.config_table->nak_limit - 2;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.acknak_count = CF_AppData.config_table->nak_limit - 2;

/* Act */
CF_CFDP_R2_Complete(arg_t, arg_ok_to_send_nak);
Expand Down Expand Up @@ -553,9 +553,9 @@ void Test_CF_CFDP_R2_Complete_Calls_CF_Chunks_ComputeGaps_Returns_non0_Set_send_
UT_SetDefaultReturnValue(UT_KEY(CF_ChunkList_ComputeGaps), 0);
arg_t->flags.rx.eof_recv = 0;

CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.counter.nak = CF_AppData.config_table->nak_limit - 2;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.acknak_count = CF_AppData.config_table->nak_limit - 2;

/* Act */
CF_CFDP_R2_Complete(arg_t, arg_ok_to_send_nak);
Expand Down Expand Up @@ -596,9 +596,9 @@ void Test_CF_CFDP_R2_Complete_Calls_CF_Chunks_ComputeGaps_Returns_non0_Set_send_
UT_SetDefaultReturnValue(UT_KEY(CF_ChunkList_ComputeGaps), 0);
arg_t->flags.rx.eof_recv = 1;

CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.counter.nak = CF_AppData.config_table->nak_limit - 2;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = Any_uint8_GreaterThan(1);
arg_t->state_data.r.r2.acknak_count = CF_AppData.config_table->nak_limit - 2;

/* Act */
CF_CFDP_R2_Complete(arg_t, arg_ok_to_send_nak);
Expand Down Expand Up @@ -1302,13 +1302,13 @@ void Test_CF_CFDP_R2_SubstateRecvEof_CallTo_CF_CFDP_R_SubstateRecvEof_Returns_R_
CF_History_t dummy_history;
CF_ConfigTable_t dummy_config_table;

arg_t->history = &dummy_history;
arg_t->history->cc = CF_CFDP_ConditionCode_NO_ERROR;
arg_t->flags.rx.md_recv = 0;
arg_t->state_data.r.r2.counter.nak = 0;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = UINT8_MAX;
arg_t->state_data.r.sub_state = Any_uint8_Except(CF_RxSubState_FILEDATA);
arg_t->history = &dummy_history;
arg_t->history->cc = CF_CFDP_ConditionCode_NO_ERROR;
arg_t->flags.rx.md_recv = 0;
arg_t->state_data.r.r2.acknak_count = 0;
CF_AppData.config_table = &dummy_config_table;
CF_AppData.config_table->nak_limit = UINT8_MAX;
arg_t->state_data.r.sub_state = Any_uint8_Except(CF_RxSubState_FILEDATA);

/* Act */
CF_CFDP_R2_SubstateRecvEof(arg_t, arg_ph);
Expand Down Expand Up @@ -1555,7 +1555,7 @@ void Test_CF_CFDP_R2_SubstateRecvFileData_t_flags_rx_fd_nak_sent_Is_0_And_t_flag

UT_SetHandlerFunction(UT_KEY(CF_CFDP_RecvFd), Handler_int_ForcedReturnOnly, &forced_return_CF_CFDP_RecvFd);

arg_t->state_data.r.r2.counter.nak = Any_uint8_Except(0);
arg_t->state_data.r.r2.acknak_count = Any_uint8_Except(0);

arg_t->flags.rx.fd_nak_sent = 0;

Expand Down Expand Up @@ -1598,8 +1598,7 @@ void Test_CF_CFDP_R2_SubstateRecvFileData_t_flags_rx_fd_nak_sent_Is_0_And_t_flag
/* Assert */
UtAssert_STUB_COUNT(CF_CFDP_RecvFd, 1);
UtAssert_STUB_COUNT(CF_ChunkListAdd, 1);
UtAssert_True(arg_t->state_data.r.r2.counter.nak == 0, "t->state_data.r.r2.counter.nak is %u and should be 0",
arg_t->state_data.r.r2.counter.nak);
UtAssert_ZERO(arg_t->state_data.r.r2.acknak_count);
/* Assert for CF_CFDP_R_ProcessFd - 3 calls, one in function two in STATIC_CAST */
UtAssert_STUB_COUNT(CF_HeaderSize, 3);
/* Assert for CF_CFDP_R2_Reset via CF_CFDP_R1_Reset */
Expand All @@ -1620,7 +1619,7 @@ void Test_CF_CFDP_R2_SubstateRecvFileData_t_flags_rx_fd_nak_sent_Is_1_Call_CF_CF

UT_SetHandlerFunction(UT_KEY(CF_CFDP_RecvFd), Handler_int_ForcedReturnOnly, &forced_return_CF_CFDP_RecvFd);

arg_t->state_data.r.r2.counter.nak = Any_uint8_Except(0);
arg_t->state_data.r.r2.acknak_count = Any_uint8_Except(0);

arg_t->flags.rx.fd_nak_sent = 1;

Expand Down Expand Up @@ -1666,8 +1665,7 @@ void Test_CF_CFDP_R2_SubstateRecvFileData_t_flags_rx_fd_nak_sent_Is_1_Call_CF_CF
/* Assert */
UtAssert_STUB_COUNT(CF_CFDP_RecvFd, 1);
UtAssert_STUB_COUNT(CF_ChunkListAdd, 1);
UtAssert_True(arg_t->state_data.r.r2.counter.nak == 0, "t->state_data.r.r2.counter.nak is %u and should be 0",
arg_t->state_data.r.r2.counter.nak);
UtAssert_ZERO(arg_t->state_data.r.r2.acknak_count);
/* Assert for CF_CFDP_R_ProcessFd - 3 calls, one in function two in STATIC_CAST */
UtAssert_STUB_COUNT(CF_HeaderSize, 3);
/* Assert for CF_CFDP_R2_Reset via CF_CFDP_R1_Reset */
Expand All @@ -1688,7 +1686,7 @@ void Test_CF_CFDP_R2_SubstateRecvFileData_t_flags_rx_fd_nak_sent_Is_0_And_t_flag

UT_SetHandlerFunction(UT_KEY(CF_CFDP_RecvFd), Handler_int_ForcedReturnOnly, &forced_return_CF_CFDP_RecvFd);

arg_t->state_data.r.r2.counter.nak = Any_uint8_Except(0);
arg_t->state_data.r.r2.acknak_count = Any_uint8_Except(0);

arg_t->flags.rx.fd_nak_sent = 0;

Expand Down Expand Up @@ -1732,8 +1730,7 @@ void Test_CF_CFDP_R2_SubstateRecvFileData_t_flags_rx_fd_nak_sent_Is_0_And_t_flag
UtAssert_STUB_COUNT(CF_CFDP_RecvFd, 1);
UtAssert_STUB_COUNT(CF_ChunkListAdd, 1);
UtAssert_STUB_COUNT(CF_CFDP_ArmAckTimer, 1);
UtAssert_True(arg_t->state_data.r.r2.counter.nak == 0, "t->state_data.r.r2.counter.nak is %u and should be 0",
arg_t->state_data.r.r2.counter.nak);
UtAssert_ZERO(arg_t->state_data.r.r2.acknak_count);
/* Assert for CF_CFDP_R_ProcessFd - 3 calls, one in function two in STATIC_CAST */
UtAssert_STUB_COUNT(CF_HeaderSize, 3);
/* Assert for CF_CFDP_R2_Reset via CF_CFDP_R1_Reset */
Expand Down Expand Up @@ -3206,8 +3203,8 @@ void Test_CF_CFDP_R2_RecvMd_Given_t_flags_rx_md_recv_Is_0_CallTo_CF_CFDP_RecvMd_
UT_SetDataBuffer(UT_KEY(CF_WrappedOpenCreate), &context_CF_WrappedOpenCreate, sizeof(context_CF_WrappedOpenCreate),
false);

arg_t->flags.rx.md_recv = 0;
arg_t->state_data.r.r2.counter.nak = Any_uint8_Except(0);
arg_t->flags.rx.md_recv = 0;
arg_t->state_data.r.r2.acknak_count = Any_uint8_Except(0);

/* Arrange for CF_CFDP_R2_Complete */
arg_t->history->cc = CF_CFDP_ConditionCode_NO_ERROR;
Expand All @@ -3224,8 +3221,7 @@ void Test_CF_CFDP_R2_RecvMd_Given_t_flags_rx_md_recv_Is_0_CallTo_CF_CFDP_RecvMd_
UtAssert_STUB_COUNT(CF_WrappedOpenCreate, 1);
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 0);
UtAssert_True(arg_t->flags.rx.md_recv == 1, "t->flags.rx.md_recv is %u and should be 1", arg_t->flags.rx.md_recv);
UtAssert_True(arg_t->state_data.r.r2.counter.nak == 0, "t->state_data.r.r2.counter.nak is %u and should be 0",
arg_t->state_data.r.r2.counter.nak);
UtAssert_ZERO(arg_t->state_data.r.r2.acknak_count);
/* Assert for CF_CFDP_R2_Complete */
UtAssert_True(arg_t->state_data.r.sub_state == CF_RxSubState_FILEDATA,
"t->state_data.r.sub_state is %u and should be %u (CF_RxSubState_FILEDATA)",
Expand Down Expand Up @@ -4499,7 +4495,8 @@ void Test_CF_CFDP_R_Tick_NothingElseSet_ack_timer_armed_Is_1_CAllTo_CF_Timer_Exp

arg_t->flags.rx.complete = 1;

arg_t->state_data.r.sub_state = CF_RxSubState_WAIT_FOR_FIN_ACK;
arg_t->state_data.r.sub_state = CF_RxSubState_WAIT_FOR_FIN_ACK;
arg_t->state_data.r.r2.acknak_count = 0;

/* Arrange for CF_CFDP_R2_Complete */
CF_History_t dummy_history;
Expand Down Expand Up @@ -4560,7 +4557,7 @@ void Test_CF_CFDP_R_Tick_NothingElseSet_ack_timer_armed_Is_1_CAllTo_CF_Timer_Exp

CF_AppData.config_table->ack_limit = Any_uint8_Except(0); /* Any_uint8 used for small value in test */

arg_t->state_data.r.r2.counter.ack = CF_AppData.config_table->ack_limit - 1; /* - 1 code increases value */
arg_t->state_data.r.r2.acknak_count = CF_AppData.config_table->ack_limit - 1; /* - 1 code increases value */

UT_SetDataBuffer(UT_KEY(CFE_EVS_SendEvent), &EventID, sizeof(EventID), false);
arg_t->history = &dummy_history;
Expand Down
14 changes: 8 additions & 6 deletions unit-test/cf_cfdp_s_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1303,8 +1303,10 @@ void Test_CF_CFDP_S_CheckAndRespondNak_CallTo_CF_Chunks_GetFirstChunkReturned_no
CF_CFDP_SendFd_context_t context_CF_CFDP_SendFd;

dummy_c->offset = 0; /* dummy_c->offset = 0 used so that (foffs+bytes_to_read)<=t->fsize is never false */
arg_t->fsize = 512;
arg_t->foffs = 0;

dummy_config_table.outgoing_file_chunk_size = arg_t->foffs;
dummy_config_table.outgoing_file_chunk_size = dummy_c->size;
CF_AppData.config_table = &dummy_config_table;
arg_t->history = &dummy_history;

Expand Down Expand Up @@ -3470,7 +3472,7 @@ void Test_CF_CFDP_S_Tick_ArmedTimerExpiredAnd_sub_state_EqTo_SEND_WAIT_FOR_EOF_A
arg_t->flags.com.ack_timer_armed = 1;
arg_t->state_data.s.sub_state = CF_TxSubState_WAIT_FOR_EOF_ACK;

arg_t->state_data.s.s2.counter.ack = dummy_ack_limit - 1;
arg_t->state_data.s.s2.acknak_count = dummy_ack_limit - 1;

CF_AppData.config_table = &dummy_config_table;
CF_AppData.hk.channel_hk[arg_t->chan_num].counters.fault.inactivity_timer = initial_inactivity_timer;
Expand Down Expand Up @@ -3540,7 +3542,7 @@ void Test_CF_CFDP_S_Tick_ArmedTimerExpiredAnd_sub_state_EqTo_SEND_WAIT_FOR_EOF_A
int *arg_cont = NULL;
uint16 initial_inactivity_timer = Any_uint16();
uint16 initial_fault_ack_limit = Any_uint16();
uint8 dummy_ack_limit = Any_uint8();
uint8 dummy_ack_limit = 5;
CF_Timer_t *context_CF_Timer_Expired[2];
CF_Timer_t *context_CF_Timer_Tick;

Expand All @@ -3550,7 +3552,7 @@ void Test_CF_CFDP_S_Tick_ArmedTimerExpiredAnd_sub_state_EqTo_SEND_WAIT_FOR_EOF_A
arg_t->flags.com.ack_timer_armed = 1;
arg_t->state_data.s.sub_state = CF_TxSubState_WAIT_FOR_EOF_ACK;

arg_t->state_data.s.s2.counter.ack = Any_uint8_Except(dummy_ack_limit - 1);
arg_t->state_data.s.s2.acknak_count = 0;

CF_AppData.config_table = &dummy_config_table;
CF_AppData.hk.channel_hk[arg_t->chan_num].counters.fault.inactivity_timer = initial_inactivity_timer;
Expand Down Expand Up @@ -3620,7 +3622,7 @@ void Test_CF_CFDP_S_Tick_ArmedTimerExpiredAnd_sub_state_EqTo_SEND_WAIT_FOR_EOF_A
arg_t->flags.com.ack_timer_armed = 1;
arg_t->state_data.s.sub_state = CF_TxSubState_WAIT_FOR_EOF_ACK;

arg_t->state_data.s.s2.counter.ack = Any_uint8_Except(dummy_ack_limit - 1);
arg_t->state_data.s.s2.acknak_count = Any_uint8_Except(dummy_ack_limit - 1);

CF_AppData.config_table = &dummy_config_table;
CF_AppData.hk.channel_hk[arg_t->chan_num].counters.fault.inactivity_timer = initial_inactivity_timer;
Expand Down Expand Up @@ -3701,7 +3703,7 @@ void Test_CF_CFDP_S_Tick_ArmedTimerExpiredAnd_sub_state_EqTo_SEND_WAIT_FOR_EOF_A
arg_t->flags.com.ack_timer_armed = 1;
arg_t->state_data.s.sub_state = CF_TxSubState_WAIT_FOR_EOF_ACK;

arg_t->state_data.s.s2.counter.ack = Any_uint8_Except(dummy_ack_limit - 1);
arg_t->state_data.s.s2.acknak_count = Any_uint8_Except(dummy_ack_limit - 1);

CF_AppData.config_table = &dummy_config_table;
CF_AppData.hk.channel_hk[arg_t->chan_num].counters.fault.inactivity_timer = initial_inactivity_timer;
Expand Down

0 comments on commit f9c00c6

Please sign in to comment.