Skip to content

Commit

Permalink
Fix #117, Replaces strncpy and strlen
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 OS_strnlen.
  • Loading branch information
jdfiguer authored and jdfiguer committed Jun 21, 2024
1 parent f033752 commit 42b13a5
Show file tree
Hide file tree
Showing 5 changed files with 31 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 = OS_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 = OS_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 = OS_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 = OS_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
9 changes: 4 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 = OS_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 = OS_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
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(OS_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(OS_strnlen), strlen(dirwithsep));
UT_SetDeferredRetcode(UT_KEY(OS_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
9 changes: 9 additions & 0 deletions unit-test/fm_cmd_utils_tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -555,19 +555,23 @@ void Test_FM_AppendPathSep(void)
char directory[3] = "";

/* Empty directory */
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen(directory));
UtAssert_VOIDCALL(FM_AppendPathSep(directory, sizeof(directory)));
UtAssert_UINT32_EQ(directory[0], 0);

/* Ends with directory separator */
strncpy(directory, "/", sizeof(directory));
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen(directory));
UtAssert_VOIDCALL(FM_AppendPathSep(directory, sizeof(directory)));
UtAssert_UINT32_EQ(strncmp(directory, "/", sizeof(directory)), 0);

/* No room */
strncpy(directory, "a", sizeof(directory));
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen(directory));
UtAssert_VOIDCALL(FM_AppendPathSep(directory, sizeof(directory) - 1));
UtAssert_UINT32_EQ(strncmp(directory, "a", sizeof(directory)), 0);

UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen(directory));
UtAssert_VOIDCALL(FM_AppendPathSep(directory, sizeof(directory)));
UtAssert_UINT32_EQ(strncmp(directory, "a/", sizeof(directory)), 0);
}
Expand Down Expand Up @@ -624,6 +628,7 @@ void Test_FM_GetDirectorySpaceEstimate(void)
UT_SetDeferredRetcode(UT_KEY(OS_DirectoryRead), 2, OS_ERROR);
UT_SetDataBuffer(UT_KEY(OS_DirectoryRead), &direntry, sizeof(direntry), false);
UT_SetDataBuffer(UT_KEY(OS_stat), &fstat, sizeof(fstat), false);
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen(direntry.FileName));

UtAssert_INT32_EQ(FM_GetDirectorySpaceEstimate("test", &blocks, &bytes), CFE_SUCCESS);
UtAssert_ZERO(blocks); /* not reported via OS_stat, so left unchanged */
Expand All @@ -636,6 +641,7 @@ void Test_FM_GetDirectorySpaceEstimate(void)
UT_SetDeferredRetcode(UT_KEY(OS_DirectoryRead), 2, OS_ERROR);
UT_SetDataBuffer(UT_KEY(OS_DirectoryRead), &direntry, sizeof(direntry), false);
UT_SetDataBuffer(UT_KEY(OS_stat), &fstat, sizeof(fstat), false);
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen("test"));

UtAssert_INT32_EQ(FM_GetDirectorySpaceEstimate("test", &blocks, &bytes), CFE_SUCCESS);
UtAssert_ZERO(blocks);
Expand All @@ -645,6 +651,7 @@ void Test_FM_GetDirectorySpaceEstimate(void)
UT_SetDeferredRetcode(UT_KEY(OS_DirectoryRead), 2, OS_ERROR);
UT_SetDataBuffer(UT_KEY(OS_DirectoryRead), &direntry, sizeof(direntry), false);
UT_SetDefaultReturnValue(UT_KEY(OS_stat), OS_ERROR);
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen("test"));
UtAssert_INT32_EQ(FM_GetDirectorySpaceEstimate("test", &blocks, &bytes), CFE_SUCCESS);
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 1);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventID, FM_DIRECTORY_ESTIMATE_ERR_EID);
Expand All @@ -655,6 +662,7 @@ void Test_FM_GetDirectorySpaceEstimate(void)
UT_SetDefaultReturnValue(UT_KEY(OS_DirectoryRead), OS_ERROR);
memset(longname, 'a', sizeof(longname) - 1);
longname[sizeof(longname) - 1] = 0;
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen(longname));
UtAssert_INT32_EQ(FM_GetDirectorySpaceEstimate(longname, &blocks, &bytes), CFE_STATUS_EXTERNAL_RESOURCE_FAIL);
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 2);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[1].EventID, FM_DIRECTORY_ESTIMATE_ERR_EID);
Expand All @@ -663,6 +671,7 @@ void Test_FM_GetDirectorySpaceEstimate(void)

/* OS_DirectoryOpen failed */
UT_SetDefaultReturnValue(UT_KEY(OS_DirectoryOpen), OS_ERROR);
UT_SetDefaultReturnValue(UT_KEY(OS_strnlen), strlen("test"));
UtAssert_INT32_EQ(FM_GetDirectorySpaceEstimate("test", &blocks, &bytes), CFE_STATUS_EXTERNAL_RESOURCE_FAIL);
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 3);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[2].EventID, FM_DIRECTORY_ESTIMATE_ERR_EID);
Expand Down

0 comments on commit 42b13a5

Please sign in to comment.