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

Improper use of unions in LC_GetSizedWPData #72

Closed
2 tasks done
jphickey opened this issue Feb 13, 2023 · 0 comments · Fixed by #73
Closed
2 tasks done

Improper use of unions in LC_GetSizedWPData #72

jphickey opened this issue Feb 13, 2023 · 0 comments · Fixed by #73

Comments

@jphickey
Copy link
Contributor

Checklist (Please check before submitting)

  • I reviewed the Contributing Guide.
  • I performed a cursory search to see if the bug report is relevant, not redundant, nor in conflict with other tickets.

Describe the bug
The LC_GetSizedWPData function is not using the LC_MultiType_t union properly. It is writing to one member and then reading from another, different member of the same union. This is "type punning" and may not work in an optimized build.

The new version of CppCheck reports this issue.

To Reproduce
Run Cppcheck workflow to see issue.
No known way to actually produce a failure though, as most platforms will behave as the code expects it to, its just not guaranteed to work.

Expected behavior
Should not read from a different union member than was written to.

Code snips
This writes to Unsigned8 but then reads from Signed8:

LC/fsw/src/lc_watch.c

Lines 854 to 855 in f49a965

TempBuffer.Unsigned8 = *WPDataPtr;
ConvBuffer.Signed32 = TempBuffer.Signed8; /* Extend signed 8 bit value to 32 bits */

Additionally, Many cases write to Signed32 but only Unsigned32 is read here at the end:

*SizedDataPtr = ConvBuffer.Unsigned32;

System observed on:
N/A

Additional context
This code does work as intended but is not necessarily safe/portable across platforms in its current form, particularly when optimization is enabled.

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

jphickey added a commit to jphickey/LC that referenced this issue Feb 13, 2023
Use ISO C standard value conversion, do not rely on platform-dependent
union access.
dzbaker added a commit that referenced this issue Feb 16, 2023
@dmknutsen dmknutsen added this to the Equuleus milestone May 26, 2023
@chillfig chillfig added the bug label Sep 5, 2023
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.

3 participants