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 #116, 117+118 - Multiple cleanups related to array indexing and range checking #127

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Nov 9, 2023

Checklist (Please check before submitting)

Describe the contribution
These commits depend on eachother, so are combined into one PR to avoid conflicts related to splitting and merging.

Commit 1: Fixes #116, scrub types used for indexing and identification

Introduces header files and dedicated types for the purpose of indexing
and identification of SC objects and structures.  This include ATS/RTS
identification, time sequence numbers, word offsets, and command
numbers.

Use a separate typedef for each, and document its purpose/intent in the
description, then update all FSW to use the typedef rather than a simple
uint16.

Note - in places where the CMD/TLM interface were _not_ using a uint16,
the old type is kept.  This maintains backward compatibility of the I/F.

Commit 2: Fixes #117, change local array refs to object pointer

This changes all "info" table access to go through an object pointer
that is obtained via an accessor method.

Commit 3: Fixes #118, implement range checking functions

Adds inline functions to do range checking, and uses them in all places
where the same logic had been copied around.  This reduces repetition of
logic.

Introduces proper data types and wrapper functions to deal with the
different types of IDs and indices.

Testing performed
Build and run all tests (Pending run with BVT setup)

Expected behavior changes
Cleaner codebase with more consistent error checking (or at least one step toward that)

System(s) tested on
Debian

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

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL-coding-standard found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

fsw/src/sc_atsrq.c Fixed Show resolved Hide resolved
fsw/src/sc_atsrq.c Dismissed Show resolved Hide resolved
@jphickey jphickey changed the title Fix #116, #117, #118, Multiple cleanups related to array indexing and range checking Fix #116, 117+118 - Multiple cleanups related to array indexing and range checking Nov 9, 2023
Introduces header files and dedicated types for the purpose of indexing
and identification of SC objects and structures.  This include ATS/RTS
identification, time sequence numbers, word offsets, and command
numbers.

Use a separate typedef for each, and document its purpose/intent in the
description, then update all FSW to use the typedef rather than a simple
uint16.

Note - in places where the CMD/TLM interface were _not_ using a uint16,
the old type is kept.  This maintains backward compatibility of the I/F.
This changes all "info" table access to go through an object pointer
that is obtained via an accessor method.
Adds inline functions to do range checking, and uses them in all places
where the same logic had been copied around.  This reduces repetition of
logic.

Introduces proper data types and wrapper functions to deal with the
different types of IDs and indices.
Adds additional range checks to avoid out-of-bounds array access
@jphickey
Copy link
Contributor Author

jphickey commented Nov 9, 2023

NOTE: had to add a fourth commit here (for #121), in order to resolve a (real/preexisting) bug that is now exposed due to the type cleanup.

@jphickey jphickey linked an issue Nov 9, 2023 that may be closed by this pull request
2 tasks
@dzbaker dzbaker merged commit ddeab42 into nasa:main Dec 5, 2023
17 checks passed
@jphickey jphickey deleted the fix-116-117-118 branch December 7, 2023 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment