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 #1458, Moves OS_strnlen to public API and adds static analysis co… …mments #1465

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

chillfig
Copy link
Contributor

@chillfig chillfig commented Jun 13, 2024

Checklist (Please check before submitting)

Describe the contribution

Testing performed
Manual inspection

Expected behavior changes
N/A

System(s) tested on
N/A

Additional context
N/A

Third party code
N/A

Contributor Info - All information REQUIRED for consideration of pull request
Justin Figueroa, Vantage Systems

@chillfig chillfig self-assigned this Jun 13, 2024
@chillfig chillfig added CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) bug labels Jun 13, 2024
@@ -215,6 +215,7 @@ static inline int64 OS_TimeGetTotalMicroseconds(OS_time_t tm)
*/
static inline OS_time_t OS_TimeFromTotalMicroseconds(int64 tm)
{
/* SAD: Overflow is unlikely, requiring an input equivalent to over 29.23 years in microseconds. */
Copy link
Contributor Author

@chillfig chillfig Jun 13, 2024

Choose a reason for hiding this comment

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

CCB 06/13/2024: Change "Overflow is unlikely..." to "This is not considered a concern because...etc"
Also verify 29.23 years. Probably 29,230 years

@chillfig chillfig force-pushed the SA_jsc2_1 branch 5 times, most recently from 4cbbcf3 to 0ab1fb3 Compare June 18, 2024 20:06
@chillfig chillfig changed the title Fix #1458, Adds JSC 2.1 Static Analysis comments Fix #1458, Adds integer overflow protection to osapi-clock multiplication Jun 20, 2024
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

Definitely some mixed feelings about this. I thought we had concurred that because an overflow would only occur if the interval was over 29000 years, that it was OK as-is?

Seems like we are just shifting risk here -- although the tool might no longer complain about this, we haven't made the code any more correct. That is, if you pass in an interval that is beyond the representable range, then returning INT64_MAX is not more (or less) correct than returning some unspecified/platform-defined value. Both results would be equally incorrect.

In order to justify this change, all callers of this API would have to check for the INT64_MAX result, and (theoretically?) take some alternate action. But that is not feasible - all these tests are not free, they take time and CPU power and increase the code size and they need to be unit-tested and functional-tested and increase our future maintenance load. All for something that only occurs in the highly unlikely event that a user needs to represent a time interval greater than 29000 years, and probably won't be handled correctly anyway?

In summary - I prefer the previous approach of simply documenting the limits of what OS_time_t can represent, and instructions on how to adjust this (note that the user can change the time tick to represent a larger range or better precision, whatever is more important)

@chillfig chillfig changed the title Fix #1458, Adds integer overflow protection to osapi-clock multiplication Fix #1458, Moves OS_strnlen to public API and adds static analysis co… …mments Jun 21, 2024
…s comments

This commit addresses issues flagged during static analysis by:
- Adding JSC 2.1 disposition comments.
- Making OS_strnlen publicly accessible and replacing strlen with it.
jphickey added a commit to chillfig/osal that referenced this pull request Jun 24, 2024
Calls to "OS_strnlen" are likely needed to return an actual length,
so make a default handler that does return a length.  The value
may still be overridden in tests, though.
Calls to "OS_strnlen" are likely needed to return an actual length,
so make a default handler that does return a length.  The value
may still be overridden in tests, though.
* Default handler implementation for 'OS_strnlen' stub
* -----------------------------------------------------------------
*/
void UT_DefaultHandler_OS_strnlen(void *UserObj, UT_EntryKey_t FuncKey, const UT_StubContext_t *Context)

Check notice

Code scanning / CodeQL

Long function without assertion Note

All functions of more than 10 lines should have at least one assertion.
size_t retval;
int32 status;

if (UT_Stub_GetInt32StatusCode(Context, &status))

Check warning

Code scanning / CodeQL

Side effect in a Boolean expression Warning

This Boolean expression is not side-effect free.
@dzbaker dzbaker added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Jun 27, 2024
dzbaker added a commit to nasa/cFS that referenced this pull request Jul 2, 2024
*Combines:*

cFE equuleus-rc1+dev137
osal equuleus-rc1+dev66
elf2cfetbl equuleus-rc1+dev56

**Includes:**

*cFS*
- #707
- #773

*cFE*
- nasa/cFE#2560
- nasa/cFE#2344
- nasa/cFE#2359
- nasa/cFE#2376
- nasa/cFE#2496
- nasa/cFE#2554
- nasa/cFE#2568
- nasa/cFE#2566

*osal*
- nasa/osal#1456
- nasa/osal#1465

*elf2cfetbl*
- nasa/elf2cfetbl#147
- nasa/elf2cfetbl#124
- nasa/elf2cfetbl#125
- nasa/elf2cfetbl#127

Co-authored by: Avi Weiss <thnkslprpt@users.noreply.github.com>
Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com>
Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Jacob Hageman <skliper@users.noreply.github.com>
Co-authored by: Anh Van <avan989@users.noreply.github.com>
@dzbaker dzbaker mentioned this pull request Jul 2, 2024
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request Jul 2, 2024
*Combines:*

cFE equuleus-rc1+dev137
osal equuleus-rc1+dev66
elf2cfetbl equuleus-rc1+dev56

**Includes:**

*cFS*
- #707
- #773

*cFE*
- nasa/cFE#2560
- nasa/cFE#2344
- nasa/cFE#2359
- nasa/cFE#2376
- nasa/cFE#2496
- nasa/cFE#2554
- nasa/cFE#2568
- nasa/cFE#2566

*osal*
- nasa/osal#1456
- nasa/osal#1465

*elf2cfetbl*
- nasa/elf2cfetbl#147
- nasa/elf2cfetbl#124
- nasa/elf2cfetbl#125
- nasa/elf2cfetbl#127

Co-authored by: Avi Weiss <thnkslprpt@users.noreply.github.com>
Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com>
Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Jacob Hageman <skliper@users.noreply.github.com>
Co-authored by: Anh Van <avan989@users.noreply.github.com>
@dzbaker dzbaker merged commit ac7c9f5 into nasa:main Jul 2, 2024
19 checks passed
dzbaker added a commit to nasa/cFS that referenced this pull request Jul 2, 2024
*Combines:*

cFE equuleus-rc1+dev137
osal equuleus-rc1+dev66
elf2cfetbl equuleus-rc1+dev56

**Includes:**

*cFS*
- #707
- #773

*cFE*
- nasa/cFE#2560
- nasa/cFE#2344
- nasa/cFE#2359
- nasa/cFE#2376
- nasa/cFE#2496
- nasa/cFE#2554
- nasa/cFE#2568
- nasa/cFE#2566

*osal*
- nasa/osal#1456
- nasa/osal#1465

*elf2cfetbl*
- nasa/elf2cfetbl#147
- nasa/elf2cfetbl#124
- nasa/elf2cfetbl#125
- nasa/elf2cfetbl#127

Co-authored by: Avi Weiss <thnkslprpt@users.noreply.github.com>
Co-authored by: Justin Figueroa <chillfig@users.noreply.github.com>
Co-authored by: Joseph Hickey <jphickey@users.noreply.github.com>
Co-authored by: Jacob Hageman <skliper@users.noreply.github.com>
Co-authored by: Anh Van <avan989@users.noreply.github.com>
dzbaker added a commit to nasa/cFS that referenced this pull request Jul 15, 2024
*Combines:*

cFE equuleus-rc1+dev167
osal equuleus-rc1+dev81

**Includes:**

*cFE*
- nasa/cFE#2560

*osal*
- nasa/osal#1456
- nasa/osal#1465

Co-authored by: Anh Van <avan989@users.noreply.github.com>
Co-authored by: Dan Knutsen <dmknutsen@users.noreply.github.com>
@dzbaker dzbaker mentioned this pull request Jul 15, 2024
2 tasks
dzbaker added a commit to nasa/cFS that referenced this pull request Jul 15, 2024
*Combines:*

cFE equuleus-rc1+dev167
osal equuleus-rc1+dev81

**Includes:**

*cFE*
- nasa/cFE#2560

*osal*
- nasa/osal#1456
- nasa/osal#1465

Co-authored by: Anh Van <avan989@users.noreply.github.com>
Co-authored by: Dan Knutsen <dmknutsen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug CCB:Approved Indicates code review and approval by community CCB
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static analysis issues JSC 2.1
3 participants