Fix nasa#1545, osal use of strcpy

[elizabash]

Checklist (Please check before submitting)

• I reviewed the Contributing Guide.
• I signed and emailed the appropriate Contributor License Agreement to GSFC-SoftwareRelease@mail.nasa.gov and copied cfs-program@lists.nasa.gov. (This issue was discussed in a cfs meeting. I think that counts!)

Describe the contribution
Static analyzer via CodeSonar found a usage of 'strcpy' in osal. Strcpy() does not check bounds and should be replaced with something that does, like strncpy(). strncpy(dest, src, sizeof(dest)-1);
Note: When using strncpy, it’s safer to pass sizeof(dest) - 1 instead of sizeof(dest) to reserve space for the null terminator and avoid unterminated strings.
Fixes #1545

Testing performed

1. make distclean
2. make native_std.prep
3. make native_std.install
4. make native_std.runtest (just in case)
5. Note: I still can't log into CodeSonar to check the static analysis results. It would be great if a reviewer could check what CodeSonar has to say. Update: I have CodeSonar access now!

[joelsherrill]

....strncpy(dest, src, sizeof(dest)-1);
Note: When using strncpy, it’s safer to pass sizeof(dest) - 1 instead of sizeof(dest) to reserve space for the null terminator and avoid unterminated strings.

The intent of this is correct but you have to be very careful to actually specify the size of the dest. In the call to strncpy() you gave as an example, dest and src would be pointers. Thus sizeof(dest)-1 will result in 3 or 7 depending on whether you have 32 or 64 bit pointers.

I have become attentive to this because it seems to be a common mistake. At least in the code I get asked to review or help debug.

[jphickey]

I'm not completely sold on this. There was a comment in the code explaining (validly) that this had already been evaluated and it is known that the two buffers being copied between are the same size and the input is null terminated.

Codesonar just pointed out that strcpy has this concern, it didn't say anything was wrong with it in this use case.

strncpy is not quite the panacea that people make it out to be. Its actually sort of misleading to say "just replace strcpy with strncpy" ... its not quite that simple. In this particular circumstance, If the input string was too long or otherwise not null terminated within the expected bounds, in the previous code it would overrun the buffer until it hit a NUL char ... but in this change it only "fixes" the overrun. You would be left with a Local_SegmentName that is not null terminated.

But since we already know the buffers are of the same length, we know this won't happen, so changing to strncpy does not fix anything at all. The only thing it will end up doing is zero-filling the remaining unused chars in the local buffer with NULs.

As this is just UT assert and not performance-sensitive in any way, the extra zero-filling is not a big deal. We might want to merge this just to make the warning go away, but understand that if there actually was a problem in the old code calling strcpy, this new code calling strncpy would also have a problem just the same. It would not fix it.

[elizabash]

Sorry about the couple of force-pushes. I made two minor errors in the commit message, which caused workflow errors. One commit message had a double quotation around it, which was failing cppcheck. Any future push to fix it still failed cppcheck because it seemed the double quotes were in the history. Next, I wrote "Fix 1545" instead of "Fix #1545", which also caused cppcheck to fail as well. I ended up doing a few "git rebase -i HEAD~2" to get rid of those pushes with bad commit messages.