Fix #390, OS_SelectFd... API's check that Set != NULL

[CDKnightNASA]

Describe the contribution
fix to OS_SelectFdZero/Set/Add/Clear/IsSet to check that Set is not NULL

Testing performed
Will commit unit tests that check this as a separate pull request for #377 .

Expected behavior changes
API calls should return OS_POINTER_ERROR when Set is NULL. (IsSet returns "false".)

System(s) tested on
Haven't tested yet, #377 will test.

Contributor Info - All information REQUIRED for consideration of pull request
Christopher.D.Knight@nasa.gov

[jphickey]

A few comments...

The pattern employed elsewhere for this type of thing is that simple parameter validation is done first and it can use an inline "return" statement. So can't we just add a simple statement:

if (Set == NULL)
{
    return OS_INVALID_POINTER;
}

And thereby keep the remainder of all the functions exactly the same?

I have a strong suspicion that cppcheck analysis is going to complain about the multiple sets of return code. (i.e. setting of the value and re-setting of the value before the original was used).

Lastly, more of a gripe -- we do realize that this is only checking for one specific bad pointer value, out of a possible >4 billion (32-bit), right? And using CPU cycles to do so? And masking/obfuscating the real error in the process? But if "coding standards" mysteriously says this is better, we do it!

[CDKnightNASA]

A few comments...

The pattern employed elsewhere for this type of thing is that simple parameter validation is done first and it can use an inline "return" statement. So can't we just add a simple statement:

if (Set == NULL)
{
    return OS_INVALID_POINTER;
}

I thought the CCB frowned upon returns/breaks as they might skip a cleanup action? I actually prefer straight returns...

Lastly, more of a gripe -- we do realize that this is only checking for one specific bad pointer value, out of a possible >4 billion (32-bit), right? And using CPU cycles to do so? And masking/obfuscating the real error in the process? But if "coding standards" mysteriously says this is better, we do it!

I am also OK if the CCB says "we don't bother to protect against NULL/invalid pointers."

We could get into more exotic options like a "constructor" that adds a header to the struct and then every call can check that the header is there and valid. :D

[jphickey]

if (Set == NULL)
{
    return OS_INVALID_POINTER;
}

I thought the CCB frowned upon returns/breaks as they might skip a cleanup action? I actually prefer straight returns...

Initial/immediate checks of input parameters are OK/preferred to do this way. The rule of thumb is anything that can be tested/validated in a stateless manner using only constants and local stack variables and no external dependencies, do it immediately upon entry to the function and use an inline return if the test fails.

Once the function gets past the point where it starts touching any global state tables, locks, or basically doing anything with an external side effect, it should carry though to the end of the function to make sure anything it did is properly undone, in a consistent manner.

What should be especially avoided is taking/locking some resource and then repeating the cleanup actions each time a validation test is done. This is a prime opportunity for errors to happen as things evolve (violates DRY principle).

[jphickey]

I am also OK if the CCB says "we don't bother to protect against NULL/invalid pointers."

Yep, I'm not expecting any action on this, which is why I qualified it as a "gripe" and will continue to do so. It is mainly to raise awareness as to the ridiculousness of expending CPU cycles testing for one particular bad value out of the billions of possible bad values, while simultaneously obfuscating the error making the code less debuggable in the process. (maybe someday the coding standards can evolve...)

Your previous suggestions of putting in an assert-like macro to test for this stuff would be quite useful, because then at least the user has the opportunity to select a more appropriate action if/when these tests fail.

[skliper]

Yup, coding standard thing. Thou shalt check for null pointers passed to APIs... (not in those exact words).

[astrogeco]

CCB 2020-04-01 - Discussed, approved as long as it doesn't change API.

[CDKnightNASA]

whoops, accidentally closed this one

[CDKnightNASA]

actually want to re-do as part of #377 and will simplify to just return rather than nested if()