CFE_ES_ScanAppTable() possible race conditions

[skliper]

The CFE_ES_ScanAppTable() is called from the ES message processing thread, but it does not lock the global ES data structure when reading/writing from the global data.

ES software bus command messages are safe because this function is called by the same thread that is processing those messages, therefore concurrency is not possible here.

But other functions, like CFE_ES_DeleteApp() and CFE_ES_ReloadApp() are part of the public API and these also update the same fields within the app state data structure that CFE_ES_ScanAppTable() is reading. CFE_ES_RunLoop() can also modify fields within this structure and this is called by pretty much every app in the system.

This issue was noted while reviewing the fix for a similar issue in #279.

[skliper]

Imported from trac issue 264. Created by jphickey on 2019-03-25T17:01:55, last modified: 2019-06-24T11:44:20

CFE_ES_RunLoop, CFE_ES_DeleteApp, and CFE_ES_ReloadApp all lock ES shared data already.
It may be possible just to guard the call to CFE_ES_ScanAppTable in the ES main task.
Before doing that, it is probably worth a more thorough analysis of how this data is locked in these various calls. We don't want to introduce a deadlock by just locking/unlocking data around the CFE_ES_ScanAppTable when ES calls it.

[jphickey]

I've looked into this already and I have a proposal/plan which is mostly implemented but just needs to be tested on all the various platforms.

The proposal is to introduce a more consistent notion of a "background job" in ES. We have several of these scattered about, including the Performance Log dump (separate ticket #456) and they are all implemented differently and inconsistently.

Worth noting that ScanAppTable has other issues besides the locking, in particular the timers are based on "scan intervals" which are not consistent, as they happen after every message (incl. SCH "send tlm") as well as after a period of message inactivity.

At any rate, my proposal is to have ES start a single, low-priority thread for all of these background jobs. The jobs will be implemented as state machines which perform some (limited) work on each invocation. The prototype will be:

bool (*RunFunc)(uint32 ElapsedTime, void *Arg);

The ElapsedTime parameter is the amount of time elapsed (based on OS clock) since the last time the job was executed, Arg is a generic/opaque state structure. The function returns false if the job is idle (no work to do) or true if it is actively doing something.

Both the periodic ScanAppTable and the Performance Log file dump fit nicely into this model, as well as (probably) any other longer-running jobs that occur in ES.