vxworks osapi.c OS_Milli2Ticks() problems

[skliper]

Two problems found:

1. OS_Milli2Ticks() returns type 'int32' from variable declared as 'uint32'.
2. There are no limit checks on input value. The calculation may generate overflow on the resulting output type.

Identified with #45 white-box coverage testing.

[skliper]

Imported from trac issue 81. Created by abrown4 on 2015-07-17T14:15:39, last modified: 2019-08-14T14:11:46

[skliper]

Trac comment by abrown4 on 2015-07-21 14:39:05:

commit: [changeset:67e90ac] Trac #104, fixed OS_Milli2Ticks() out of range bugs.

Improved error detection to eliminate negative values, fixed types, added explicit casts, and added explicit range check.

[skliper]

Trac comment by abrown4 on 2015-07-24 17:19:47:

Feedback: Don't use a hardcoded number. Return OS_ERROR instead of an incorrect value.

[skliper]

Trac comment by abrown4 on 2015-07-24 18:53:28:

commit: [changeset:0383f17] Trac #104, Fixed osapi.c OS_Milli2Ticks() to return OS_ERROR. Also removed hardcoded number in favor of including limits.h.

[skliper]

Trac comment by abrown4 on 2015-08-20 13:37:10:

commit: [changeset:442d759] reverted the above commits after 8/19 code review inspection. The inspection discussion resolved that it would be better to separate the returned ticks and an error code indication. Combining the two ideas into a single return value is too risky for callers to blindly use. All the call sites for OS_Milli2Ticks() need to be tracked down and corrected as well.

This problem remains.

[skliper]

Trac comment by jhageman on 2019-02-28 15:16:28:

Please confirm condition still exists, and if so provide estimated work complete date or detail what external inputs are needed to be able to provide a work complete date.

[skliper]

Neither this function OS_Milli2Ticks or OS_Tick2Micros are great. Worth a review of use cases.

[jphickey]

It is not clear to me why an application (high level task) would need to deal directly with the concept of OS ticks. This should be entirely abstracted by OSAL and the mere presence of a function like this is really pointing to an issue elsewhere, something OSAL isn't sufficiently abstracting.

I would like to see these two functions deprecated.

[astrogeco]

CCB 2020-03-25: No objection

[skliper]

@tngo67 @ejtimmon - we OK to deprecate OS_Milli2Ticks and OS_Tick2Micros?

[ejtimmon]

@tngo67 @ejtimmon - we OK to deprecate OS_Milli2Ticks and OS_Tick2Micros?

No objection from me. The GSFC apps do not use these functions.

[tngo67]

@tngo67 @ejtimmon - we OK to deprecate OS_Milli2Ticks and OS_Tick2Micros?

No issue for us either.

[skliper]

Just making OS_Milli2Ticks an internal function doesn't really solve the issues, since it's reachable from several other API's and timeout isn't checked before it's passed in. In at least the case I'm looking at now, timeout is int32 from the API also (OS_QueueGet). Do we add checks to timeout values at the shared layer (>= 0, < whatever the rollover limit would be)?

EDIT - -1 is OK as wait forever... so -1, or >=0 and < rollover value?

[skliper]

What to do in a case where the requested value in milliseconds would roll over? Wait up to the maximum number of ticks and timeout? Reject the input as invalid and return immediately?

[skliper]

CCB 2020-08-12: Return immediately an error and check for error cases internally