From 23e2ba1618c55da3949e75fb7b16fb29152ca0dc Mon Sep 17 00:00:00 2001 From: jdfiguer Date: Mon, 10 Jun 2024 09:35:43 -0400 Subject: [PATCH] Fix #117, Replaces strncpy with snprintf This commit addresses issues flagged during static analysis by: - Replacing strncpy with snprintf to enhance safety and compliance. - Replacing strlen with FM_strnlen. --- fsw/src/fm_child.c | 25 ++++++++---------- fsw/src/fm_cmd_utils.c | 25 ++++++++++++++---- fsw/src/fm_cmd_utils.h | 17 +++++++++++++ fsw/src/fm_cmds.c | 9 +++---- unit-test/fm_child_tests.c | 3 +++ unit-test/fm_cmd_utils_tests.c | 38 ++++++++++++++++++++++++++++ unit-test/stubs/fm_cmd_utils_stubs.c | 17 +++++++++++++ 7 files changed, 110 insertions(+), 24 deletions(-) diff --git a/fsw/src/fm_child.c b/fsw/src/fm_child.c index 8559abc..2e7d23d 100644 --- a/fsw/src/fm_child.c +++ b/fsw/src/fm_child.c @@ -801,8 +801,7 @@ void FM_ChildFileInfoCmd(FM_ChildQueueEntry_t *CmdArgs) /* Report directory or filename state, name, size and time */ ReportPtr->FileStatus = (uint8)CmdArgs->FileInfoState; - strncpy(ReportPtr->Filename, CmdArgs->Source1, OS_MAX_PATH_LEN - 1); - ReportPtr->Filename[OS_MAX_PATH_LEN - 1] = '\0'; + snprintf(ReportPtr->Filename, OS_MAX_PATH_LEN, "%s", CmdArgs->Source1); ReportPtr->FileSize = CmdArgs->FileInfoSize; ReportPtr->LastModifiedTime = CmdArgs->FileInfoTime; @@ -1136,7 +1135,7 @@ void FM_ChildDirListPktCmd(const FM_ChildQueueEntry_t *CmdArgs) ** CmdArgs->Source2 = directory name plus separator ** CmdArgs->DirListOffset = index of 1st reported dir entry */ - PathLength = strlen(CmdArgs->Source2); + PathLength = FM_strnlen(CmdArgs->Source2, OS_MAX_PATH_LEN); /* Open source directory for reading directory list */ Status = OS_DirectoryOpen(&DirId, CmdArgs->Source1); @@ -1157,9 +1156,8 @@ void FM_ChildDirListPktCmd(const FM_ChildQueueEntry_t *CmdArgs) ReportPtr = &FM_GlobalData.DirListPkt.Payload; - strncpy(ReportPtr->DirName, CmdArgs->Source1, OS_MAX_PATH_LEN - 1); - ReportPtr->DirName[OS_MAX_PATH_LEN - 1] = '\0'; - ReportPtr->FirstFile = CmdArgs->DirListOffset; + snprintf(ReportPtr->DirName, OS_MAX_PATH_LEN, "%s", CmdArgs->Source1); + ReportPtr->FirstFile = CmdArgs->DirListOffset; StillProcessing = true; while (StillProcessing == true) @@ -1187,14 +1185,13 @@ void FM_ChildDirListPktCmd(const FM_ChildQueueEntry_t *CmdArgs) ListIndex = ReportPtr->PacketFiles; ListEntry = &ReportPtr->FileList[ListIndex]; - EntryLength = strlen(OS_DIRENTRY_NAME(DirEntry)); + EntryLength = FM_strnlen(OS_DIRENTRY_NAME(DirEntry), OS_MAX_FILE_NAME); /* Verify combined directory plus filename length */ if ((PathLength + EntryLength) < sizeof(LogicalName)) { /* Add filename to directory listing telemetry packet */ - strncpy(ListEntry->EntryName, OS_DIRENTRY_NAME(DirEntry), sizeof(ListEntry->EntryName) - 1); - ListEntry->EntryName[sizeof(ListEntry->EntryName) - 1] = '\0'; + snprintf(ListEntry->EntryName, sizeof(ListEntry->EntryName), "%s", OS_DIRENTRY_NAME(DirEntry)); /* Build filename - Directory already has path separator */ memcpy(LogicalName, CmdArgs->Source2, PathLength); @@ -1378,7 +1375,7 @@ void FM_ChildDirListFileLoop(osal_id_t DirId, osal_id_t FileHandle, const char * memset(&DirEntry, 0, sizeof(DirEntry)); - PathLength = strlen(DirWithSep); + PathLength = FM_strnlen(DirWithSep, OS_MAX_PATH_LEN); /* Until end of directory entries or output file write error */ while ((CommandResult == true) && (ReadingDirectory == true)) @@ -1399,7 +1396,7 @@ void FM_ChildDirListFileLoop(osal_id_t DirId, osal_id_t FileHandle, const char * /* Count all files - write limited number */ if (FileEntries < FM_DIR_LIST_FILE_ENTRIES) { - EntryLength = strlen(OS_DIRENTRY_NAME(DirEntry)); + EntryLength = FM_strnlen(OS_DIRENTRY_NAME(DirEntry), OS_MAX_FILE_NAME); /* * DirListData.EntryName and TempName are both OS_MAX_PATH_LEN, DirEntry name is OS_MAX_FILE_NAME, @@ -1484,9 +1481,9 @@ void FM_ChildDirListFileLoop(osal_id_t DirId, osal_id_t FileHandle, const char * { FM_GlobalData.ChildCmdCounter++; - CFE_EVS_SendEvent(FM_GET_DIR_FILE_CMD_INF_EID, - CFE_EVS_EventType_INFORMATION, "%s command: wrote %d of %d names: dir = %s, filename = %s", - CmdText, (int)FileEntries, (int)DirEntries, Directory, Filename); + CFE_EVS_SendEvent(FM_GET_DIR_FILE_CMD_INF_EID, CFE_EVS_EventType_INFORMATION, + "%s command: wrote %d of %d names: dir = %s, filename = %s", CmdText, (int)FileEntries, + (int)DirEntries, Directory, Filename); } } diff --git a/fsw/src/fm_cmd_utils.c b/fsw/src/fm_cmd_utils.c index 4b018bc..7f021a5 100644 --- a/fsw/src/fm_cmd_utils.c +++ b/fsw/src/fm_cmd_utils.c @@ -508,7 +508,7 @@ void FM_AppendPathSep(char *Directory, uint32 BufferSize) */ size_t StringLength = 0; - StringLength = strlen(Directory); + StringLength = FM_strnlen(Directory, OS_MAX_PATH_LEN); /* Do nothing if string already ends with a path separator */ if ((StringLength != 0) && (Directory[StringLength - 1] != '/')) @@ -577,9 +577,8 @@ CFE_Status_t FM_GetDirectorySpaceEstimate(const char *Directory, uint64 *BlockCo TotalBytes = 0; memset(&DirEntry, 0, sizeof(DirEntry)); - strncpy(FullPath, Directory, sizeof(FullPath) - 1); - FullPath[sizeof(FullPath) - 1] = 0; - DirLen = strlen(FullPath); + snprintf(FullPath, sizeof(FullPath), "%s", Directory); + DirLen = FM_strnlen(FullPath, OS_MAX_PATH_LEN); if (DirLen < (sizeof(FullPath) - 2)) { FullPath[DirLen] = '/'; @@ -607,7 +606,7 @@ CFE_Status_t FM_GetDirectorySpaceEstimate(const char *Directory, uint64 *BlockCo /* Read each directory entry and stat the files */ while (OS_DirectoryRead(DirId, &DirEntry) == OS_SUCCESS) { - strncpy(&FullPath[DirLen], OS_DIRENTRY_NAME(DirEntry), sizeof(FullPath) - DirLen - 1); + snprintf(&FullPath[DirLen], sizeof(FullPath) - DirLen, "%s", OS_DIRENTRY_NAME(DirEntry)); OS_Status = OS_stat(FullPath, &FileStat); if (OS_Status != OS_SUCCESS) @@ -635,3 +634,19 @@ CFE_Status_t FM_GetDirectorySpaceEstimate(const char *Directory, uint64 *BlockCo return Result; } + +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ +/* */ +/* FM utility function -- get string length */ +/* */ +/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */ +size_t FM_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; +} \ No newline at end of file diff --git a/fsw/src/fm_cmd_utils.h b/fsw/src/fm_cmd_utils.h index d89dade..6d64131 100644 --- a/fsw/src/fm_cmd_utils.h +++ b/fsw/src/fm_cmd_utils.h @@ -400,4 +400,21 @@ CFE_Status_t FM_GetVolumeFreeSpace(const char *FileSys, uint64 *BlockCount, uint */ CFE_Status_t FM_GetDirectorySpaceEstimate(const char *Directory, uint64 *BlockCount, uint64 *ByteCount); +/************************************************************************/ +/** @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 FM_strnlen(const char *str, size_t maxlen); + #endif diff --git a/fsw/src/fm_cmds.c b/fsw/src/fm_cmds.c index 48f4ace..1420c2f 100644 --- a/fsw/src/fm_cmds.c +++ b/fsw/src/fm_cmds.c @@ -387,10 +387,8 @@ bool FM_DecompressFileCmd(const CFE_SB_Buffer_t *BufPtr) /* Set handshake queue command args */ CmdArgs->CommandCode = FM_DECOMPRESS_FILE_CC; - strncpy(CmdArgs->Source1, CmdPtr->Source, OS_MAX_PATH_LEN - 1); - CmdArgs->Source1[OS_MAX_PATH_LEN - 1] = '\0'; - strncpy(CmdArgs->Target, CmdPtr->Target, OS_MAX_PATH_LEN - 1); - CmdArgs->Target[OS_MAX_PATH_LEN - 1] = '\0'; + snprintf(CmdArgs->Source1, OS_MAX_PATH_LEN, "%s", CmdPtr->Source); + snprintf(CmdArgs->Target, OS_MAX_PATH_LEN, "%s", CmdPtr->Target); /* Invoke lower priority child task */ FM_InvokeChildTask(); @@ -831,7 +829,8 @@ bool FM_MonitorFilesystemSpaceCmd(const CFE_SB_Buffer_t *BufPtr) CFE_SB_TransmitMsg(CFE_MSG_PTR(FM_GlobalData.MonitorReportPkt.TelemetryHeader), true); /* Send command completion event (info) */ - CFE_EVS_SendEvent(FM_MONITOR_FILESYSTEM_SPACE_CMD_INF_EID, CFE_EVS_EventType_INFORMATION, "%s command", CmdText); + CFE_EVS_SendEvent(FM_MONITOR_FILESYSTEM_SPACE_CMD_INF_EID, CFE_EVS_EventType_INFORMATION, "%s command", + CmdText); } return CommandResult; diff --git a/unit-test/fm_child_tests.c b/unit-test/fm_child_tests.c index 40a18f6..3800f60 100644 --- a/unit-test/fm_child_tests.c +++ b/unit-test/fm_child_tests.c @@ -1747,6 +1747,7 @@ void Test_FM_ChildDirListPktCmd_PathAndEntryLengthGreaterMaxPathLength(void) UT_SetDeferredRetcode(UT_KEY(OS_DirectoryRead), 2, !OS_SUCCESS); UT_SetDataBuffer(UT_KEY(OS_DirectoryRead), &direntry, sizeof(direntry), false); + UT_SetDefaultReturnValue(UT_KEY(FM_strnlen), strlen(queue_entry.Source2)); /* Act */ UtAssert_VOIDCALL(FM_ChildDirListPktCmd(&queue_entry)); @@ -1985,6 +1986,8 @@ void Test_FM_ChildDirListFileLoop_PathLengthAndEntryLengthGreaterMaxPathLen(void UT_SetDataBuffer(UT_KEY(OS_DirectoryRead), &direntry, sizeof(direntry), false); UT_SetDeferredRetcode(UT_KEY(OS_DirectoryRead), 2, !OS_SUCCESS); + UT_SetDefaultReturnValue(UT_KEY(FM_strnlen), strlen(dirwithsep)); + UT_SetDeferredRetcode(UT_KEY(FM_strnlen), 2, strlen(OS_DIRENTRY_NAME(direntry))); /* Act */ UtAssert_VOIDCALL(FM_ChildDirListFileLoop(FM_UT_OBJID_1, FM_UT_OBJID_2, "dir", dirwithsep, "fname", false)); diff --git a/unit-test/fm_cmd_utils_tests.c b/unit-test/fm_cmd_utils_tests.c index f02022e..fa1516a 100644 --- a/unit-test/fm_cmd_utils_tests.c +++ b/unit-test/fm_cmd_utils_tests.c @@ -669,6 +669,40 @@ void Test_FM_GetDirectorySpaceEstimate(void) UtAssert_STUB_COUNT(OS_DirectoryRead, 0); } +/* ************************** + * FM_strnlen Tests + * *************************/ +void Test_FM_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 = FM_strnlen(str, sizeof(str)); + + /* Assert */ + UtAssert_INT32_EQ(result, sizeof(str) - 1); +} + +void Test_FM_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 = FM_strnlen(str, sizeof(str)); + + /* Assert */ + UtAssert_INT32_EQ(result, sizeof(str)); +} + /* * Register the test cases to execute with the unit test tool */ @@ -690,4 +724,8 @@ void UtTest_Setup(void) UtTest_Add(Test_FM_AppendPathSep, FM_Test_Setup, FM_Test_Teardown, "Test_FM_AppendPathSep"); UtTest_Add(Test_FM_GetVolumeFreeSpace, FM_Test_Setup, FM_Test_Teardown, "Test_FM_GetVolumeFreeSpace"); UtTest_Add(Test_FM_GetDirectorySpaceEstimate, FM_Test_Setup, FM_Test_Teardown, "Test_FM_GetDirectorySpaceEstimate"); + UtTest_Add(Test_FM_strnlen_null_character_found, FM_Test_Setup, FM_Test_Teardown, + "Test_FM_strnlen_null_character_found"); + UtTest_Add(Test_FM_strnlen_null_character_not_found, FM_Test_Setup, FM_Test_Teardown, + "Test_FM_strnlen_null_character_not_found"); } diff --git a/unit-test/stubs/fm_cmd_utils_stubs.c b/unit-test/stubs/fm_cmd_utils_stubs.c index a5f134c..5d423d4 100644 --- a/unit-test/stubs/fm_cmd_utils_stubs.c +++ b/unit-test/stubs/fm_cmd_utils_stubs.c @@ -318,3 +318,20 @@ bool FM_VerifyOverwrite(uint16 Overwrite, uint32 EventID, const char *CmdText) return UT_GenStub_GetReturnValue(FM_VerifyOverwrite, bool); } + +/* + * ---------------------------------------------------- + * Generated stub function for FM_strnlen() + * ---------------------------------------------------- + */ +size_t FM_strnlen(const char *str, size_t maxlen) +{ + UT_GenStub_SetupReturnBuffer(FM_strnlen, size_t); + + UT_GenStub_AddParam(FM_strnlen, const char *, str); + UT_GenStub_AddParam(FM_strnlen, size_t, maxlen); + + UT_GenStub_Execute(FM_strnlen, Basic, NULL); + + return UT_GenStub_GetReturnValue(FM_strnlen, size_t); +}