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.

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

Fix #546 #547, api argument validation #1140

Merged

Conversation

zanzaben
Copy link
Contributor

@zanzaben zanzaben commented Jan 29, 2021

Describe the contribution
Fixes #546
Fixes #547
Add validation for method parameters. Mostly null pointer checks for #547 and a few size checks for #546

Fixes #1119
Returning the input instead of an error code

Testing performed
Build and run unit test

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 20.04

Additional Context
Here is the function list from #546 and the changes done

cfe_es_api.c:CFE_ES_DeleteApp - Can get a segmentation fault if user tries to delete an APP greater than CFE_PLATFORM_ES_MAX_APPLICATIONS
CFE_ES_LocateAppRecordByID gets a pointer that is either valid or null

cfe_es_api.c:CFE_ES_ReloadApp - Can Result in Segmentation fault if APID is invalid
CFE_ES_LocateAppRecordByID gets a pointer that is either valid or null

cfe_es_api.c:CFE_ES_CreateChildTask - Input Argument 'Flags' is not validated…also it does not appear to be used anywhere, consider removing
Not changing functions in this task

cfe_es_api.c:CFE_ES_GetAppName - Consider comparing BufferLength with OS_MAX_API_NAME prior to use.
No invalid buffer length

cfe_es_api.c:CFE_ES_RegisterCDS - Consider checking if block size is less than CFE_PLATFORM_ES_MAX_BLOCK_SIZE
CFE_ES_RegisterCDSEx checks BlockSize

cfe_es_perf.c:CFE_ES_PerfLogAdd - Should check if EntryExit is either a 0 or 1
It's internal and the macro that calls it will only pass in valid values

cfe_fs_api.c:CFE_FS_InitHeader - SubType not checked
The API doesn't have a limitation for SubType

cfe_sb_api.c:CFE_SB_SubscribeFull - Quality is not checked…consider checking that it is 0 or 1
Quality is not nessacarly 1 or 0

cfe_sb_api.c:CFE_SB_ZeroCopyGetPtr - Is there a maximum message size? Consider verifying MsgSize prior to use.
Checks MsgSize

cfe_sb_api.c:CFE_SB_SubscribeLocal - MsgLim is not checked…if a max limit does exist, should add argument validation
a max limit doesn't exists

cfe_sb_util.c:CFE_SB_SetUserDataLength - Consider verifying Length of user data (if there exists a limit) and/or TotalMsgSize
Checks TotalMsgSize
cfe_tbl_api.c:CFE_TBL_GetAddresses - Can result in Segmentation fault if NumTables grows larger than max number of tables.
Needs to be enforced by user

cfe_tbl_api.c:CFE_TBL_ReleaseAddresses - Should check to make sure NumTables is less than CFE_PLATFORM_TBL_MAX_NUM_TABLES
Needs to be enforced by user

Here is a list of the functions from #547 and their new current state

cfe_es_api.c:CFE_ES_CalculateCRC
checks for NULL
cfe_es_api.c:CFE_ES_CopyToCDS
checks for NULL
cfe_es_api.c:CFE_ES_CreateChildTask
checks for NULL
cfe_es_api.c:CFE_ES_GetAppID
checks for NULL
cfe_es_api.c:CFE_ES_GetAppName
checks for NULL
cfe_es_api.c:CFE_ES_GetGenCounterIDByName
checks for NULL
cfe_es_api.c:CFE_ES_GetTaskInfo
checks for NULL
cfe_es_api.c:CFE_ES_ProcessCoreException
Method no longer exists
cfe_es_api.c:CFE_ES_RegisterCDS
checks for NULL
cfe_es_api.c:CFE_ES_RestoreFromCDS
checks for NULL
cfe_es_api.c:CFE_ES_RunLoop
Can be called with NULL
cfe_es_api.c:CFE_ES_WriteToSysLog
checks for NULL
cfe_esmempool.c:CFE_ES_GetMemPoolStats
checks for NULL
cfe_esmempool.c:CFE_ES_GetPoolBuf
checks for NULL
cfe_esmempool.c:CFE_ES_GetPoolBufInfo
checks for NULL
cfe_esmempool.c:CFE_ES_PoolCreate
Calls CFE_ES_PoolCreateEx which checks for NULL
cfe_esmempool.c:CFE_ES_PoolCreateEx
checks for NULL
cfe_esmempool.c:CFE_ES_PoolCreateNoSem
Calls CFE_ES_PoolCreateEx which checks for NULL
cfe_esmempool.c:CFE_ES_PutPoolBuf
checks for NULL
cfe_evs.c:CFE_EVS_SendEvent
checks for NULL
cfe_evs.c:CFE_EVS_SendEventWithAppID
checks for NULL
cfe_evs.c:CFE_EVS_SendTimedEvent
checks for NULL
cfe_fs_api.c:CFE_FS_InitHeader
checks for NULL
cfe_fs_api.c:CFE_FS_ReadHeader
checks for NULL
cfe_fs_api.c:CFE_FS_SetTimestamp
No pointer to check
cfe_fs_api.c:CFE_FS_WriteHeader
checks for NULL
cfe_sb_api.c:CFE_SB_CreatePipe
checks for NULL
cfe_sb_api.c:CFE_SB_ZeroCopyGetPtr
checks for NULL
cfe_sb_msg_id_util.c:CFE_SB_GetMsgId
Deprecated
cfe_sb_msg_id_util.c:CFE_SB_SetMsgId
Deprecated
cfe_sb_util.c:CFE_SB_GenerateChecksum
Deprecated
cfe_sb_util.c:CFE_SB_GetChecksum
Deprecated
cfe_sb_util.c:CFE_SB_GetCmdCode
Deprecated
cfe_sb_util.c:CFE_SB_GetMsgTime
Deprecated
cfe_sb_util.c:CFE_SB_GetTotalMsgLength
Deprecated
cfe_sb_util.c:CFE_SB_GetUserData
checks for NULL
cfe_sb_util.c:CFE_SB_GetUserDataLength
checks for NULL
cfe_sb_util.c:CFE_SB_InitMsg
Deprecated
cfe_sb_util.c:CFE_SB_MessageStringGet
checks for NULL
cfe_sb_util.c:CFE_SB_MessageStringSet
checks for NULL
cfe_sb_util.c:CFE_SB_MsgHdrSize
checks for NULL
cfe_sb_util.c:CFE_SB_SetCmdCode
Deprecated
cfe_sb_util.c:CFE_SB_SetMsgTime
Deprecated
cfe_sb_util.c:CFE_SB_SetTotalMsgLength
Deprecated
cfe_sb_util.c:CFE_SB_SetUserDataLength
checks for NULL
cfe_sb_util.c:CFE_SB_TimeStampMsg
calls CFE_MSG_SetMsgTime which checks for NULL
cfe_sb_util.c:CFE_SB_ValidateChecksum
Deprecated
cfe_tbl_api.c:CFE_TBL_GetAddress
checks for NULL
cfe_tbl_api.c:CFE_TBL_GetAddresses
checks for NULL
cfe_tbl_api.c:CFE_TBL_GetInfo
checks for NULL
cfe_tbl_api.c:CFE_TBL_Load
checks for NULL
cfe_tbl_api.c:CFE_TBL_Register
checks for NULL
cfe_tbl_api.c:CFE_TBL_Share
checks for NULL
cfe_time_api:CFE_TIME_Print
checks for NULL
cfe_time_api:CFE_TIME_RegisterSynchCallback
checks for NULL

Contributor Info - All information REQUIRED for consideration of pull request
Alex Campbell GSFC

@zanzaben zanzaben force-pushed the fix546_API_argument_validation branch from bd84409 to be35514 Compare February 2, 2021 14:51
@zanzaben zanzaben changed the title WIP Fix #546 #547, api argument validation Fix #546 #547, api argument validation Feb 3, 2021
@zanzaben zanzaben marked this pull request as ready for review February 3, 2021 18:10
@zanzaben zanzaben added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 8, 2021
@skliper
Copy link
Contributor

skliper commented Feb 16, 2021

@zanzaben can you address conflicts so we can get this in?

@zanzaben zanzaben force-pushed the fix546_API_argument_validation branch 3 times, most recently from b8a6409 to d2b26a3 Compare February 16, 2021 20:44
@zanzaben zanzaben removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 17, 2021
@astrogeco
Copy link
Contributor

CCB 2021-02-17 PENDING fixes and conflicts.

@zanzaben zanzaben force-pushed the fix546_API_argument_validation branch 2 times, most recently from 790c222 to c85ff3b Compare February 18, 2021 17:20
@zanzaben zanzaben added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Feb 18, 2021
@skliper
Copy link
Contributor

skliper commented Feb 24, 2021

@zanzaben - for the "Not needed" responses, can you add justification? Looking for the "why" to capture/record the valuable analysis you did for future reference.

fsw/cfe-core/src/es/cfe_es_api.c Outdated Show resolved Hide resolved
@@ -2147,7 +2153,13 @@ CFE_SB_Buffer_t *CFE_SB_ZeroCopyGetPtr(size_t MsgSize,
CFE_ES_GetAppID(&AppId);
zcd->AppID = AppId;

(*BufferHandle) = (CFE_SB_ZeroCopyHandle_t) zcd;
if(BufferHandle != NULL){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might have conflicts with zero copy updates related to CFE_SB_ZeroCopyHandle_t

@astrogeco astrogeco added CCB:2021-02-24 and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Feb 24, 2021
@zanzaben zanzaben force-pushed the fix546_API_argument_validation branch from c85ff3b to 5b7be60 Compare February 24, 2021 15:55
@astrogeco
Copy link
Contributor

astrogeco commented Feb 24, 2021

CCB 2021-02-24 APPROVED

  • change int32 to new error type

@astrogeco
Copy link
Contributor

@skliper and @zanzaben are we good to merge this? Looks like there are some open discussions remaining.

@zanzaben
Copy link
Contributor Author

zanzaben commented Mar 2, 2021

I am good for it to be merged.

@skliper
Copy link
Contributor

skliper commented Mar 2, 2021

@astrogeco looks fine to me, may have conflicts with the zero copy updates but that's probably in IC?

@astrogeco astrogeco changed the base branch from main to integration-candidate March 2, 2021 18:06
@astrogeco
Copy link
Contributor

I am good for it to be merged.

Just changed the target to IC and the conflicts popped up, can you fix them?

@zanzaben zanzaben force-pushed the fix546_API_argument_validation branch from 1b9b242 to 260570d Compare March 2, 2021 18:25
@zanzaben
Copy link
Contributor Author

zanzaben commented Mar 2, 2021

@astrogeco Conflicts fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants