sched/task: fix nxtask_exit() freeing the wrong TCB on non-SMP (#19308)

[94xhn]

Note: Please adhere to Contributing Guidelines.

Summary

Fixes #19308.

On a non-SMP build, nxtask_exit() identifies the exiting task with dtcb = this_task() - the head of the ready-to-run list. If a higher-priority task becomes ready and is placed at the ready-to-run head while the context switch away from the exiting task is still deferred (so the head no longer equals the task that is actually running), nxtask_exit() calls nxsched_remove_self(dtcb) and nxsched_release_tcb(dtcb) on that higher-priority task instead of on the task that is actually exiting. This frees the TCB/stack of a live task - a use-after-free that hard-faults or locks up the board.

The SMP path of the same function already avoids this by using the actually-running task (current_task(this_cpu()) == g_assignedtasks[cpu]); only the non-SMP path used the ready-to-run head. Note that this_task() and current_task(cpu) are literally the same expression on non-SMP (both resolve to the ready-to-run list head), so the divergence isn't about SMP vs non-SMP semantics per se - it's that the non-SMP path never had a correct "actually running" accessor to fall back to.

Fix: use g_running_tasks[this_cpu()] (this_cpu() is (0) on non-SMP), which is updated only at real context switches and so stays correct exactly when the ready-to-run head has diverged due to a deferred higher-priority switch.

Note this is deliberately not the same as the existing running_task() macro (running_task() == up_interrupt_context() ? g_running_tasks[this_cpu()] : this_task()), since that macro only prefers g_running_tasks[] during interrupt-level context switches - the race this issue describes happens in a perfectly ordinary (non-interrupt) call chain (exit() -> _exit() -> nxtask_exithook() -> up_exit() -> nxtask_exit()), so running_task() would still resolve to this_task() here and would not fix anything.

Removing the correct task is safe: the exiting task is in TSTATE_TASK_READYTORUN and present in the ready-to-run list, so nxsched_remove_readytorun() takes its dq_rem path, and the CPU then switches to the pending higher-priority task. nxtask_exit() is the only exit-path site that derives the exiting task from this_task() (nxtask_terminate() uses an explicit pid), so this one line is the complete fix.

Impact

Affects the non-SMP task-exit path only (sched/task/task_exit.c). No API/ABI change, no build-time configuration change. Fixes a use-after-free/hard-fault that can occur whenever a higher-priority task is made ready between the exiting task calling exit() and the pending context switch actually being taken (see reply below for the concrete mechanism).

Testing

I don't have the affected hardware (RP2350/non-SMP) set up to reproduce the exact race, but the issue report itself includes extensive on-target validation: the reporter captured the divergence with an SRAM breadcrumb (this_task() resolving to a live high-priority Wi-Fi thread while g_running_tasks[cpu] correctly pointed at the exiting app), reproduced the resulting hard fault deterministically with CONFIG_MM_FILL_ALLOCATIONS, and validated this exact one-line fix survives 20+ ELF load/run/exit cycles where the pre-fix build hard-faults on the first run.

I independently re-verified the code citations against the current upstream master (confirmed the non-SMP path still uses this_task(), confirmed g_running_tasks[]/this_cpu() are unconditionally available per include/nuttx/sched.h, and confirmed running_task() would not have fixed this given the non-interrupt call path), and verified the core divergence logic (this_task() resolving to the wrong task vs. g_running_tasks[this_cpu()] resolving to the actually-exiting task) with a standalone C reproduction of the two accessors under the described race condition. I also traced the actual mechanism by which the two can diverge in arch/*/src/common/*_doirq.c (see reply to @hujun260 below) to answer exactly when this happens.

Disclosure

Generative AI (Claude) was used to help investigate this issue and implement this fix. All changes were reviewed by me before submission.

[hujun260]

I noticed you replaced this_task() with g_running_tasks[this_cpu()]. Could you explain under what circumstances these two might not be equal in nxtask_exit?

[github-actions]

MemBrowse Memory Report

arduino-mega2560

• flash: .text +2 B (+0.0%, 67,202 B / 262,144 B, total: 26% used)

esp32-devkitc

• ROM: .flash.text +4 B (+0.0%, 122,656 B / 4,194,272 B, total: 3% used)
• irom0_0_seg: .flash.text +4 B (+0.0%, 87,024 B / 3,342,304 B, total: 3% used)

hifive1-revb

• flash: .text +4 B (+0.0%, 83,256 B / 4,194,304 B, total: 2% used)

mirtoo

• kseg0_progmem: .text -4 B (-0.0%, 67,084 B / 131,072 B, total: 51% used)

qemu-intel64

• Code: .text +4 B (+0.0%, 8,655,353 B)

rx65n-rsk2mb

• ROM: .text +16 B (+0.0%, 85,624 B / 2,097,152 B, total: 4% used)

stm32-nucleo-f103rb

• flash: .text +8 B (+0.0%, 33,744 B / 131,072 B, total: 26% used)
  No memory changes detected for:
• qemu-armv8a
• s698pm-dkit

[linguini1]

Please follow the contributing guide and ensure your description follows the PR template.

[94xhn]

@hujun260 Good question — I re-verified this against master rather than just repeating the issue text. The divergence happens in nxsched_add_readytorun() (sched/sched/sched_addreadytorun.c, non-SMP path):

else if (nxsched_add_prioritized(btcb, list_readytorun()))
  {
    /* The new btcb was added at the head of the ready-to-run list.  It
     * is now the new active task!
     */

    btcb->task_state = TSTATE_TASK_RUNNING;
    btcb->flink->task_state = TSTATE_TASK_READYTORUN;
    up_update_task(btcb);
    ret = true;
  }

When a higher-priority task btcb becomes ready and preemption is not locked, this immediately (synchronously, in whatever context called nxsched_add_readytorun()) makes btcb the new ready-to-run head — so this_task() returns btcb from this point on. up_update_task(btcb) only signals that a context switch is needed; the actual CPU-level register switch (and the point where g_running_tasks[cpu] gets updated to btcb, e.g. in arm_doirq.c/riscv_doirq.c) happens later, on whatever deferred mechanism the architecture uses (e.g. PendSV on ARMv7-M). Until that later switch actually executes, g_running_tasks[cpu] still correctly points at the old task, which is still the one whose registers are loaded and which keeps executing.

So the two diverge whenever a task's own currently-executing code creates or wakes a higher-priority task partway through a call chain, without immediately yielding the CPU. nxtask_exit() is reached through exactly such a chain: exit() -> _exit() -> nxtask_exithook() -> up_exit() -> nxtask_exit(). For a CONFIG_BINFMT_LOADABLE app this is a comparatively long, preemptible window (binfmt_exit() -> unload_module() -> elf_unloadbinary() -> libelf_uninit() -> up_textheap_free(), etc.) - if a higher-priority task is promoted to the ready-to-run head anywhere in that window, this_task() in the subsequent nxtask_exit() call resolves to that new task while the exiting task (still g_running_tasks[cpu]) is the one that's actually finishing up.

This matches the original issue reporter's on-target finding (#19308): they captured this exact divergence with an SRAM breadcrumb during the teardown of a loadable ELF app - this_task() resolved to a live, higher-priority Wi-Fi thread while g_running_tasks[cpu] correctly still pointed at the exiting app - and reproduced the resulting hard fault deterministically with CONFIG_MM_FILL_ALLOCATIONS.

[hujun260]

@hujun260 Good question — I re-verified this against master rather than just repeating the issue text. The divergence happens in nxsched_add_readytorun() (sched/sched/sched_addreadytorun.c, non-SMP path):

else if (nxsched_add_prioritized(btcb, list_readytorun()))
  {
    /* The new btcb was added at the head of the ready-to-run list.  It
     * is now the new active task!
     */

    btcb->task_state = TSTATE_TASK_RUNNING;
    btcb->flink->task_state = TSTATE_TASK_READYTORUN;
    up_update_task(btcb);
    ret = true;
  }

When a higher-priority task btcb becomes ready and preemption is not locked, this immediately (synchronously, in whatever context called nxsched_add_readytorun()) makes btcb the new ready-to-run head — so this_task() returns btcb from this point on. up_update_task(btcb) only signals that a context switch is needed; the actual CPU-level register switch (and the point where g_running_tasks[cpu] gets updated to btcb, e.g. in arm_doirq.c/riscv_doirq.c) happens later, on whatever deferred mechanism the architecture uses (e.g. PendSV on ARMv7-M). Until that later switch actually executes, g_running_tasks[cpu] still correctly points at the old task, which is still the one whose registers are loaded and which keeps executing.

So the two diverge whenever a task's own currently-executing code creates or wakes a higher-priority task partway through a call chain, without immediately yielding the CPU. nxtask_exit() is reached through exactly such a chain: exit() -> _exit() -> nxtask_exithook() -> up_exit() -> nxtask_exit(). For a CONFIG_BINFMT_LOADABLE app this is a comparatively long, preemptible window (binfmt_exit() -> unload_module() -> elf_unloadbinary() -> libelf_uninit() -> up_textheap_free(), etc.) - if a higher-priority task is promoted to the ready-to-run head anywhere in that window, this_task() in the subsequent nxtask_exit() call resolves to that new task while the exiting task (still g_running_tasks[cpu]) is the one that's actually finishing up.

This matches the original issue reporter's on-target finding (#19308): they captured this exact divergence with an SRAM breadcrumb during the teardown of a loadable ELF app - this_task() resolved to a live, higher-priority Wi-Fi thread while g_running_tasks[cpu] correctly still pointed at the exiting app - and reproduced the resulting hard fault deterministically with CONFIG_MM_FILL_ALLOCATIONS.

@94xhn
The case you just described, where this_task and g_running_tasks can be unequal, only occurs in an interrupt context or during an active context switch (though in the latter case, interrupts are masked and cannot be interrupted until the switch completes). However:
1 nxtask_exit runs in a thread context. A thread can never preempt an interrupt; once the interrupt exits and thread execution resumes, this_task and g_running_tasks will be equal again.
2 nxtask_exit runs with interrupts masked.