From 0409a4aa47802af5ca0d5f17f0022fb3894cf0db 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 OS_strnlen. --- fsw/src/mm_app.c | 10 +++++----- fsw/src/mm_dump.c | 10 +++++++--- fsw/src/mm_utils.c | 7 ++++++- unit-test/mm_app_tests.c | 31 +++++++++++++++++++------------ 4 files changed, 37 insertions(+), 21 deletions(-) diff --git a/fsw/src/mm_app.c b/fsw/src/mm_app.c index f099a04..73772d6 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 (OS_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 OS_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 (OS_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 OS_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..78d1cf2 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 = OS_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 = OS_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..59f9a64 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 @@ -381,6 +383,7 @@ bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeI MaxSize = MM_MAX_FILL_DATA_MEM32; } PSP_MemType = CFE_PSP_MEM_RAM; + /* SAD: No need to check snprintf return value; buffer size can store "MEM32" without overflow */ snprintf(MemTypeStr, MM_MAX_MEM_TYPE_STR_LEN, "%s", "MEM32"); if (MM_Verify32Aligned(Address, SizeInBytes) != true) { @@ -406,6 +409,7 @@ bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeI MaxSize = MM_MAX_FILL_DATA_MEM16; } PSP_MemType = CFE_PSP_MEM_RAM; + /* SAD: No need to check snprintf return value; buffer size can store "MEM16" without overflow */ snprintf(MemTypeStr, MM_MAX_MEM_TYPE_STR_LEN, "%s", "MEM16"); if (MM_Verify16Aligned(Address, SizeInBytes) != true) { @@ -431,6 +435,7 @@ bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeI MaxSize = MM_MAX_FILL_DATA_MEM8; } PSP_MemType = CFE_PSP_MEM_RAM; + /* SAD: No need to check snprintf return value; buffer size can store "MEM8" without overflow */ snprintf(MemTypeStr, MM_MAX_MEM_TYPE_STR_LEN, "%s", "MEM8"); break; #endif @@ -515,7 +520,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 (OS_strnlen(SymAddr->SymName, OS_MAX_SYM_LEN) == 0) { *ResolvedAddr = SymAddr->Offset; Valid = true; diff --git a/unit-test/mm_app_tests.c b/unit-test/mm_app_tests.c index 0f63da3..32921fd 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); @@ -1183,8 +1185,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)); @@ -1218,7 +1221,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); @@ -1345,9 +1349,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);