Skip to content

Fix nasa#231, to_lab app: strcpy() does not check bound.#232

Merged
elizabash merged 6 commits into
devfrom
fix-231-to-lab-use-of-strcpy
Jun 15, 2026
Merged

Fix nasa#231, to_lab app: strcpy() does not check bound.#232
elizabash merged 6 commits into
devfrom
fix-231-to-lab-use-of-strcpy

Conversation

@elizabash

@elizabash elizabash commented May 26, 2026

Copy link
Copy Markdown
Contributor

Checklist (Please check before submitting)

Describe the contribution
Static analyzer via CodeSonar found two usages of 'strcpy' in the to_lab App. 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 #231

More Info
Code Sonar error: BADFUNC.BO.STRCPY : Use of strcpy
Summary: A use of strcpy() or a similar function (see full list below), which are vulnerable to buffer overflows.
Resolution: Use a function that does bounds checking, such as strncpy(), StrCbCopy(), or StrCchCopy().

The issue seems to be a concern over buffer overflow. The source string being copied over to a destination string could potentially cause issues if, for example, the source string doesn't have a null terminator for some reason. That would mean garbage could potentially be added into the destination string.

Strncpy can add null characters, omit them, and pad them depending on the size you pass.
If the source string is shorter than n, strncpy copies it and pads with null characters until n bytes.
If the source string length is equal to or longer than n, strncpy copies exactly n bytes with NO null terminator.
Because of this, you must always add a terminator yourself when using strncpy for string copying.

Common practice while using strncpy:
strncpy(dest, src, sizeof(dest) - 1); //sizeof()-1 makes sure there will be room for null terminator
dest[sizeof(dest) - 1] = '\0'; //make sure the destination string has a null terminator

Testing performed

  1. make distclean
  2. make native_std.prep
  3. make native_std.install
  4. make native_std.runtest
  5. cd build-native_std/native/default_cpu1/
  6. ctest
  7. Run Cosmos
  8. 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.

@elizabash elizabash self-assigned this May 26, 2026
@elizabash elizabash marked this pull request as draft May 28, 2026 17:24
elizabeth.e.ash@nasa.gov added 2 commits June 9, 2026 09:52
@elizabash elizabash force-pushed the fix-231-to-lab-use-of-strcpy branch from d2c5a5d to c12ea2d Compare June 9, 2026 15:12
@elizabash elizabash marked this pull request as ready for review June 9, 2026 15:25
@elizabash

elizabash commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

As the code stands today, stcpy is fine as there is no worry for an overflow. PipeName and ToTlmPipeName's sizes are 16 and the strings being copied into them each have 15 characters plus room for a null terminator.
However, it may be best to follow CodeSonar's suggestion and implement some bounds checking now in case this code changes in the future. Which is why this PR uses strncpy, which limits bounds, instead of stcpy.

@elizabash elizabash marked this pull request as draft June 11, 2026 14:38
… such as copying a string from one variable to a local variable, when using the the original variable works the same but with less steps.
@elizabash elizabash marked this pull request as ready for review June 15, 2026 11:22
@elizabash

Copy link
Copy Markdown
Contributor Author

During the cFS Core Framework & Maintenance Code Reviews meeting on 6/11, I brought up this PR and my above comment. Joe suggested instead of using using strcpy or strncpy at all, the code could use some cleanup as there were unnecessary actions being done. Copying one hard coded string into a local variable only to use that local variable once was just adding additional unnecessary tasks. The code is now cleaned up.

@jphickey jphickey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this ... its simpler!

@elizabash elizabash merged commit 56394ae into dev Jun 15, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TO_Lab App: Use of strcpy

3 participants