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 improperly uses the EventCount member for incorrect appID filtering #897

Closed
jphickey opened this issue Sep 22, 2020 · 1 comment · Fixed by #898 or #936
Closed

EVS improperly uses the EventCount member for incorrect appID filtering #897

jphickey opened this issue Sep 22, 2020 · 1 comment · Fixed by #898 or #936
Assignees
Labels
Milestone

Comments

@jphickey
Copy link
Contributor

Describe the bug
The EventCount member of the EVS internal app data is intended to track the number of events sent by a particular app.

BUT - it is also overloaded to track if the event came from the wrong app, here:

/* Send only one "not registered" event per application */
if (AppDataPtr->EventCount == 0)
{
/* Increment count of "not registered" applications */
CFE_EVS_GlobalData.EVS_TlmPkt.Payload.UnregisteredAppCounter++;
/* Indicate that "not registered" event has been sent for this app */
AppDataPtr->EventCount++;

The problem is that this is, by definition, invoked when the calling AppID is wrong (not registered), but it is changing a field that is also potentially used for valid (registered) AppIDs.

To Reproduce
This can theoretically occur if an app calls CFE_EVS_SendEventWithAppId() using an old AppID value, for instance if an app was restarted it gets unregistered, and then gets a different AppID but refs to the old value could still exist. The new AppID doesn't necessarily have the same slot in the table - in fact it shouldn't. The old table entry might refer to a totally different app.

So if this happens it will corrupt/change the EventCount member on an unrelated app data entry.

This can be seen in the EVS telemetry, where if the "unregistered" event occurs it inadvertently creates a nonzero AppMessageSentCounter in the TLM data on an unrelated app that happens to share that slot in the table.

Also, if the counter was already nonzero because the table entry is in use by another (registered) app then this prevents the notification about the unregistered app from appearing at all.

Expected behavior
Should not overload the EventCounter to track a basically unrelated item - which is whether or not an "unregistered" event occurred on a different app that happened to map to the same entry.

Simple fix would be to just introduce a separate field to track this.

System observed on:
Ubuntu 20.04

Reporter Info
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey self-assigned this Sep 22, 2020
@jphickey
Copy link
Contributor Author

I have a fix for this, it is simple but it depends on some pending PRs ... will push as draft and rebase once dependencies are satisfied.

@jphickey jphickey added the bug label Sep 22, 2020
jphickey added a commit to jphickey/cFE that referenced this issue Sep 22, 2020
Do not use EventCount to track whether an "unregistered" event
was generated, because that by definition came from a different app
than the one that "owns" that field.

This just adds a separate field to track that state, so it doesn't
potentially modify the counter for an unrelated app.
jphickey added a commit to jphickey/cFE that referenced this issue Sep 29, 2020
Do not use EventCount to track whether an "unregistered" event
was generated, because that by definition came from a different app
than the one that "owns" that field.

This just adds a separate field to track that state, so it doesn't
potentially modify the counter for an unrelated app.
@astrogeco astrogeco added this to the 7.0.0 milestone Oct 1, 2020
astrogeco added a commit that referenced this issue Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants