CFE_EVS_WriteLogFileCmd requires recursive locking

[thewonderidiot]

Describe the bug
The function CFE_EVS_WriteLogFileCmd in cfe_evs_log.c locks CFE_EVS_GlobalData.EVS_SharedDataMutexID for the entire duration of the function. If an error is encountered, it will call EVS_SendEvent, which also tries to lock the mutex. If the OS/OSAL do not support recursive locking on mutexes, this will lead to a deadlock.

To Reproduce
Steps to reproduce the behavior:

1. Cause an error during the execution of CFE_EVS_WriteLogFileCmd (by e.g. giving it an invalid filename).

Expected behavior
This may be expected behavior, if cFE requires its mutexes to be recursively lockable. If so, this isn't a bug and I'll have to modify our OSAL. But if that is not the expectation, then I would this function should relinquish its lock before calling EVS_SendEvent.

More specifically, it looks like the this can be accomplished by moving the line

   OS_MutSemGive(CFE_EVS_GlobalData.EVS_SharedDataMutexID);

to just before each instance of EVS_SendEvent() in the function. By each of these points, this function is done accessing sensitive EVS data.

Code snips

• CFE_EVS_WriteLogFileCmd's release of the lock

System observed on:

• Capella flight computer
• OS: FreeRTOS 10.1.1
• Versions: cFE 6.5.0, OSAL 4.2.0 (plus in-house FreeRTOS port)

Additional context
I found this while testing our fix for #372.

Reporter Info
Mike Stewart, Capella Space.

[jphickey]

Has this bug been checked in the current version?

This bug is for cFE 6.5.0. The logging code was considerably updated in CFE 6.6.0.

[jphickey]

I checked the code and concur that this still does apply to the current version. The function is now called CFE_EVS_WriteLogDataFileCmd() but it is basically the same.

We don't support recursive locking. The code must be updated to avoid calling EVS_SendEvent while holding the lock. There are several calls to this function. As a general style it is preferred to have a single sem give that corresponds to a single sem take, and a code path that always goes through either both or neither.

My vote would be to defer the actual EVS_SendEvent until the process is done. It looks like it can only be a write error or success. I can implement this.