Fix #546 #547, api argument validation#1140
Merged
astrogeco merged 2 commits intonasa:integration-candidatefrom Mar 2, 2021
Merged
Fix #546 #547, api argument validation#1140astrogeco merged 2 commits intonasa:integration-candidatefrom
astrogeco merged 2 commits intonasa:integration-candidatefrom
Conversation
bd84409 to
be35514
Compare
Contributor
|
@zanzaben can you address conflicts so we can get this in? |
b8a6409 to
d2b26a3
Compare
Contributor
|
CCB 2021-02-17 PENDING fixes and conflicts. |
790c222 to
c85ff3b
Compare
Contributor
|
@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. |
skliper
reviewed
Feb 24, 2021
fsw/cfe-core/src/sb/cfe_sb_api.c
Outdated
| zcd->AppID = AppId; | ||
|
|
||
| (*BufferHandle) = (CFE_SB_ZeroCopyHandle_t) zcd; | ||
| if(BufferHandle != NULL){ |
Contributor
There was a problem hiding this comment.
Might have conflicts with zero copy updates related to CFE_SB_ZeroCopyHandle_t
c85ff3b to
5b7be60
Compare
Contributor
|
CCB 2021-02-24 APPROVED
|
5b7be60 to
1b9b242
Compare
Contributor
Contributor
Author
|
I am good for it to be merged. |
Contributor
|
@astrogeco looks fine to me, may have conflicts with the zero copy updates but that's probably in IC? |
Contributor
Just changed the target to IC and the conflicts popped up, can you fix them? |
1b9b242 to
260570d
Compare
Contributor
Author
|
@astrogeco Conflicts fixed |
astrogeco
added a commit
to nasa/cFS
that referenced
this pull request
Mar 4, 2021
Combines: - nasa/cFE#1196 - nasa/osal#835 - nasa/elf2cfetbl#72 Includes: - nasa/cFE#1154 - nasa/cFE#935 - nasa/cFE#1179 - nasa/cFE#1140 - nasa/cFE#1178 - nasa/cFE#1197 - nasa/osal#834 - nasa/osal#826 - nasa/osal#827 - nasa/osal#829 - nasa/osal#824 - nasa/elf2cfetbl#71
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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