From 010330fc4c2d25b6eaace8e4c990551a7d3ab13f Mon Sep 17 00:00:00 2001 From: jdfiguer Date: Mon, 10 Jun 2024 15:39:51 -0400 Subject: [PATCH] Fix #92, Adds static analysis comments, replaces strncpy and strlen This commit addresses issues flagged during static analysis by: - Adding JSC 2.1 disposition comments. - Replacing strncpy with snprintf to enhance safety and compliance. - Replacing strlen with MM_strnlen. --- fsw/src/mm_app.c | 10 ++++---- fsw/src/mm_dump.c | 10 +++++--- fsw/src/mm_utils.c | 23 ++++++++++++++++- fsw/src/mm_utils.h | 17 +++++++++++++ unit-test/mm_app_tests.c | 42 +++++++++++++++++++++++--------- unit-test/mm_utils_tests.c | 37 ++++++++++++++++++++++++++++ unit-test/stubs/mm_utils_stubs.c | 8 ++++++ 7 files changed, 126 insertions(+), 21 deletions(-) diff --git a/fsw/src/mm_app.c b/fsw/src/mm_app.c index f099a04..0c6ad39 100644 --- a/fsw/src/mm_app.c +++ b/fsw/src/mm_app.c @@ -455,7 +455,7 @@ bool MM_LookupSymbolCmd(const CFE_SB_Buffer_t *BufPtr) /* ** Check if the symbol name string is a nul string */ - if (strlen(SymName) == 0) + if (MM_strnlen(SymName, OS_MAX_SYM_LEN) == 0) { CFE_EVS_SendEvent(MM_SYMNAME_NUL_ERR_EID, CFE_EVS_EventType_ERROR, "NUL (empty) string specified as symbol name"); @@ -482,7 +482,7 @@ bool MM_LookupSymbolCmd(const CFE_SB_Buffer_t *BufPtr) "Symbolic address can't be resolved: Name = '%s'", SymName); } - } /* end strlen(CmdPtr->Payload.SymName) == 0 else */ + } /* end MM_strnlen(CmdPtr->Payload.SymName, OS_MAX_SYM_LEN) == 0 else */ return Result; } @@ -508,7 +508,7 @@ bool MM_SymTblToFileCmd(const CFE_SB_Buffer_t *BufPtr) /* ** Check if the filename string is a nul string */ - if (strlen(FileName) == 0) + if (MM_strnlen(FileName, OS_MAX_PATH_LEN) == 0) { CFE_EVS_SendEvent(MM_SYMFILENAME_NUL_ERR_EID, CFE_EVS_EventType_ERROR, "NUL (empty) string specified as symbol dump file name"); @@ -520,7 +520,7 @@ bool MM_SymTblToFileCmd(const CFE_SB_Buffer_t *BufPtr) { /* Update telemetry */ MM_AppData.HkPacket.Payload.LastAction = MM_SYMTBL_SAVE; - strncpy(MM_AppData.HkPacket.Payload.FileName, FileName, OS_MAX_PATH_LEN); + snprintf(MM_AppData.HkPacket.Payload.FileName, OS_MAX_PATH_LEN, "%s", FileName); CFE_EVS_SendEvent(MM_SYMTBL_TO_FILE_INF_EID, CFE_EVS_EventType_INFORMATION, "Symbol Table Dump to File Started: Name = '%s'", FileName); @@ -533,7 +533,7 @@ bool MM_SymTblToFileCmd(const CFE_SB_Buffer_t *BufPtr) FileName); } - } /* end strlen(FileName) == 0 else */ + } /* end MM_strnlen(FileName, OS_MAX_PATH_LEN) == 0 else */ return Result; } diff --git a/fsw/src/mm_dump.c b/fsw/src/mm_dump.c index cc6bcb6..8943055 100644 --- a/fsw/src/mm_dump.c +++ b/fsw/src/mm_dump.c @@ -318,7 +318,7 @@ bool MM_DumpMemToFileCmd(const CFE_SB_Buffer_t *BufPtr) ** Update last action statistics */ MM_AppData.HkPacket.Payload.LastAction = MM_DUMP_TO_FILE; - strncpy(MM_AppData.HkPacket.Payload.FileName, FileName, OS_MAX_PATH_LEN); + snprintf(MM_AppData.HkPacket.Payload.FileName, OS_MAX_PATH_LEN, "%s", FileName); MM_AppData.HkPacket.Payload.MemType = CmdPtr->Payload.MemType; MM_AppData.HkPacket.Payload.Address = SrcAddress; MM_AppData.HkPacket.Payload.BytesProcessed = CmdPtr->Payload.NumOfBytes; @@ -512,7 +512,7 @@ bool MM_DumpInEventCmd(const CFE_SB_Buffer_t *BufPtr) */ CFE_SB_MessageStringGet(&EventString[EventStringTotalLength], HeaderString, NULL, sizeof(EventString) - EventStringTotalLength, sizeof(HeaderString)); - EventStringTotalLength = strlen(EventString); + EventStringTotalLength = MM_strnlen(EventString, CFE_MISSION_EVS_MAX_MESSAGE_LENGTH); /* ** Build dump data string @@ -522,10 +522,12 @@ bool MM_DumpInEventCmd(const CFE_SB_Buffer_t *BufPtr) BytePtr = (uint8 *)DumpBuffer; for (i = 0; i < CmdPtr->Payload.NumOfBytes; i++) { + /* SAD: No need to check snprintf return; CFE_SB_MessageStringGet() handles safe concatenation and + * prevents overflow */ snprintf(TempString, MM_DUMPINEVENT_TEMP_CHARS, "0x%02X ", *BytePtr); CFE_SB_MessageStringGet(&EventString[EventStringTotalLength], TempString, NULL, sizeof(EventString) - EventStringTotalLength, sizeof(TempString)); - EventStringTotalLength = strlen(EventString); + EventStringTotalLength = MM_strnlen(EventString, CFE_MISSION_EVS_MAX_MESSAGE_LENGTH); BytePtr++; } @@ -533,6 +535,8 @@ bool MM_DumpInEventCmd(const CFE_SB_Buffer_t *BufPtr) ** Append tail ** This adds up to 33 characters depending on pointer representation including the NUL terminator */ + /* SAD: No need to check snprintf return; CFE_SB_MessageStringGet() handles safe concatenation and + * prevents overflow */ snprintf(TempString, MM_DUMPINEVENT_TEMP_CHARS, "from address: %p", (void *)SrcAddress); CFE_SB_MessageStringGet(&EventString[EventStringTotalLength], TempString, NULL, sizeof(EventString) - EventStringTotalLength, sizeof(TempString)); diff --git a/fsw/src/mm_utils.c b/fsw/src/mm_utils.c index 3bc16bf..663973a 100644 --- a/fsw/src/mm_utils.c +++ b/fsw/src/mm_utils.c @@ -348,6 +348,7 @@ bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeI MaxSize = MM_MAX_FILL_DATA_RAM; } PSP_MemType = CFE_PSP_MEM_RAM; + /* SAD: No need to check snprintf return value; buffer size can store "MEM_RAM" without overflow */ snprintf(MemTypeStr, MM_MAX_MEM_TYPE_STR_LEN, "%s", "MEM_RAM"); break; case MM_EEPROM: @@ -364,6 +365,7 @@ bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeI MaxSize = MM_MAX_FILL_DATA_EEPROM; } PSP_MemType = CFE_PSP_MEM_EEPROM; + /* SAD: No need to check snprintf return value; buffer size can store "MEM_EEPROM" without overflow */ snprintf(MemTypeStr, MM_MAX_MEM_TYPE_STR_LEN, "%s", "MEM_EEPROM"); break; #ifdef MM_OPT_CODE_MEM32_MEMTYPE @@ -515,7 +517,7 @@ bool MM_ResolveSymAddr(MM_SymAddr_t *SymAddr, cpuaddr *ResolvedAddr) ** If the symbol name string is a nul string ** we use the offset as the absolute address */ - if (strlen(SymAddr->SymName) == 0) + if (MM_strnlen(SymAddr->SymName, OS_MAX_SYM_LEN) == 0) { *ResolvedAddr = SymAddr->Offset; Valid = true; @@ -586,3 +588,22 @@ int32 MM_ComputeCRCFromFile(osal_id_t FileHandle, uint32 *CrcPtr, uint32 TypeCRC return OS_Status; } + +/*---------------------------------------------------------------- + * + * Function: MM_strnlen + * + * Application-scope internal function + * See description in mm_utils.h for argument/return detail + * + *-----------------------------------------------------------------*/ +size_t MM_strnlen(const char *str, size_t maxlen) +{ + const char *end = memchr(str, 0, maxlen); + if (end != NULL) + { + /* actual length of string is difference */ + maxlen = end - str; + } + return maxlen; +} diff --git a/fsw/src/mm_utils.h b/fsw/src/mm_utils.h index 30a3b50..6d87ff4 100644 --- a/fsw/src/mm_utils.h +++ b/fsw/src/mm_utils.h @@ -224,4 +224,21 @@ bool MM_ResolveSymAddr(MM_SymAddr_t *SymAddr, cpuaddr *ResolvedAddr); */ int32 MM_ComputeCRCFromFile(osal_id_t FileHandle, uint32 *CrcPtr, uint32 TypeCRC); +/************************************************************************/ +/** @brief Calculates the length of a string up to a maximum length + * + * Purpose: Provides a local OSAL routine to get the functionality + * of the (non-C99) "strnlen()" function, via the + * C89/C99 standard "memchr()" function instead. + * + * @par Assumptions, External Events, and Notes: + * None + * + * @param str Pointer to the input string + * @param maxlen Maximum number of characters to check + * + * @returns Length of the string up to `maxlen` characters + */ +size_t MM_strnlen(const char *str, size_t maxlen); + #endif diff --git a/unit-test/mm_app_tests.c b/unit-test/mm_app_tests.c index 0f63da3..5740199 100644 --- a/unit-test/mm_app_tests.c +++ b/unit-test/mm_app_tests.c @@ -451,7 +451,8 @@ void MM_AppPipe_Test_NoopSuccess(void) MM_AppPipe(&UT_CmdBuf.Buf); /* Verify results */ - UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_NOOP, "MM_AppData.HkPacket.Payload.LastAction == MM_NOOP"); + UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_NOOP, + "MM_AppData.HkPacket.Payload.LastAction == MM_NOOP"); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.CmdCounter, 1); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.ErrCounter, 0); @@ -518,7 +519,8 @@ void MM_AppPipe_Test_ResetSuccess(void) MM_AppPipe(&UT_CmdBuf.Buf); /* Verify results */ - UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_RESET, "MM_AppData.HkPacket.Payload.LastAction == MM_RESET"); + UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_RESET, + "MM_AppData.HkPacket.Payload.LastAction == MM_RESET"); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.CmdCounter, 0); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.ErrCounter, 0); @@ -565,8 +567,8 @@ void MM_AppPipe_Test_ResetFail(void) void MM_AppPipe_Test_PeekSuccess(void) { - CFE_SB_MsgId_t TestMsgId = CFE_SB_ValueToMsgId(MM_CMD_MID); - CFE_MSG_FcnCode_t FcnCode = MM_PEEK_CC; + CFE_SB_MsgId_t TestMsgId = CFE_SB_ValueToMsgId(MM_CMD_MID); + CFE_MSG_FcnCode_t FcnCode = MM_PEEK_CC; UT_SetDataBuffer(UT_KEY(CFE_MSG_GetMsgId), &TestMsgId, sizeof(TestMsgId), false); UT_SetDataBuffer(UT_KEY(CFE_MSG_GetFcnCode), &FcnCode, sizeof(FcnCode), false); @@ -585,8 +587,8 @@ void MM_AppPipe_Test_PeekSuccess(void) void MM_AppPipe_Test_PeekFail(void) { - CFE_SB_MsgId_t TestMsgId = CFE_SB_ValueToMsgId(MM_CMD_MID); - CFE_MSG_FcnCode_t FcnCode = MM_PEEK_CC; + CFE_SB_MsgId_t TestMsgId = CFE_SB_ValueToMsgId(MM_CMD_MID); + CFE_MSG_FcnCode_t FcnCode = MM_PEEK_CC; UT_SetDataBuffer(UT_KEY(CFE_MSG_GetMsgId), &TestMsgId, sizeof(TestMsgId), false); UT_SetDataBuffer(UT_KEY(CFE_MSG_GetFcnCode), &FcnCode, sizeof(FcnCode), false); @@ -866,6 +868,7 @@ void MM_AppPipe_Test_LookupSymbolSuccess(void) UT_SetDataBuffer(UT_KEY(CFE_MSG_GetSize), &MsgSize, sizeof(MsgSize), false); strncpy(UT_CmdBuf.LookupSymCmd.Payload.SymName, "name", sizeof(UT_CmdBuf.LookupSymCmd.Payload.SymName) - 1); + UT_SetDefaultReturnValue(UT_KEY(MM_strnlen), sizeof("name")); UT_SetDataBuffer(UT_KEY(OS_SymbolLookup), &ResolvedAddr, sizeof(ResolvedAddr), false); /* ignore dummy message length check */ @@ -922,6 +925,8 @@ void MM_AppPipe_Test_SymTblToFileSuccess(void) strncpy(UT_CmdBuf.SymTblToFileCmd.Payload.FileName, "name", sizeof(UT_CmdBuf.SymTblToFileCmd.Payload.FileName) - 1); + UT_SetDefaultReturnValue(UT_KEY(MM_strnlen), sizeof("name")); + /* Set to satisfy condition "OS_Status == OS_SUCCESS" */ UT_SetDeferredRetcode(UT_KEY(OS_SymbolTableDump), 1, CFE_SUCCESS); @@ -1183,8 +1188,9 @@ void MM_HousekeepingCmd_Test(void) UtAssert_True(MM_AppData.HkPacket.Payload.DataValue == 6, "MM_AppData.HkPacket.Payload.DataValue == 6"); UtAssert_True(MM_AppData.HkPacket.Payload.BytesProcessed == 7, "MM_AppData.HkPacket.Payload.BytesProcessed == 7"); - UtAssert_True(strncmp(MM_AppData.HkPacket.Payload.FileName, MM_AppData.HkPacket.Payload.FileName, OS_MAX_PATH_LEN) == 0, - "strncmp(MM_AppData.HkPacket.Payload.FileName, MM_AppData.HkPacket.Payload.FileName, OS_MAX_PATH_LEN) == 0"); + UtAssert_True( + strncmp(MM_AppData.HkPacket.Payload.FileName, MM_AppData.HkPacket.Payload.FileName, OS_MAX_PATH_LEN) == 0, + "strncmp(MM_AppData.HkPacket.Payload.FileName, MM_AppData.HkPacket.Payload.FileName, OS_MAX_PATH_LEN) == 0"); call_count_CFE_EVS_SendEvent = UT_GetStubCount(UT_KEY(CFE_EVS_SendEvent)); @@ -1209,6 +1215,8 @@ void MM_LookupSymbolCmd_Test_Nominal(void) strncpy(UT_CmdBuf.LookupSymCmd.Payload.SymName, "name", sizeof(UT_CmdBuf.LookupSymCmd.Payload.SymName) - 1); + UT_SetDefaultReturnValue(UT_KEY(MM_strnlen), sizeof("name")); + /* ignore dummy message length check */ UT_SetDefaultReturnValue(UT_KEY(MM_VerifyCmdLength), true); @@ -1218,7 +1226,8 @@ void MM_LookupSymbolCmd_Test_Nominal(void) MM_LookupSymbolCmd(&UT_CmdBuf.Buf); /* Verify results */ - UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_SYM_LOOKUP, "MM_AppData.HkPacket.Payload.LastAction == MM_SYM_LOOKUP"); + UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_SYM_LOOKUP, + "MM_AppData.HkPacket.Payload.LastAction == MM_SYM_LOOKUP"); UtAssert_True(MM_AppData.HkPacket.Payload.Address == 0, "MM_AppData.HkPacket.Payload.Address == 0"); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.CmdCounter, 0); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.ErrCounter, 0); @@ -1292,6 +1301,8 @@ void MM_LookupSymbolCmd_Test_SymbolLookupError(void) strncpy(UT_CmdBuf.LookupSymCmd.Payload.SymName, "name", sizeof(UT_CmdBuf.LookupSymCmd.Payload.SymName) - 1); + UT_SetDefaultReturnValue(UT_KEY(MM_strnlen), sizeof("name")); + /* Set to generate error message MM_SYMNAME_ERR_EID */ UT_SetDeferredRetcode(UT_KEY(OS_SymbolLookup), 1, -1); @@ -1335,6 +1346,8 @@ void MM_SymTblToFileCmd_Test_Nominal(void) strncpy(UT_CmdBuf.SymTblToFileCmd.Payload.FileName, "name", sizeof(UT_CmdBuf.SymTblToFileCmd.Payload.FileName) - 1); + UT_SetDefaultReturnValue(UT_KEY(MM_strnlen), sizeof("name")); + /* Set to satisfy condition "OS_Status == OS_SUCCESS" */ UT_SetDeferredRetcode(UT_KEY(OS_SymbolTableDump), 1, CFE_SUCCESS); @@ -1345,9 +1358,12 @@ void MM_SymTblToFileCmd_Test_Nominal(void) MM_SymTblToFileCmd(&UT_CmdBuf.Buf); /* Verify results */ - UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_SYMTBL_SAVE, "MM_AppData.HkPacket.Payload.LastAction == MM_SYMTBL_SAVE"); - UtAssert_True(strncmp(MM_AppData.HkPacket.Payload.FileName, UT_CmdBuf.SymTblToFileCmd.Payload.FileName, OS_MAX_PATH_LEN) == 0, - "strncmp(MM_AppData.HkPacket.Payload.FileName, UT_CmdBuf.SymTblToFileCmd.Payload.FileName, OS_MAX_PATH_LEN) == 0"); + UtAssert_True(MM_AppData.HkPacket.Payload.LastAction == MM_SYMTBL_SAVE, + "MM_AppData.HkPacket.Payload.LastAction == MM_SYMTBL_SAVE"); + UtAssert_True( + strncmp(MM_AppData.HkPacket.Payload.FileName, UT_CmdBuf.SymTblToFileCmd.Payload.FileName, OS_MAX_PATH_LEN) == 0, + "strncmp(MM_AppData.HkPacket.Payload.FileName, UT_CmdBuf.SymTblToFileCmd.Payload.FileName, OS_MAX_PATH_LEN) == " + "0"); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.CmdCounter, 0); UtAssert_INT32_EQ(MM_AppData.HkPacket.Payload.ErrCounter, 0); @@ -1421,6 +1437,8 @@ void MM_SymTblToFileCmd_Test_SymbolTableDumpError(void) strncpy(UT_CmdBuf.SymTblToFileCmd.Payload.FileName, "name", sizeof(UT_CmdBuf.SymTblToFileCmd.Payload.FileName) - 1); + UT_SetDefaultReturnValue(UT_KEY(MM_strnlen), sizeof("name")); + /* Set to generate error message MM_SYMTBL_TO_FILE_FAIL_ERR_EID */ UT_SetDeferredRetcode(UT_KEY(OS_SymbolTableDump), 1, -1); diff --git a/unit-test/mm_utils_tests.c b/unit-test/mm_utils_tests.c index 1d3b1c1..b282e74 100644 --- a/unit-test/mm_utils_tests.c +++ b/unit-test/mm_utils_tests.c @@ -3569,6 +3569,37 @@ void MM_ComputeCRCFromFile_Test(void) UtAssert_True(Result == -1, "Result == -1"); } +void Test_MM_strnlen_null_character_found(void) +{ + /* Arrange */ + size_t result; + char str[CFE_MISSION_EVS_MAX_MESSAGE_LENGTH]; + + memset(str, 0xFF, sizeof(str) - 1); + str[CFE_MISSION_EVS_MAX_MESSAGE_LENGTH - 1] = '\0'; + + /* Act */ + result = MM_strnlen(str, sizeof(str)); + + /* Assert */ + UtAssert_INT32_EQ(result, sizeof(str) - 1); +} + +void Test_MM_strnlen_null_character_not_found(void) +{ + /* Arrange */ + size_t result; + char str[CFE_MISSION_EVS_MAX_MESSAGE_LENGTH]; + + memset(str, 0xFF, sizeof(str)); + + /* Act */ + result = MM_strnlen(str, sizeof(str)); + + /* Assert */ + UtAssert_INT32_EQ(result, sizeof(str)); +} + /* * Register the test cases to execute with the unit test tool */ @@ -3793,4 +3824,10 @@ void UtTest_Setup(void) UtTest_Add(MM_ResolveSymAddr_Test, MM_Test_Setup, MM_Test_TearDown, "MM_ResolveSymAddr_Test"); UtTest_Add(MM_ComputeCRCFromFile_Test, MM_Test_Setup, MM_Test_TearDown, "MM_ComputeCRCFromFile_Test"); + + /* MM_strnlen Tests */ + UtTest_Add(Test_MM_strnlen_null_character_found, MM_Test_Setup, MM_Test_TearDown, + "Test_MM_strnlen_null_character_found"); + UtTest_Add(Test_MM_strnlen_null_character_not_found, MM_Test_Setup, MM_Test_TearDown, + "Test_MM_strnlen_null_character_not_found"); } diff --git a/unit-test/stubs/mm_utils_stubs.c b/unit-test/stubs/mm_utils_stubs.c index 2ecdebe..f2fdc01 100644 --- a/unit-test/stubs/mm_utils_stubs.c +++ b/unit-test/stubs/mm_utils_stubs.c @@ -100,3 +100,11 @@ int32 MM_ComputeCRCFromFile(osal_id_t FileHandle, uint32 *CrcPtr, uint32 TypeCRC UT_Stub_RegisterContextGenericArg(UT_KEY(MM_ComputeCRCFromFile), TypeCRC); return UT_DEFAULT_IMPL(MM_ComputeCRCFromFile); } + +size_t MM_strnlen(const char *str, size_t maxlen) +{ + UT_Stub_RegisterContext(UT_KEY(MM_strnlen), str); + UT_Stub_RegisterContextGenericArg(UT_KEY(MM_strnlen), maxlen); + + return UT_DEFAULT_IMPL(MM_strnlen); +} \ No newline at end of file