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

Add an assert o catch overflow of recursive mutex counter #1254

Merged
merged 4 commits into from
Mar 4, 2025

Conversation

InnerSteff
Copy link
Contributor

Fix add overflow check on recursive mutex counter

Description

There is no overflow check of the recursive mutex counter yet.
If the counter overflows, the mutex will be released on the next call of xQueueGiveMutexRecursive(),
even if the number of calls does not match the number of calls of xQueueTakeMutexRecursive().

This would lead to unpredictable behavior in the application.

Test Steps

call xQueueTakeMutexRecursive() more than the maximum value of UBaseType_t

Checklist:

  • I have tested my changes. No regression in existing tests.
  • I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@InnerSteff InnerSteff requested a review from a team as a code owner March 3, 2025 07:53
@aggarg
Copy link
Member

aggarg commented Mar 3, 2025

Does this change attempt to catch infinite recursion of xQueueGiveMutexRecursive? Wouldn't an application result in stack overflow before that happens. Is this a theoretical fix or did you face any problem?

@InnerSteff
Copy link
Contributor Author

This change attempt to catch an infinite call of xSemaphoreTakeRecursive().

It’s a theoretical bug fix I came across during design phase of my application when someone asked how often you can take a recursive mutex.

But, according to my understanding, it can not only occur in an infinite recursion scenario, but also if xSemaphoreTakeRecursive() is called in an infinite loop. Or in a loop with more iteration than the max value of UBaseType_t.
An (infinite) loop would not lead to a stack overflow.

@aggarg
Copy link
Member

aggarg commented Mar 3, 2025

But, according to my understanding, it can not only occur in an infinite recursion scenario, but also if xSemaphoreTakeRecursive() is called in an infinite loop. Or in a loop with more iteration than the max value of UBaseType_t.
An (infinite) loop would not lead to a stack overflow.

Agree, but that would be a poorly written application. No sane application would do that. Asserts are there to help programmers to catch reasonable mistakes.

aggarg
aggarg previously approved these changes Mar 4, 2025
Signed-off-by: Gaurav Aggarwal <[email protected]>
Copy link

sonarqubecloud bot commented Mar 4, 2025

@aggarg aggarg merged commit 742729e into FreeRTOS:main Mar 4, 2025
17 checks passed
@aggarg aggarg changed the title Fix add overflow check on recursive mutex counter Add an assert o catch overflow of recursive mutex counter. Mar 4, 2025
@aggarg aggarg changed the title Add an assert o catch overflow of recursive mutex counter. Add an assert o catch overflow of recursive mutex counter Mar 4, 2025
@InnerSteff
Copy link
Contributor Author

Thanks for picking up and merging my pull request, but may I ask what changed your mind to accept it?

@aggarg
Copy link
Member

aggarg commented Mar 4, 2025

In our internal discussion, we realized that this check may help to catch a situation where somebody keeps taking a mutex but never gives it back.

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants