Skip to content

Commit

Permalink
Fix #117, Replaces strncpy with snprintf
Browse files Browse the repository at this point in the history
This commit addresses issues flagged during static analysis by:
- Replacing strncpy with snprintf to enhance safety and compliance.
- Replacing strlen with FM_strnlen.
  • Loading branch information
jdfiguer authored and jdfiguer committed Jun 18, 2024
1 parent f033752 commit 23e2ba1
Show file tree
Hide file tree
Showing 7 changed files with 110 additions and 24 deletions.
25 changes: 11 additions & 14 deletions fsw/src/fm_child.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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)
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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))
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}
}

Expand Down
25 changes: 20 additions & 5 deletions fsw/src/fm_cmd_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -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] != '/'))
Expand Down Expand Up @@ -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] = '/';
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}
17 changes: 17 additions & 0 deletions fsw/src/fm_cmd_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
9 changes: 4 additions & 5 deletions fsw/src/fm_cmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 3 additions & 0 deletions unit-test/fm_child_tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down Expand Up @@ -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));
Expand Down
38 changes: 38 additions & 0 deletions unit-test/fm_cmd_utils_tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand All @@ -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");
}
17 changes: 17 additions & 0 deletions unit-test/stubs/fm_cmd_utils_stubs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

0 comments on commit 23e2ba1

Please sign in to comment.