Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #46 #48 #49 #50, Resolve multiple static analysis issues (includes bug fixes) and remove unnecessary UT handlers #47

Merged
merged 4 commits into from
Aug 25, 2022
Merged

Fix #46 #48 #49 #50, Resolve multiple static analysis issues (includes bug fixes) and remove unnecessary UT handlers #47

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
598379a
Fix #46, Resolve possible string buffer overflows
skliper Aug 17, 2022
e6f9682
Fix #48, Filename reporting when move is empty bug
skliper Aug 18, 2022
97dc150
Fix #49, Resolve minor static analysis warnings
skliper Aug 18, 2022
1030705
Fix #50, Remove unnecessary handlers in UT
skliper Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions fsw/src/ds_appdefs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@

#define DS_INDEX_NONE -1 /**< \brief Packet filter table look-up = not found */

#define DS_PATH_SEPARATOR '/' /**< \brief File system path separator */
#define DS_EMPTY_STRING "" /**< \brief Empty string buffer entries in DS tables */
#define DS_STRING_TERMINATOR '\0' /**< \brief ASCIIZ string terminator character */
#define DS_PATH_SEPARATOR '/' /**< \brief File system path separator */
#define DS_EMPTY_STRING "" /**< \brief Empty string buffer entries in DS tables */

#define DS_TABLE_VERIFY_ERR 0xFFFFFFFF /**< \brief Table verification error return value */

Expand Down
9 changes: 5 additions & 4 deletions fsw/src/ds_cmds.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ void DS_CmdSetDestPath(const CFE_SB_Buffer_t *BufPtr)
** Set path portion of destination table filename...
*/
pDest = &DS_AppData.DestFileTblPtr->File[DS_DestPathCmd->FileTableIndex];
strcpy(pDest->Pathname, DS_DestPathCmd->Pathname);
strncpy(pDest->Pathname, DS_DestPathCmd->Pathname, sizeof(pDest->Pathname));

/*
** Notify cFE that we have modified the table data...
Expand Down Expand Up @@ -833,7 +833,7 @@ void DS_CmdSetDestBase(const CFE_SB_Buffer_t *BufPtr)
** Set base portion of destination table filename...
*/
pDest = &DS_AppData.DestFileTblPtr->File[DS_DestBaseCmd->FileTableIndex];
strcpy(pDest->Basename, DS_DestBaseCmd->Basename);
strncpy(pDest->Basename, DS_DestBaseCmd->Basename, sizeof(pDest->Basename));

/*
** Notify cFE that we have modified the table data...
Expand Down Expand Up @@ -903,7 +903,7 @@ void DS_CmdSetDestExt(const CFE_SB_Buffer_t *BufPtr)
** Set extension portion of destination table filename...
*/
pDest = &DS_AppData.DestFileTblPtr->File[DS_DestExtCmd->FileTableIndex];
strcpy(pDest->Extension, DS_DestExtCmd->Extension);
strncpy(pDest->Extension, DS_DestExtCmd->Extension, sizeof(pDest->Extension));

/*
** Notify cFE that we have modified the table data...
Expand Down Expand Up @@ -1366,7 +1366,8 @@ void DS_CmdGetFileInfo(const CFE_SB_Buffer_t *BufPtr)
/*
** Set current open filename...
*/
strcpy(DS_FileInfoPkt.FileInfo[i].FileName, DS_AppData.FileStatus[i].FileName);
strncpy(DS_FileInfoPkt.FileInfo[i].FileName, DS_AppData.FileStatus[i].FileName,
sizeof(DS_FileInfoPkt.FileInfo[i].FileName));
}
}

Expand Down
168 changes: 56 additions & 112 deletions fsw/src/ds_file.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ void DS_FileWriteHeader(int32 FileIndex)
*/
memset(&CFE_FS_Header, 0, sizeof(CFE_FS_Header));
CFE_FS_Header.SubType = DS_FILE_HDR_SUBTYPE;
strcpy(CFE_FS_Header.Description, DS_FILE_HDR_DESCRIPTION);
strncpy(CFE_FS_Header.Description, DS_FILE_HDR_DESCRIPTION, sizeof(CFE_FS_Header.Description));

/*
** Let cFE finish the init and write the primary header...
Expand All @@ -421,7 +421,7 @@ void DS_FileWriteHeader(int32 FileIndex)
memset(&DS_FileHeader, 0, sizeof(DS_FileHeader));
DS_FileHeader.FileTableIndex = FileIndex;
DS_FileHeader.FileNameType = DestFile->FileNameType;
strcpy(DS_FileHeader.FileName, FileStatus->FileName);
strncpy(DS_FileHeader.FileName, FileStatus->FileName, sizeof(DS_FileHeader.FileName));

/*
** Manually write the secondary header...
Expand Down Expand Up @@ -501,7 +501,7 @@ void DS_FileCreateDest(uint32 FileIndex)
*/
DS_FileCreateName(FileIndex);

if (FileStatus->FileName[0] != DS_STRING_TERMINATOR)
if (FileStatus->FileName[0] != 0)
{
/*
** Success - create a new destination file...
Expand Down Expand Up @@ -574,128 +574,73 @@ void DS_FileCreateName(uint32 FileIndex)
DS_DestFileEntry_t *DestFile = &DS_AppData.DestFileTblPtr->File[FileIndex];
DS_AppFileStatus_t *FileStatus = &DS_AppData.FileStatus[FileIndex];
int32 TotalLength = 0;
int32 WorknameLen = 2 * DS_TOTAL_FNAME_BUFSIZE;

char Workname[WorknameLen];
char Workname[2 * DS_TOTAL_FNAME_BUFSIZE];
char Sequence[DS_TOTAL_FNAME_BUFSIZE];

Workname[0] = DS_STRING_TERMINATOR;
Sequence[0] = DS_STRING_TERMINATOR;
/* Copy in path */
CFE_SB_MessageStringGet(Workname, DestFile->Pathname, NULL, sizeof(Workname), sizeof(DestFile->Pathname));
TotalLength = strlen(Workname);

/*
** Start with the path portion of the filename...
*/
strncpy(Workname, DestFile->Pathname, WorknameLen - 1);
Workname[WorknameLen - 1] = '\0';
TotalLength = strlen(Workname);

/*
** Add a path separator (if needed) before appending the base name...
*/
if (TotalLength > 0)
{
/* Add separator if needed */
if (Workname[TotalLength - 1] != DS_PATH_SEPARATOR)
{
/* if there's room, write the path separator and a new terminating
character. If there's not room, this will be caught by the
next condition */
if (TotalLength != (WorknameLen - 1))
{
Workname[TotalLength] = DS_PATH_SEPARATOR;
Workname[TotalLength + 1] = DS_STRING_TERMINATOR;
}
/* There's always space since Workname is twice the size of Pathname */
Workname[TotalLength++] = DS_PATH_SEPARATOR;
}
}
else
{
/* If path name is empty, start with the path separator. This should
* not happen because the path name is verified as non-empty in
* DS_TableVerifyDestFileEntry */
CFE_EVS_SendEvent(DS_FILE_CREATE_EMPTY_PATH_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: Path empty. dest = %d, path = '%s'", (int)FileIndex, DestFile->Pathname);

/*
** Something needs to get fixed before we try again...
*/
DS_AppData.FileStatus[FileIndex].FileState = DS_DISABLED;

return;
}

/*
** Verify that the path plus the base portion is not too large...
*/
if ((strlen(Workname) + strlen(DestFile->Basename)) < DS_TOTAL_FNAME_BUFSIZE)
{
/*
** Append the base portion to the path portion...
*/
strcat(Workname, DestFile->Basename);
/* Add base name */
CFE_SB_MessageStringGet(&Workname[TotalLength], DestFile->Basename, NULL, sizeof(Workname) - TotalLength,
sizeof(DestFile->Basename));
TotalLength = strlen(Workname);

/*
** Create the sequence portion of the filename...
*/
/* Create the sequence portion of the filename */
DS_FileCreateSequence(Sequence, DestFile->FileNameType, FileStatus->FileCount);

/*
** Verify that the path/base plus the sequence portion is not too large...
*/
if ((strlen(Workname) + strlen(Sequence)) < DS_TOTAL_FNAME_BUFSIZE)
{
/*
** Append the sequence portion to the path/base portion...
*/
strcat(Workname, Sequence);
/* Sequence is always null terminated so can use strncat */
strncat(&Workname[TotalLength], Sequence, sizeof(Workname) - TotalLength - 1);
TotalLength = strlen(Workname);

/*
** Check for an optional file extension...
*/
if (strlen(DestFile->Extension) > 0)
/* Only add extension if not empty */
if (DestFile->Extension[0] != '\0')
{
/* Add a "." character (if needed) before appending the extension */
if (DestFile->Extension[0] != '.')
{
/*
** Add a "." character (if needed) before appending the extension...
*/
if (DestFile->Extension[0] != '.')
{
strcat(Workname, ".");
}

/*
** Append the extension portion to the path/base+sequence portion...
*/
strcat(Workname, DestFile->Extension);
strncat(Workname, ".", sizeof(Workname) - strlen(Workname) - 1);
TotalLength++;
}

/*
** Final test - is "path/base+sequence.extension" length valid?...
*/
if (strlen(Workname) < DS_TOTAL_FNAME_BUFSIZE)
{
/*
** Success - copy workname to filename buffer...
*/
strcpy(FileStatus->FileName, Workname);
}
/* Append the extension portion to the path/base+sequence portion */
CFE_SB_MessageStringGet(&Workname[TotalLength], DestFile->Extension, NULL, sizeof(Workname) - TotalLength,
sizeof(DestFile->Extension));
}
}

if (FileStatus->FileName[0] == DS_STRING_TERMINATOR)
/* Confirm working name fits */
if (strlen(Workname) < DS_TOTAL_FNAME_BUFSIZE)
{
/* Success - copy workname to filename buffer */
strcpy(FileStatus->FileName, Workname);
}
else
{
/* Error - send event and disable destination */
CFE_EVS_SendEvent(DS_FILE_NAME_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: dest = %d, path = '%s', base = '%s', seq = '%s', ext = '%s'",
(int)FileIndex, DestFile->Pathname, DestFile->Basename, Sequence, DestFile->Extension);
DS_AppData.FileStatus[FileIndex].FileState = DS_DISABLED;
}
}
else
{
/*
** Error - send event and disable destination...
*/
CFE_EVS_SendEvent(DS_FILE_NAME_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: dest = %d, path = '%s', base = '%s', seq = '%s', ext = '%s'",
(int)FileIndex, DestFile->Pathname, DestFile->Basename, Sequence, DestFile->Extension);

/*
** Something needs to get fixed before we try again...
*/
/* Send event and disable for invalid path */
CFE_EVS_SendEvent(DS_FILE_CREATE_EMPTY_PATH_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE NAME error: Path empty. dest = %d, path = '%s'", (int)FileIndex, DestFile->Pathname);
DS_AppData.FileStatus[FileIndex].FileState = DS_DISABLED;
}

return;

} /* End of DS_FileCreateName() */

/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
Expand Down Expand Up @@ -742,7 +687,7 @@ void DS_FileCreateSequence(char *Buffer, uint32 Type, uint32 Count)
/*
** Add string terminator...
*/
Buffer[DS_SEQUENCE_DIGITS] = DS_STRING_TERMINATOR;
Buffer[DS_SEQUENCE_DIGITS] = '\0';
}
else if (Type == DS_BY_TIME)
{
Expand Down Expand Up @@ -817,14 +762,14 @@ void DS_FileCreateSequence(char *Buffer, uint32 Type, uint32 Count)
/*
** Step 7: Add string terminator...
*/
Buffer[DS_TERM_INDEX] = DS_STRING_TERMINATOR;
Buffer[DS_TERM_INDEX] = '\0';
}
else
{
/*
** Bad filename type, init buffer as empty...
*/
Buffer[0] = DS_STRING_TERMINATOR;
Buffer[0] = '\0';
}

return;
Expand Down Expand Up @@ -903,7 +848,8 @@ void DS_FileCloseDest(int32 FileIndex)
/*
** Make sure directory name does not end with slash character...
*/
strcpy(PathName, DS_AppData.DestFileTblPtr->File[FileIndex].Movename);
CFE_SB_MessageStringGet(PathName, DS_AppData.DestFileTblPtr->File[FileIndex].Movename, NULL, sizeof(PathName),
sizeof(DS_AppData.DestFileTblPtr->File[FileIndex].Movename));
PathLength = strlen(PathName);
if (PathName[PathLength - 1] == '/')
{
Expand Down Expand Up @@ -960,12 +906,10 @@ void DS_FileCloseDest(int32 FileIndex)
CFE_EVS_SendEvent(DS_MOVE_FILE_ERR_EID, CFE_EVS_EventType_ERROR,
"FILE MOVE error: dir name = '%s', filename = '%s'", PathName, FileName);
}
}

/*
** Get the directory name...
*/
strncpy(FileStatus->FileName, PathName, sizeof(PathName));
/* Update the path name for reporting */
strncpy(FileStatus->FileName, PathName, sizeof(PathName));
}
#else
/*
** Close the file...
Expand Down Expand Up @@ -1082,7 +1026,7 @@ void DS_FileTransmit(DS_AppFileStatus_t *FileStatus)
/*
** Set current open filename...
*/
strcpy(PktBuf->Pkt.FileInfo.FileName, FileStatus->FileName);
strncpy(PktBuf->Pkt.FileInfo.FileName, FileStatus->FileName, sizeof(PktBuf->Pkt.FileInfo.FileName));

/*
** Timestamp and send file info telemetry...
Expand Down
8 changes: 4 additions & 4 deletions fsw/src/ds_table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -828,8 +828,8 @@ bool DS_TableVerifyCount(uint32 SequenceCount)
void DS_TableSubscribe(void)
{
DS_PacketEntry_t *FilterPackets = NULL;
CFE_SB_MsgId_t MessageID = CFE_SB_INVALID_MSG_ID;
int32 i = 0;
CFE_SB_MsgId_t MessageID;
int32 i;

FilterPackets = DS_AppData.FilterTblPtr->Packet;

Expand Down Expand Up @@ -863,8 +863,8 @@ void DS_TableSubscribe(void)
void DS_TableUnsubscribe(void)
{
DS_PacketEntry_t *FilterPackets = NULL;
CFE_SB_MsgId_t MessageID = CFE_SB_INVALID_MSG_ID;
int32 i = 0;
CFE_SB_MsgId_t MessageID;
int32 i;

FilterPackets = DS_AppData.FilterTblPtr->Packet;

Expand Down
32 changes: 2 additions & 30 deletions unit-test/ds_file_tests.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,8 +620,6 @@ void DS_FileCreateDest_Test_StringTerminate(void)
{
uint32 FileIndex = 0;

DS_AppData.FileStatus[FileIndex].FileName[0] = DS_STRING_TERMINATOR;

/* Execute the function being tested */
UtAssert_VOIDCALL(DS_FileCreateDest(FileIndex));

Expand Down Expand Up @@ -843,7 +841,7 @@ void DS_FileCreateName_Test_Error(void)
DS_AppData.DestFileTblPtr->File[FileIndex].Basename[i] = 'a';
}

DS_AppData.DestFileTblPtr->File[FileIndex].Basename[DS_TOTAL_FNAME_BUFSIZE - 1] = DS_STRING_TERMINATOR;
DS_AppData.DestFileTblPtr->File[FileIndex].Basename[DS_TOTAL_FNAME_BUFSIZE - 1] = '\0';

/* Execute the function being tested */
DS_FileCreateName(FileIndex);
Expand All @@ -856,31 +854,6 @@ void DS_FileCreateName_Test_Error(void)

} /* end DS_FileCreateName_Test_Error */

void DS_FileCreateName_Test_MaxPathnameLength(void)
{
int32 FileIndex = 0;
int32 WorknameLen = 2 * DS_TOTAL_FNAME_BUFSIZE;
int32 i;

UT_DS_SetDestFileEntry(&DS_AppData.DestFileTblPtr->File[FileIndex]);

/* Set to fail the condition "if (TotalLength != (WorknameLen - 1))" */
for (i = 0; i < WorknameLen - 1; i++)
{
DS_AppData.DestFileTblPtr->File[FileIndex].Pathname[i] = 'a';
}

/* Execute the function being tested */
UtAssert_VOIDCALL(DS_FileCreateName(FileIndex));

/* Verify results */
UtAssert_INT32_EQ(strlen(DS_AppData.DestFileTblPtr->File[FileIndex].Pathname), WorknameLen - 1);
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 1);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventType, CFE_EVS_EventType_ERROR);
UtAssert_INT32_EQ(context_CFE_EVS_SendEvent[0].EventID, DS_FILE_NAME_ERR_EID);

} /* end DS_FileCreateName_Test_MaxPathnameLength */

void DS_FileCreateName_Test_PathBaseSeqTooLarge(void)
{
int32 FileIndex = 0;
Expand Down Expand Up @@ -991,7 +964,7 @@ void DS_FileCreateSequence_Test_ByTime(void)
int32 FileIndex = 0;
CFE_TIME_SysTime_t FakeTime;

char Sequence[DS_TOTAL_FNAME_BUFSIZE];
char Sequence[DS_TOTAL_FNAME_BUFSIZE] = "";

memset(&FakeTime, 0, sizeof(FakeTime));

Expand Down Expand Up @@ -1536,7 +1509,6 @@ void UtTest_Setup(void)
UT_DS_TEST_ADD(DS_FileCreateName_Test_NominalWithPeriod);
UT_DS_TEST_ADD(DS_FileCreateName_Test_EmptyPath);
UT_DS_TEST_ADD(DS_FileCreateName_Test_Error);
UT_DS_TEST_ADD(DS_FileCreateName_Test_MaxPathnameLength);
UT_DS_TEST_ADD(DS_FileCreateName_Test_PathBaseSeqTooLarge);
UT_DS_TEST_ADD(DS_FileCreateName_Test_PathBaseSeqExtTooLarge);
UT_DS_TEST_ADD(DS_FileCreateName_Test_ExtensionZero);
Expand Down