-
Notifications
You must be signed in to change notification settings - Fork 215
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
Conversation
src/os/inc/osapi-clock.h
Outdated
@@ -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. */ |
There was a problem hiding this comment.
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
4cbbcf3
to
0ab1fb3
Compare
There was a problem hiding this 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)
…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.
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
size_t retval; | ||
int32 status; | ||
|
||
if (UT_Stub_GetInt32StatusCode(Context, &status)) |
Check warning
Code scanning / CodeQL
Side effect in a Boolean expression Warning
*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>
*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>
*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>
*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>
*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>
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