This is an automated email from the ASF dual-hosted git repository.
xiaoxiang781216 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/nuttx.git
The following commit(s) were added to refs/heads/master by this push:
new 31fbb992189 sched/semaphore: Keep a negative task id out of the mutex
holder.
31fbb992189 is described below
commit 31fbb9921899325fc0c17e39cda90d0d621c4eae
Author: Justin Hammond <[email protected]>
AuthorDate: Tue Aug 4 19:15:06 2026 +0800
sched/semaphore: Keep a negative task id out of the mutex holder.
A mutex records its holder as a task id in the low 31 bits of a word
whose top bit means "someone is blocked on this". The id was stored
without masking, so an id with its top bit set became a holder with the
blocking bit raised.
Task ids are normally small and positive, but not always.
nxsched_gettid() reports -ESRCH for a context that no longer maps to a
running task, and there is a window where that is exactly what the
running context is: nxtask_exit() marks the next task ready to run
while the dying task is still executing on its own stack, and only then
releases the TCB. Freeing the group inside that release takes and
drops the group's mutexes, so the lock stores 0xfffffffd and the unlock
compares 0x7ffffffd, which are not equal.
With assertions enabled the unlock trips its holder check, and every
exit of a process that frees memory panics. In a kernel build that is
every exit, so no program could be run twice, and running one at all
took the shell down with it. Without assertions the failure is silent:
the accidental blocking bit sends the unlock looking for a waiter that
never existed.
Encode the id the same way everywhere it is stored or compared, so that
a lock and an unlock from one context agree whatever the id's sign.
The masked forms of -1 and -2 would alias the "no holder" and "reset"
values, but nxsched_gettid() yields only valid ids and -ESRCH.
mm_lock() already sidesteps this window with a note that gettid() may
return -ESRCH during a context switch; this gives the generic mutex the
same footing rather than a second special case.
Test case, on the EIC7700 EVB, which is a kernel build with assertions:
nsh> hello
Hello, World!!
Before, that printed and then panicked in sem_post, taking the shell
with it, every time. After, five runs in a row complete and the shell
survives. ps over telnet still completes.
Assisted-by: Claude:claude-opus-5
Signed-off-by: Justin Hammond <[email protected]>
---
include/nuttx/semaphore.h | 16 ++++++++++++++++
libs/libc/semaphore/sem_trywait.c | 2 +-
libs/libc/semaphore/sem_wait.c | 2 +-
sched/semaphore/sem_post.c | 7 +++++--
sched/semaphore/sem_trywait.c | 2 +-
sched/semaphore/sem_wait.c | 8 ++++++--
6 files changed, 30 insertions(+), 7 deletions(-)
diff --git a/include/nuttx/semaphore.h b/include/nuttx/semaphore.h
index 769da8baa4b..4178a51b474 100644
--- a/include/nuttx/semaphore.h
+++ b/include/nuttx/semaphore.h
@@ -74,6 +74,22 @@
#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&((s)->val.mholder))
+/* Make a mutex holder value from a task id. The holder field keeps it in
+ * the low 31 bits because bit 31 is the blocking flag, and a task id can
+ * be negative here: nxsched_gettid() reports -ESRCH for a context that no
+ * longer maps to a task, which is exactly the state a task is in while
+ * nxtask_exit() tears it down and its group's mutexes get their final
+ * lock and unlock. Storing such an id unmasked would raise the blocking
+ * flag by accident, and an unlock would then try to wake waiters that do
+ * not exist. Masking keeps a lock and unlock from the same context
+ * consistent with each other, whatever the id's sign.
+ *
+ * The ids -1 and -2 would alias NXSEM_MRESET and NXSEM_NO_MHOLDER, but
+ * nxsched_gettid() produces only valid ids and -ESRCH, which is -3.
+ */
+
+#define NXSEM_MAKE_MHOLDER(tid) ((uint32_t)(tid) & ~NXSEM_MBLOCKING_BIT)
+
/* Check if holder value (TID) is not NO_HOLDER or RESET */
#define NXSEM_MACQUIRED(h) (((h) & NXSEM_NO_MHOLDER) != NXSEM_NO_MHOLDER)
diff --git a/libs/libc/semaphore/sem_trywait.c
b/libs/libc/semaphore/sem_trywait.c
index 8a62ac9bd88..a7a8bd39894 100644
--- a/libs/libc/semaphore/sem_trywait.c
+++ b/libs/libc/semaphore/sem_trywait.c
@@ -152,7 +152,7 @@ int nxsem_trywait(FAR sem_t *sem)
return -EAGAIN;
}
- new = _SCHED_GETTID();
+ new = NXSEM_MAKE_MHOLDER(_SCHED_GETTID());
}
else
{
diff --git a/libs/libc/semaphore/sem_wait.c b/libs/libc/semaphore/sem_wait.c
index 36fb106bfc5..20b73a02f95 100644
--- a/libs/libc/semaphore/sem_wait.c
+++ b/libs/libc/semaphore/sem_wait.c
@@ -180,7 +180,7 @@ int nxsem_wait(FAR sem_t *sem)
break;
}
- new = _SCHED_GETTID();
+ new = NXSEM_MAKE_MHOLDER(_SCHED_GETTID());
}
else
{
diff --git a/sched/semaphore/sem_post.c b/sched/semaphore/sem_post.c
index 3ffa4e8e890..8018a1d869a 100644
--- a/sched/semaphore/sem_post.c
+++ b/sched/semaphore/sem_post.c
@@ -99,11 +99,14 @@ int nxsem_post_slow(FAR sem_t *sem)
mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKING_BIT);
/* Mutex post from another thread is not allowed, unless
- * called from nxsem_reset
+ * called from nxsem_reset. The comparison uses the same encoding
+ * as the lock side so that a context whose id is -ESRCH, which is
+ * what a task being torn down reports, still matches its own lock.
*/
DEBUGASSERT(mholder == (NXSEM_MBLOCKING_BIT | NXSEM_MRESET) ||
- (mholder & (~NXSEM_MBLOCKING_BIT)) == nxsched_gettid());
+ (mholder & (~NXSEM_MBLOCKING_BIT)) ==
+ NXSEM_MAKE_MHOLDER(nxsched_gettid()));
blocking = NXSEM_MBLOCKING(mholder);
diff --git a/sched/semaphore/sem_trywait.c b/sched/semaphore/sem_trywait.c
index d8ec4b3d96c..40f2e10275c 100644
--- a/sched/semaphore/sem_trywait.c
+++ b/sched/semaphore/sem_trywait.c
@@ -88,7 +88,7 @@ int nxsem_trywait_slow(FAR sem_t *sem)
break;
}
- new = nxsched_gettid();
+ new = NXSEM_MAKE_MHOLDER(nxsched_gettid());
}
else
{
diff --git a/sched/semaphore/sem_wait.c b/sched/semaphore/sem_wait.c
index e4c065bfb47..a8391b13b9f 100644
--- a/sched/semaphore/sem_wait.c
+++ b/sched/semaphore/sem_wait.c
@@ -119,9 +119,13 @@ int nxsem_wait_slow(FAR sem_t *sem)
mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKING_BIT);
- /* Avoid mutex recursion, which is not allowed. */
+ /* Avoid mutex recursion, which is not allowed. The comparison uses
+ * the lock side's encoding so that ids of either sign compare the
+ * way they were stored.
+ */
- DEBUGASSERT((mholder & (~NXSEM_MBLOCKING_BIT)) != nxsched_gettid());
+ DEBUGASSERT((mholder & (~NXSEM_MBLOCKING_BIT)) !=
+ NXSEM_MAKE_MHOLDER(nxsched_gettid()));
if (NXSEM_MACQUIRED(mholder))
{
