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

EVS_GenerateEventTelemetry doesn't handle vsnprintf error cases #1195

Closed
skliper opened this issue Mar 1, 2021 · 2 comments · Fixed by #1235 or #1243
Closed

EVS_GenerateEventTelemetry doesn't handle vsnprintf error cases #1195

skliper opened this issue Mar 1, 2021 · 2 comments · Fixed by #1235 or #1243
Milestone

Comments

@skliper
Copy link
Contributor

skliper commented Mar 1, 2021

Is your feature request related to a problem? Please describe.
vsnprintf can return negative error values, but is compared to unsigned int to handle truncation:

ExpandedLength = vsnprintf((char *)LongEventTlm.Payload.Message, sizeof(LongEventTlm.Payload.Message), MsgSpec, ArgPtr);
/* Were any characters truncated in the buffer? */
if (ExpandedLength >= sizeof(LongEventTlm.Payload.Message))

I wouldn't call this a bug (will just pass the initialized to zero string), but might be worth a unique message?

Describe the solution you'd like
Explicitly handle failure (and cast for comparison)

Describe alternatives you've considered
Place termination character at the start? Any other way to provide clues.

Additional context
Static analysis warning for coercion alters value.

Requester Info
Jacob Hageman - NASA/GSFC

@skliper skliper added this to the 7.0.0 milestone Mar 1, 2021
@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 17, 2021
@skliper
Copy link
Contributor Author

skliper commented Mar 17, 2021

Looking for opinions - could just cast sizeof to (int) which wouldn't do anything (message would still be empty, which is current behavior), could write something unique to message, could do an additional debug event (recursion), could write to the system log... simple "cFE_EVS_SendEvent vsnprintf error %d", or strerror(errno), or explain_vsnprintf().

@jphickey
Copy link
Contributor

It wouldn't hurt to first test for a negative result before the truncation test at line 417. This way it will be ensured that the truncation test does not implicitly do a sign conversion.

As far as what to do -- it really shouldn't fail -- so anything is highly unlikely to occur. Perhaps a WriteToSyslog, but that uses the same function (I think). Just return early and skip the rest maybe?

skliper added a commit to skliper/cFE that referenced this issue Mar 17, 2021
skliper added a commit to skliper/cFE that referenced this issue Mar 17, 2021
skliper added a commit to skliper/cFE that referenced this issue Mar 17, 2021
skliper added a commit to skliper/cFE that referenced this issue Mar 17, 2021
astrogeco added a commit that referenced this issue Mar 18, 2021
Fix #1195, Avoid implicit conversion from vsnprintf errors
@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants