anjiahao1 opened a new pull request, #18338:
URL: https://github.com/apache/nuttx/pull/18338
## Summary
* **Why change is necessary**: Mutex recursion (a task attempting to lock a
mutex it already holds) is not allowed in NuttX and indicates a programming
error. Currently, such attempts may fail silently or cause unexpected behavior.
Adding a debug assertion helps catch these bugs early during development and
testing.
* **What functional part of the code is being changed**: The
`nxsem_wait_slow()` function in `sched/semaphore/sem_wait.c`, which handles the
slow path of semaphore/mutex acquisition.
* **How does the change exactly work**: A `DEBUGASSERT` is added to verify
that the current task (obtained via `nxsched_gettid()`) is not already the
holder of the mutex being acquired. The assertion checks the mutex holder bits
(excluding the blocking bit) against the current task ID. If a task attempts to
recursively lock a mutex it already holds, the assertion will trigger, alerting
developers to the bug.
* **Related NuttX Issue reference**: None
* **Related NuttX Apps Issue / Pull Request reference**: None
## Impact
* **Is new feature added? Is existing feature changed?** YES - Existing
feature enhanced. A debug-time safety check is added to the mutex acquisition
path to detect illegal recursion attempts.
* **Impact on user**: NO - This is a debug-only assertion (DEBUGASSERT) that
only triggers in debug builds. Production builds are unaffected. Users who have
mutex recursion bugs will now get clear assertion failures instead of silent
failures.
* **Impact on build**: NO - The change is minimal (5 lines added) and does
not affect build process or configuration.
* **Impact on hardware**: NO - This is a pure software change in the
scheduler/semaphore subsystem with no hardware-specific implications.
* **Impact on documentation**: NO - No documentation updates required. This
is an internal debug enhancement.
* **Impact on security**: NO - This change improves robustness by catching
programming errors earlier, which is a positive security practice.
* **Impact on compatibility**: NO - Backward compatible. Debug assertions
are non-breaking; they only help catch bugs.
* **Anything else to consider**: This change follows NuttX best practices
for defensive programming. The assertion is placed at the critical point where
mutex recursion would be detected, making it effective for catching bugs during
development and testing.
## Testing
I confirm that changes are verified on local setup and works as intended:
* **Build Host(s)**: ubuntu24.04 + arm-none-eabi-gcc 12.2.1
* **Target(s)**: mps3-an547:nsh
* **Run command**: `qemu-system-arm -M mps3-an547 -m 2G -nographic -kernel
nuttx.bin`
Testing logs before change:
```
N/A (baseline log not captured - this is a debug assertion enhancement)
```
Testing logs after change:
```
qemu-system-arm -M mps3-an547 -m 2G -nographic -kernel nuttx.bin
NuttShell (NSH) NuttX-12.7.2-vela
nsh>
nsh> ostest
stdio_test: write fd=1
stdio_test: Standard I/O Check: printf
stdio_test: write fd=2
ostest_main: putenv(Variable1=BadValue3)
ostest_main: setenv(Variable1, GoodValue1, TRUE)
ostest_main: setenv(Variable2, BadValue1, FALSE)
ostest_main: setenv(Variable2, GoodValue2, TRUE)
ostest_main: setenv(Variable3, GoodValue3, FALSE)
ostest_main: setenv(Variable3, BadValue2, FALSE)
show_variable: Variable=Variable1 has value=GoodValue1
show_variable: Variable=Variable2 has value=GoodValue2
show_variable: Variable=Variable3 has value=GoodValue3
ostest_main: Started user_main at PID=4
user_main: Begin argument test
user_main: Started with argc=5
user_main: argv[0]="ostest"
user_main: argv[1]="Arg1"
user_main: argv[2]="Arg2"
user_main: argv[3]="Arg3"
user_main: argv[4]="Arg4"
End of test memory usage:
VARIABLE BEFORE AFTER
======== ======== ========
arena 801fb944 801fb944
ordblks 2 2
mxordblk 7ffffff0 7ffffff0
uordblks 641c 641c
fordblks 801f5528 801f5528
user_main: getopt() test
getopt(): Simple test
getopt(): Invalid argument
getopt(): Missing optional argument
getopt_long(): Simple test
getopt_long(): No short options
getopt_long(): Argument for --option=argument
getopt_long(): Invalid long option
getopt_long(): Mixed long and short options
getopt_long(): Invalid short option
getopt_long(): Missing optional arguments
getopt_long_only(): Mixed long and short options
getopt_long_only(): Single hyphen long options
[... extensive test output showing all tests passing ...]
user_main: mutex test
Initializing mutex
Starting thread 1
Starting thread 2
Thread1 Thread2
Loops 32 32
Errors 0 0
Testing moved mutex
Starting moved mutex thread 1
Starting moved mutex thread 2
Thread1 Thread2
Moved Loops 32 32
Moved Errors 0 0
[... all remaining tests pass successfully ...]
user_main: vfork() test
vfork_test: Child 85 ran successfully
Final memory usage:
VARIABLE BEFORE AFTER
======== ======== ========
arena 801fb944 801fb944
ordblks 2 6
mxordblk 7ffffff0 7ffffff0
uordblks 641c 64b4
fordblks 801f5528 801f5490
user_main: Exiting
ostest_main: Exiting with status 0
stdio_test: Standard I/O Check: fprintf to stderr
nsh> QEMU: Terminated
```
**Test Result**: PASS - All ostest tests completed successfully, including
mutex tests. The debug assertion does not interfere with normal mutex
operation. No assertion failures detected, indicating no mutex recursion
attempts in the test suite.
## PR verification Self-Check
* [X] This PR introduces only one functional change.
* [X] I have updated all required description fields above.
* [X] My PR adheres to Contributing Guidelines and Documentation (git commit
title and message, coding standard, etc).
* [ ] My PR is still work in progress (not ready for review).
* [X] My PR is ready for review and can be safely merged into a codebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
