From 15e43163f8507fc0733c41fd1a9a885fd99a6f90 Mon Sep 17 00:00:00 2001 From: Joseph Hickey Date: Tue, 21 Mar 2023 09:53:51 -0400 Subject: [PATCH] Fix #71, table processing fixes First check that table name is null-terminated before comparing any strings, then the normal strcmp() can be safely used. This reverses the direction of the inner check loop, so it is reading entries that have been already validated otherwise, rather than reading entries that have not yet been checked at all. --- fsw/inc/cs_events.h | 12 +++++++ fsw/src/cs_table_processing.c | 38 ++++++++++++++-------- unit-test/cs_table_processing_tests.c | 46 +++++++++++++-------------- 3 files changed, 59 insertions(+), 37 deletions(-) diff --git a/fsw/inc/cs_events.h b/fsw/inc/cs_events.h index afbeb37..48228f8 100644 --- a/fsw/inc/cs_events.h +++ b/fsw/inc/cs_events.h @@ -1974,6 +1974,18 @@ */ #define CS_BKGND_COMPUTE_PROG_INF_EID 153 +/** + * \brief CS Apps Table Validate Failed Illegal State With Long Name Event ID + * + * \par Type: ERROR + * + * \par Cause: + * + * This event message is issued when CS validation for the Apps definition table finds an entry + * that contains a non-terminated name field. + */ +#define CS_VAL_APP_DEF_TBL_LONG_NAME_ERR_EID 154 + /**@}*/ #endif diff --git a/fsw/src/cs_table_processing.c b/fsw/src/cs_table_processing.c index 43c6d6a..59c77f4 100644 --- a/fsw/src/cs_table_processing.c +++ b/fsw/src/cs_table_processing.c @@ -327,16 +327,16 @@ int32 CS_ValidateTablesChecksumDefinitionTable(void *TblPtr) /* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ int32 CS_ValidateAppChecksumDefinitionTable(void *TblPtr) { - int32 Result = CFE_SUCCESS; - CS_Def_App_Table_Entry_t *StartOfTable = NULL; - CS_Def_App_Table_Entry_t *OuterEntry = NULL; - int32 OuterLoop = 0; - int32 InnerLoop = 0; - uint32 StateField = 0; - int32 GoodCount = 0; - int32 BadCount = 0; - int32 EmptyCount = 0; - bool DuplicateFound = false; + int32 Result = CFE_SUCCESS; + CS_Def_App_Table_Entry_t *StartOfTable; + CS_Def_App_Table_Entry_t *OuterEntry; + int32 OuterLoop; + int32 InnerLoop; + uint32 StateField; + int32 GoodCount = 0; + int32 BadCount = 0; + int32 EmptyCount = 0; + bool DuplicateFound; StartOfTable = (CS_Def_App_Table_Entry_t *)TblPtr; @@ -346,8 +346,18 @@ int32 CS_ValidateAppChecksumDefinitionTable(void *TblPtr) StateField = OuterEntry->State; - /* Check for non-zero length for table name */ - if (strlen(OuterEntry->Name) != 0) + if (memchr(OuterEntry->Name, 0, sizeof(OuterEntry->Name)) == NULL) + { + /* Not null-terminated, name is too long */ + if (Result != CS_TABLE_ERROR) + { + CFE_EVS_SendEvent(CS_VAL_APP_DEF_TBL_LONG_NAME_ERR_EID, CFE_EVS_EventType_ERROR, + "CS Apps Table Validate: Unterminated Name found at entry %d", (int)OuterLoop); + Result = CS_TABLE_ERROR; + } + BadCount++; + } + else if (strlen(OuterEntry->Name) != 0) { /* Verify valid state definition */ if (((StateField == CS_STATE_EMPTY) || (StateField == CS_STATE_ENABLED) || @@ -356,9 +366,9 @@ int32 CS_ValidateAppChecksumDefinitionTable(void *TblPtr) DuplicateFound = false; /* Verify the name field is not duplicated */ - for (InnerLoop = OuterLoop + 1; InnerLoop < CS_MAX_NUM_APP_TABLE_ENTRIES; InnerLoop++) + for (InnerLoop = 0; InnerLoop < OuterLoop; InnerLoop++) { - if (strncmp(OuterEntry->Name, (&StartOfTable[InnerLoop])->Name, CFE_TBL_MAX_FULL_NAME_LEN) == 0) + if (strcmp(OuterEntry->Name, StartOfTable[InnerLoop].Name) == 0) { if (DuplicateFound != true) { diff --git a/unit-test/cs_table_processing_tests.c b/unit-test/cs_table_processing_tests.c index 2b712ce..c9be2b3 100644 --- a/unit-test/cs_table_processing_tests.c +++ b/unit-test/cs_table_processing_tests.c @@ -998,35 +998,18 @@ void CS_ValidateTablesChecksumDefinitionTable_Test_CsTableError(void) void CS_ValidateAppChecksumDefinitionTable_Test_Nominal(void) { - int32 Result; - int32 strCmpResult; - char ExpectedEventString[CFE_MISSION_EVS_MAX_MESSAGE_LENGTH]; - - snprintf(ExpectedEventString, CFE_MISSION_EVS_MAX_MESSAGE_LENGTH, - "CS Apps Table verification results: good = %%d, bad = %%d, unused = %%d"); - - CS_AppData.DefAppTblPtr[0].State = CS_STATE_ENABLED; /* All other states are empty by default, and so this test also - covers CS_STATE_EMPTY branch */ - - strncpy(CS_AppData.DefAppTblPtr[0].Name, "name", 10); + CS_AppData.DefAppTblPtr[0].State = CS_STATE_ENABLED; + strncpy(CS_AppData.DefAppTblPtr[0].Name, "app1", sizeof(CS_AppData.DefAppTblPtr[0].Name)); + CS_AppData.DefAppTblPtr[1].State = CS_STATE_ENABLED; + strncpy(CS_AppData.DefAppTblPtr[1].Name, "app2", sizeof(CS_AppData.DefAppTblPtr[1].Name)); /* Execute the function being tested */ - Result = CS_ValidateAppChecksumDefinitionTable(CS_AppData.DefAppTblPtr); + UtAssert_INT32_EQ(CS_ValidateAppChecksumDefinitionTable(CS_AppData.DefAppTblPtr), CFE_SUCCESS); /* Verify results */ + UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 1); UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventID, CS_VAL_APP_INF_EID); UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventType, CFE_EVS_EventType_INFORMATION); - - strCmpResult = strncmp(ExpectedEventString, context_CFE_EVS_SendEvent[0].Spec, CFE_MISSION_EVS_MAX_MESSAGE_LENGTH); - - UtAssert_True(strCmpResult == 0, "Event string matched expected result, '%s'", context_CFE_EVS_SendEvent[0].Spec); - - UtAssert_True(Result == CFE_SUCCESS, "Result == CFE_SUCCESS"); - - call_count_CFE_EVS_SendEvent = UT_GetStubCount(UT_KEY(CFE_EVS_SendEvent)); - - UtAssert_True(call_count_CFE_EVS_SendEvent == 1, "CFE_EVS_SendEvent was called %u time(s), expected 1", - call_count_CFE_EVS_SendEvent); } void CS_ValidateAppChecksumDefinitionTable_Test_DuplicateNameStateEmpty(void) @@ -1261,6 +1244,21 @@ void CS_ValidateAppChecksumDefinitionTable_Test_IllegalStateEmptyName(void) call_count_CFE_EVS_SendEvent); } +void CS_ValidateAppChecksumDefinitionTable_Test_LongName(void) +{ + memset(CS_AppData.DefAppTblPtr[0].Name, 'x', sizeof(CS_AppData.DefAppTblPtr[0].Name)); + memset(CS_AppData.DefAppTblPtr[1].Name, 'y', sizeof(CS_AppData.DefAppTblPtr[1].Name)); + + UtAssert_INT32_EQ(CS_ValidateAppChecksumDefinitionTable(CS_AppData.DefAppTblPtr), CS_TABLE_ERROR); + + /* Verify results */ + UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 2); + UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventID, CS_VAL_APP_DEF_TBL_LONG_NAME_ERR_EID); + UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventType, CFE_EVS_EventType_ERROR); + UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[1].EventID, CS_VAL_APP_INF_EID); + UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[1].EventType, CFE_EVS_EventType_INFORMATION); +} + void CS_ValidateAppChecksumDefinitionTable_Test_TableErrorResult(void) { int32 Result; @@ -3388,6 +3386,8 @@ void UtTest_Setup(void) "CS_ValidateAppChecksumDefinitionTable_Test_IllegalStateField"); UtTest_Add(CS_ValidateAppChecksumDefinitionTable_Test_IllegalStateEmptyName, CS_Test_Setup, CS_Test_TearDown, "CS_ValidateAppChecksumDefinitionTable_Test_IllegalStateEmptyName"); + UtTest_Add(CS_ValidateAppChecksumDefinitionTable_Test_LongName, CS_Test_Setup, CS_Test_TearDown, + "CS_ValidateAppChecksumDefinitionTable_Test_LongName"); UtTest_Add(CS_ValidateAppChecksumDefinitionTable_Test_TableErrorResult, CS_Test_Setup, CS_Test_TearDown, "CS_ValidateAppChecksumDefinitionTable_Test_TableErrorResult"); UtTest_Add(CS_ValidateAppChecksumDefinitionTable_Test_UndefTableErrorResult, CS_Test_Setup, CS_Test_TearDown,