Skip to content

Commit

Permalink
Fix #92, Adds static analysis comments, replaces strncpy and strlen
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
jdfiguer authored and jdfiguer committed Jun 27, 2024
1 parent 2dfcc48 commit 0409a4a
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 21 deletions.
10 changes: 5 additions & 5 deletions fsw/src/mm_app.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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;
}
Expand All @@ -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");
Expand All @@ -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);
Expand All @@ -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;
}
Expand Down
10 changes: 7 additions & 3 deletions fsw/src/mm_dump.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -522,17 +522,21 @@ 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++;
}

/*
** 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));
Expand Down
7 changes: 6 additions & 1 deletion fsw/src/mm_utils.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
Expand All @@ -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)
{
Expand All @@ -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)
{
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
31 changes: 19 additions & 12 deletions unit-test/mm_app_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 0409a4a

Please sign in to comment.