Argument checks in internal functions (CFE_SB_TransmitMsgValidate, etc)

[jphickey]

Is your feature request related to a problem? Please describe.
As discussed in #1197 we need to have some consensus on the proper level of argument checking for internal helper functions.

Sometimes internal functions have tests to validate their inputs (range check etc) on behalf of the caller, in the case where several public APIs need to repeat the same tests -- putting these in a helper can reduce repeated code and make all APIs consistent in their validation (a good thing).

But in other cases the helper is invoked from contexts where the inputs are never out of range or pointers can never be NULL. Testing for such inputs can be redundant.

Describe the solution you'd like
Need to confirm/reach consensus on whether functions like CFE_SB_TransmitMsgValidate() in CFE SB need to validate all their arguments. Probably should better document which args are tested and why - and if there are limitations on other args (e.g. certain args are assumed to be non-null).

Additional context
This just causes some confusion during review and probably some additional comments/documentation could help.

See thread here: #1197 (comment)

Requester Info
Joseph Hickey, Vantage Systems, Inc.

[jphickey]

To summarize - the original subject that brought up this issue was about CFE_SB_TransmitMsgValidate():

• This is an internal SB function that consolidates some common logic between CFE_SB_TransmitMsg and CFE_SB_TransmitBuffer
• It accepts four pointer arguments. First (MsgPtr) is an input, the other 3 are output buffers.
• It validates the input MsgPtr on behalf of the caller. This is because the pointer may have originated outside SB (i.e. it is an unknown/uncontrolled value) and as such coding standards require that pointer inputs from outside sources are checked for NULL before dereferencing.
• It does NOT validate the other three pointers - the outputs. These all point to local stack variables in the caller, and so can never be NULL in practice.

The questions are:

1. Given this functions role does it need to accept NULL on any input pointer, like a public API would?
2. Does it need to ensure all outputs are written/set even if it returns non-success?

There is also a similar issue described in #1194 where a common helper routine is invoked from a context with known-good inputs, and so the error checking on the output is skipped. There are other places where the same function is invoked where the input is not known to be good - so the range check inside the function is intended for those cases. But it is called from multiple contexts - and the question there is whether the caller needs to check for error returns even though it is not possible for it to fail in the current context where the input is known to be good.

[skliper]

To be honest I don't think we had different opinions on this (after discussion). As an internal function it can do validation as a "helper" to the APIs, but doesn't require validation on the parameters that are guaranteed valid by the calling function. I thought the open issue was more for setting outputs on errors - bf8f481.

Really just 2.

I see #1194 as a separate issue (even from 2.), can we ignore possible returns when using an API from an internal function. We'd never do this for a system call even if we know it should never error, or likely any API from outside the core... can we really ignore it for our APIs? Implementations change...

[skliper]

CCB:2021-03-17:

1. recommendation to comment where helpers do checks to be clear when/why, and why not others ("internal" pointers don't need checking)
2. Initialize in caller is the least logic (error checking and setting internally could get messy), good enough to squash the few warnings where static analyzers are confused
3. See CFE_ES_CheckCounterIdSlotUsed doesn't handle error case (NULL dereference) - static analysis warning #1194 (short story, add if test)

[skliper]

Action forward for this issue - document internal checks.