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

CF unit tests use incorrect dummy buffers #44

Closed
jphickey opened this issue Nov 23, 2021 · 0 comments · Fixed by #76
Closed

CF unit tests use incorrect dummy buffers #44

jphickey opened this issue Nov 23, 2021 · 0 comments · Fixed by #76
Milestone

Comments

@jphickey
Copy link
Contributor

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1780] CF unit tests use incorrect dummy buffers
Originally submitted by: Hickey, Joseph P. (GSFC-582.0)[VANTAGE SYSTEMS INC] on Tue Nov 16 16:59:31 2021

Original Description:
The CF unit tests contain an oft-repeated sequence to initialize a message pointer, for example:

    /* Arrange */
    pdu_r_msg_t dummy_ph;
    int local_result;

    CF_AppData.engine.in.msg = (CFE_SB_Buffer_t*)&dummy_ph;

This sequence is not valid for two reasons:

  1. Because the pdu_r_msg_t instance is not aligned appropriately to be cast to a CFE_SB_Buffer_t*. This invalid cast generates a warning on many compilers.

  2. Because the pdu_r_msg_t instance does not contain any additional data beyond the pdu_header_t value. Almost all CF calls will read beyond this header, depending on what the function call is, and some will write too. In the case of writing, this results in stack smashing, and the test may segfault/crash.

Recommendation is to create a union for the message buffer, which can address the alignment problem and also be used to reserve some extra space for data beyond the header that many calls do access.

It looks like this was done at one point in the "pdu_t" type (in cfdp.h) but this was commented out. Recommend reinstating this and using it in unit tests as it will be more correct.

jphickey added a commit to jphickey/CF that referenced this issue Nov 23, 2021
For unit unit tests that invoke CF PDU processing functions
on either input or output, ensure that the locally instantiated
"dummy" PDU is both sized sufficiently and aligned correctly.

This removes quite a bit of questionable casting between the
buffer types, and fixes a number of stack-smashing issues.

For completeness, this also clears (memset to 0) all instantiated
buffers, before setting values in the test.  This ensures that the
entire message structure has predictable/repeatable content.
jphickey added a commit to jphickey/CF that referenced this issue Dec 1, 2021
For unit unit tests that invoke CF PDU processing functions
on either input or output, ensure that the locally instantiated
"dummy" PDU is both sized sufficiently and aligned correctly.

This removes quite a bit of questionable casting between the
buffer types, and fixes a number of stack-smashing issues.

For completeness, this also clears (memset to 0) all instantiated
buffers, before setting values in the test.  This ensures that the
entire message structure has predictable/repeatable content.
astrogeco added a commit that referenced this issue Dec 8, 2021
Fix #44, #46, instantiate properly sized and aligned buffers
@skliper skliper added this to the Draco milestone Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants