CFE_PSP_MemCpy/Set not checking for NULL pointer args

[skliper]

In the psp/fsw/shared/cfe_psp_memutils.c, the CFE_PSP_MemCpy() and CFE_PSP_MemSet() do not check for null pointer arguments.

If a null pointer is passed then a segfault occurs. Both function signatures already have a return value for an error code.

However, we have an API problem in practice:

CFE_PSP_MemCpy() is called by these CFS apps: CF, CI, CS, FM, HK, io_lib, MD, MM, SBN, SBN653, and SC, by the cFE ES, EVS, FS, SB, and TBL but the function return value is not checked by any of them. The same goes for CFE_PS_MemSet(), it is also called by CFS apps: CF, CI, CS, DS, FM, io_lib, LC, LCX, MD, MM, SBN653, SC, SCH and TO. It is also called by cFE ES, EVS, FS, SB, TBL and TIME where none of them check the return value.

It appears that either the PSP return value was added later than most of the client development OR client developers just assumed a C-like behavior with no return values.

[skliper]

Imported from trac issue 31. Created by abrown4 on 2015-08-11T15:23:00, last modified: 2019-06-13T10:46:43

[skliper]

Trac comment by abrown4 on 2015-08-11 15:34:52:

Tested with commit: [changeset:a0a2a9b] & [changeset:05dfc40] Trac #14 Added white-box test for cfe_psp_memutils.c.

[skliper]

Trac comment by abrown4 on 2015-08-18 15:09:53:

commit: [changeset:5f89346] Trac #35 Added null pointer checks to CFE_PSP_MemCpy()/Set().

Added a return value of CFE_PSP_INVALID_POINTER for these null pointer arg conditions, but that's probably OK since no caller was checking them anyway. At least it'll prevent segfaults in the PSP.

[skliper]

Trac comment by abrown4 on 2015-08-18 15:16:00:

(merged to #14 branch)

[skliper]

Trac comment by jphickey on 2015-12-07 12:19:50:

CCB review: split back out from #14 merge

commit [changeset:8c9e827] contains the final change against "development" branch. trac-31-memcpy-set-null-args branch created for this.

[skliper]

Trac comment by glimes on 2015-12-07 12:51:43:

Milestone milestone1 deleted

[skliper]

Trac comment by jphickey on 2015-12-07 13:37:40:

Review comments:

This needs more justification as to why MemSet/MemCpy needs to return a non-success error code.

The C library version returns void and this has been perfectly fine for many years. If you supply a bad address results are undefined (likely producing an exception of some type).

As pointed out, nothing actually checks the return code of this, so while this change does not appear to //break// anything, it will not //benefit// anything either.

I also wrote CFE ticket [cfs_cfe:30] about this a while back.

The only justification for having an external "MemCpy" and/or "MemSet" wrapper function is if you need to specifically access certain //physical// memory ranges on a machine that uses an MMU. Wrapping memset/memcpy for the typical case of copying/setting directly accessible memory provides negative benefit because it reduces the compilers ability to optimize this (forcing it to be an out-of-line function call where it otherwise could have been inline)

To summarize, I recommend do not accept only because this will not have any effect on actual functionality.

That being said, it does not have any effect, so I'm neutral on this - it is OK to include this if others see some benefit here.

[skliper]

Trac comment by sstrege on 2015-12-08 10:48:51:

This code may prevent a segfault which is a benefit. However, since the higher level code is NOT checking the return value and appropriately responding to/alerting a CFE_PSP_INVALID_POINTER strange behaviors could occur that would be more hidden than if a segfault did occur.

If this change is accepted, I recommend updating the code to support a single return.

For safety critical software this is not a bad check to have in place. However, the calling applications and cFE will need to be updated to fully support this change. Before merging this change we need to ensure tickets are in place to update the calling code appropriately.

[skliper]

Trac comment by jphickey on 2015-12-08 11:03:18:

I would argue that before we accept this we need to have a more fundamental understanding/documentation of when to use CFE_PSP_MemSet() versus the memset() function as supplied by the C library (ditto for memcpy).

• Why does the PSP wrap these two functions
• What use-cases is the wrapper supposed to handle

I see many many occurrences of using CFE_PSP_MemSet() to initialize a local stack variable inside of a function. This is a negative benefit, since GCC's optimizer can no longer inline this function call (memset is one of those functions where it will use a builtin where possible, if you have enabled those optimization options).

When you are simply setting local memory or copying local memory to local memory, I cannot understand why we would want to use a wrapper function.

Also, local memory should not be failure-prone. Checking the return value from every CFE_PSP_MemCpy or CFE_PSP_MemSet operation is going to bog down the application code unnecessarily, and there is also typically no recovery mechanism here. If your local memory is bad, you are going to crash. We can't fix that in software by checking a return code.

If the failure is due to supplying a bad address (as the NULL check is trying to check for) then we have only checked for one specific one - but in reality there are MANY addresses that are probably out of range (why not check for address "1", since that is probably bad too?)

In summary, we can't prevent application code from doing something bad. I would generally prefer the application to crash/segfault if a bad address was passed to MemCpy/MemSet - this is almost certainly a bug in the code and if an application is buggy enough to pass in a bad address, it is certainly not correct enough to check that return code and clean up after itself. It is going to crash. This check will just delay it and make it more obscure.

[skliper]

Trac comment by sduran on 2015-12-14 17:56:50:

I always questioned why these wrapper functions were provided too. Our general rule is to check for null pointers, because they do cause seg faults. But if the calling code does not use the return code, the check is kinda pointless, as no action will be taken because, and in this case, that does obscure what is probably a bug to begin with, as Joe points out.

[skliper]

Trac comment by jphickey on 2018-07-24 14:55:12:

The plan as discussed on 2018-07-24 is to continue moving away from these wrapper functions.

• For the upcoming cFS release, every app will be updated to call the library function directly.
• We can include a compatibility macro similar to CFE_OMIT_DEPRECATED_6_6 to maintain compatibility with other existing applications just in case they are calling CFE_PSP_MemSet/MemCpy

[skliper]

Trac comment by jhageman on 2019-06-10 14:53:49:

Marked for CCB review - related to #10 suggest "wontfix", mark functions as deprecated)

[skliper]

Trac comment by jhageman on 2019-06-13 10:46:43:

CCB 6/12/19 - Agreed to deprecate, opened #97 to address this (rather than hijack this ticket). Closing as wontfix.