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 MM_strnlen.
  • Loading branch information
jdfiguer authored and jdfiguer committed Jun 18, 2024
1 parent 2dfcc48 commit cfd01f0
Show file tree
Hide file tree
Showing 7 changed files with 129 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 (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");
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 MM_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 (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");
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 MM_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 = MM_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 = MM_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
26 changes: 25 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 (MM_strnlen(SymAddr->SymName, OS_MAX_SYM_LEN) == 0)
{
*ResolvedAddr = SymAddr->Offset;
Valid = true;
Expand Down Expand Up @@ -586,3 +591,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;
}
17 changes: 17 additions & 0 deletions fsw/src/mm_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
42 changes: 30 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 @@ -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 */
Expand Down Expand Up @@ -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);

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

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

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

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

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

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

Expand Down
37 changes: 37 additions & 0 deletions unit-test/mm_utils_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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");
}
8 changes: 8 additions & 0 deletions unit-test/stubs/mm_utils_stubs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit cfd01f0

Please sign in to comment.