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 #390, OS_SelectFd... API's check that Set != NULL #391

Closed

Conversation

CDKnightNASA
Copy link
Contributor

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

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.

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

skliper commented Mar 26, 2020

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

@astrogeco astrogeco changed the title fix for #390 - OS_SelectFd... API's check that Set != NULL Fix #390, OS_SelectFd... API's check that Set != NULL Apr 1, 2020
@astrogeco astrogeco added CCB - 20200401 CCB:Approved Indicates code review and approval by community CCB and removed CCB:Approved Indicates code review and approval by community CCB labels Apr 1, 2020
@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label Apr 1, 2020
@astrogeco
Copy link
Contributor

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

@astrogeco astrogeco removed the CCB:Approved Indicates code review and approval by community CCB label Apr 1, 2020
@CDKnightNASA
Copy link
Contributor Author

whoops, accidentally closed this one

@CDKnightNASA CDKnightNASA reopened this Apr 3, 2020
@CDKnightNASA
Copy link
Contributor Author

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

jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022


Fixes nasa#379, Fixes nasa#380, Fixes nasa#383, Fixes nasa#384,
Fixes nasa#385, Fixes nasa#392
Code reviewed and approved at 20191106 and 20191113 CB
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS_SelectFdZero/OS_SelectFdAdd/OS_SelectFdClear/OS_SelectFdIsSet does not ensure Set is != NULL
4 participants