Fix crash caused by race condition in xSemaphoreTake #173
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This race condition would make infinisim eventually crash with the message "Mutex released without being held" on my machine. The function `xSemaphoreTake` waits for the semaphore to be available to take by looping + sleeping until there is an element in the `pxQueue->queue`. Then it locks the `pxQueue->mutex` and checks the queuesize again. Since the first check is not done under the mutex, some other thread could remove the element from the queue before the mutex is locked. Then the second check in `xSemaphoreTake` will return false. The method `DateTimeController::CurrentDateTime()` uses a semaphore as a mutex to guard calls to `UpdateTime()`. But it (and the other methods from `DateTimeController`) do not check if `xSemaphoreTake` actually gave them the semaphore. They will try to give it back even if they failed to take it, which leads to the crash in `xSemaphoreGive`. The other thread in this crash was calling the watchface refresh method, which also calls `CurrentDateTime()`. To fix the race condition, lock the mutex guarding `pxQeue->queue` before checking the queue size and unlock before sleeping. When implemented like this, `xSemaphoreTake` from InfiniSim follows the API from FreeRTOS more closely, which waits the full `xTicksToWait` while InfiniSim would abort prematurely in some cases. With the assumption that `UpdateTime()` will finish within some finite amount of time, this make the methods in `DateTimerController` work even without error handling since they have such long wait times.
NeroBurner
requested changes
Mar 9, 2025
Collaborator
NeroBurner
left a comment
NeroBurner
left a comment
There was a problem hiding this comment.
Tiny nitpicks. Other than that a BIG thank you! The semaphore crash was driving me crazy for a while, but I didn't find time to investigate.
Thanks for the thorough explanation and methodical fix!
…TEMPTS Since xTicksToWait is TickType_t (uint32) and SDL_Delay takes uint32 as well, we should avoid any chance of funny business due to differences in signedness.
Contributor
Author
|
I was wondering if someone else was effected by this, on my machine it would occur pretty frequently. I am happy to help out. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #172.
This race condition would make infinisim eventually crash with the message "Mutex released without being held" on my machine.
The function
xSemaphoreTakewaits for the semaphore to be available to take by looping + sleeping until there is an element in thepxQueue->queue. Then it locks thepxQueue->mutexand checks the queuesize again. Since the first check is not done under the mutex, some other thread could remove the element from the queue before the mutex is locked. Then the second check inxSemaphoreTakewill return false. The methodDateTimeController::CurrentDateTime()uses a semaphore as a mutex to guard calls toUpdateTime(). But it (and the other methods fromDateTimeController) do not check ifxSemaphoreTakeactually gave them the semaphore. They will try to give it back even if they failed to take it, which leads to the crash inxSemaphoreGive. The other thread in this crash was calling the watchface refresh method, which also callsCurrentDateTime().To fix the race condition, lock the mutex guarding
pxQeue->queuebefore checking the queue size and unlock before sleeping. When implemented like this,xSemaphoreTakefrom InfiniSim follows the API from FreeRTOS more closely, which waits the fullxTicksToWaitwhile InfiniSim would abort prematurely in some cases.With the assumption that
UpdateTime()will finish within some finite amount of time, this makes the methods inDateTimerControllerwork even without error handling since they have such long wait times.