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 #2550, use resourceids for internal table validation and dump control blocks #2551

Merged
merged 2 commits into from
Jul 2, 2024
Merged

Fix #2550, use resourceids for internal table validation and dump control blocks #2551

merged 2 commits into from
Jul 2, 2024

Conversation

jphickey
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
Use a resourceID value for access into the dump control and validation result structure arrays. This allows for consistent lookup, matching, and free/in-use determination, as well as improved resilience to race conditions and stale data.

Fixes #2550

Testing performed
Build and run all tests, including functional and coverage. Sanity check all results.

Expected behavior changes
No externally visible changes, but API calls and external commands may be more strict about commands they accept/reject due to extra validation checking.

System(s) tested on
Debian

Additional context
Registry records will also be converted to use similar patterns, as a separate commit/PR.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 19, 2024
View reviewed changes
modules/tbl/fsw/src/cfe_tbl_internal.c
Comment on lines +1033 to +1034
if (CFE_TBL_DumpCtrlBlockIsUsed(DumpCtrlPtr) &&
CFE_RESOURCEID_TEST_EQUAL(DumpCtrlPtr->RegRecPtr->OwnerAppId, AppId))

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
modules/tbl/fsw/src/cfe_tbl_resource.c Fixed Show fixed Hide fixed
modules/tbl/fsw/src/cfe_tbl_resource.c Fixed Show fixed Hide fixed
modules/tbl/fsw/src/cfe_tbl_task_cmds.c
if (CFE_TBL_Global.DumpControlBlocks[i].State == CFE_TBL_DUMP_PERFORMED)
DumpCtrlPtr = &CFE_TBL_Global.DumpCtrlsJPHFIX[i];

if (CFE_TBL_DumpCtrlBlockIsUsed(DumpCtrlPtr) && DumpCtrlPtr->State == CFE_TBL_DUMP_PERFORMED)

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
modules/tbl/fsw/src/cfe_tbl_internal.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_TBL_ValidationResult_t *CFE_TBL_CheckValidationRequest(CFE_TBL_ValidationResultId_t *ValIdPtr)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/ut-coverage/tbl_UT.c
/*
* Tests the resource accessors for Validation Results
*/
void Test_CFE_TBL_ResourceID_ValidationResult(void)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/ut-coverage/tbl_UT.c
memset(ValResultPtr, 0, sizeof(*ValResultPtr));
}

void UT_TBL_SetupPendingDump(uint32 ArrayIndex, CFE_TBL_LoadBuff_t *DumpBufferPtr, CFE_TBL_RegistryRec_t *RegRecPtr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/ut-coverage/tbl_UT.c
@@ -113,6 +113,92 @@
}
}

/* Sets up the indicated validation request/result buffer as VALIDATION_PENDING */
void UT_TBL_SetupPendingValidation(uint32 ArrayIndex, bool UseActive, CFE_TBL_RegistryRec_t *RegRecPtr,

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
modules/tbl/fsw/src/cfe_tbl_api.c
/* Call the Application's Validation function for the static buffer */
Status = (RegRecPtr->ValidationFuncPtr)(RegRecPtr->Buffers[0].BufferPtr);
/* Save the result of the Validation function for the Table Services Task */
ResultPtr->Result = (RegRecPtr->ValidationFuncPtr)(BuffPtr->BufferPtr);

Check notice

Code scanning / CodeQL

Use of non-constant function pointer Note

This call does not go through a const function pointer.
modules/tbl/fsw/src/cfe_tbl_internal.h
@@ -57,7 +57,7 @@
* \param AccDescPtr Pointer to the current access descriptor
* \param Arg Opaque argument from caller (passed through)
*/
typedef void (* const CFE_TBL_AccessDescFunc_t)(CFE_TBL_AccessDescriptor_t *AccDescPtr, void *Arg);
typedef void (*const CFE_TBL_AccessDescFunc_t)(CFE_TBL_AccessDescriptor_t *AccDescPtr, void *Arg);

Check notice

Code scanning / CodeQL

Hidden pointer indirection Note

The typedef CFE_TBL_AccessDescFunc_t hides pointer indirection.
@jphickey jphickey force-pushed the fix-2550-tbl-val-dump-resourceid branch from ff5d60d to c27f114 Compare April 19, 2024 16:48
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 19, 2024
View reviewed changes
modules/tbl/fsw/src/cfe_tbl_resource.c
CFE_TBL_ValidationResult_t *ResultPtr;
uint32 Idx;

if (CFE_TBL_ValidationResultId_ToIndex(ValResultId, &Idx) == CFE_SUCCESS)

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
modules/tbl/fsw/src/cfe_tbl_resource.c
CFE_TBL_DumpControl_t *BlockPtr;
uint32 Idx;

if (CFE_TBL_DumpCtrlId_ToIndex(BlockId, &Idx) == CFE_SUCCESS)

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
modules/tbl/fsw/src/cfe_tbl_resource.c
* See description in header file for argument/return detail
*
*-----------------------------------------------------------------*/
CFE_TBL_ValidationResult_t *CFE_TBL_LocateValidationResultByID(CFE_TBL_ValidationResultId_t ValResultId)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 19, 2024
View reviewed changes
modules/tbl/ut-coverage/tbl_UT.c
DumpCtrlPtr->State = CFE_TBL_DUMP_PENDING;
DumpCtrlPtr->BlockId = CFE_TBL_DUMPCTRLID_C(PendingId);
DumpCtrlPtr->RegRecPtr = RegRecPtr;
DumpCtrlPtr->DumpBufferPtr = DumpBufferPtr;

Check warning

Code scanning / CodeQL

Local variable address stored in non-local memory Warning

A stack address which arrived via a
parameter
Loading
may be assigned to a non-local variable.
@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 19, 2024
@jphickey jphickey force-pushed the fix-2550-tbl-val-dump-resourceid branch from c27f114 to 1319746 Compare April 19, 2024 20:24
@jphickey
Fix nasa#2550, Use resource ID for validation index
1272f6d 
Use a resourceID value for access into the validation result
structure array.  This allows for consistent lookup, matching,
and free/in-use determination, as well as improved resilience
to race conditions and stale data.
@jphickey
Fix nasa#2550, Use resource ID for dump control index
b906671 
Use a resourceID value for access into the dump control
structure array.  This allows for consistent lookup, matching,
and free/in-use determination, as well as improved resilience
to race conditions and stale data.
@jphickey jphickey force-pushed the fix-2550-tbl-val-dump-resourceid branch from 1319746 to b906671 Compare April 19, 2024 20:43
@dzbaker
Copy link
Collaborator

dzbaker commented Apr 25, 2024

CCB 25 April 2024: Will review next week when @jphickey is back.

@dzbaker dzbaker added CCB:Provisionally-Approved and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels May 2, 2024
@dzbaker
Copy link
Collaborator

dzbaker commented May 2, 2024

CCB 2 May 2024: Approved pending BVT run.

@dzbaker dzbaker mentioned this pull request May 6, 2024
Merged
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request May 6, 2024
@dzbaker
IC: equuleus-rc1+dev12
770d1e7 
*Combines:*

cFE equuleus-rc1+dev141
osal equuleus-rc1+dev66
PSP equuleus-rc1+dev42

**Includes:**

*cFS*
- #751
- #762

*cFE*
- nasa/cFE#2537
- nasa/cFE#2381
- nasa/cFE#2551

*osal*
- nasa/osal#1460

*PSP*
- nasa/PSP#430

Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com>
Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
@dzbaker dzbaker mentioned this pull request May 6, 2024
Merged
2 tasks
@dzbaker dzbaker merged commit 0a253d9 into nasa:main Jul 2, 2024
22 checks passed
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Provisionally-Approved labels Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use proper resource identifers for table validation and dump state structs
2 participants
@jphickey @dzbaker