Refused thread delete and reset while an exit transition is in progress

[fdesbiens]

Summary

_tx_thread_shell_entry and _tx_thread_terminate both publish a thread's terminal state — TX_COMPLETED or TX_TERMINATED — and then call that thread's exit notification callback, before the thread has been detached from the ready list and before either service has finished with the pointer it holds to the control block. That terminal state is exactly the state _tx_thread_delete and _tx_thread_reset accept as authorization to invalidate or rebuild the control block, and neither service tested whether the transition producing it had finished.

This marks the transition and has the two services refuse a marked target, returning the errors they already document: TX_DELETE_ERROR and TX_NOT_DONE.

It is a robustness and safety fix, not a security fix. Reaching the window requires code running at the kernel's own privilege in the kernel's own address space, which can corrupt the scheduler directly and needs no callback ordering to do it. A memory-protected module cannot reach it: _txm_module_manager_tx_thread_entry_exit_notify_dispatch registers _txm_module_manager_thread_notify_trampoline instead of the module's own pointer, and the trampoline posts a message to the module's callback request queue, so the module's code runs later on the module's callback thread. That was checked in the source, including _txm_module_manager_callback_request, whose _tx_queue_send uses TX_NO_WAIT and touches the callback queue and the callback thread rather than the exiting thread.

What could happen before this change

A callback could delete the thread it was called for — and then lawfully recreate it over the same memory, since delete exists to permit that — while the scheduler was still linked to the old incarnation. tx_thread_create zeroes the whole control block and can auto-start the new one, so the old priority list is left heading at a block whose own priority field names a different list, with the old priority's map bit set behind nothing.

Alternatively a callback could reset a terminated thread, which moves it out of the terminal state, and then resume it. The interrupted-suspension logic in _tx_thread_system_resume refuses to void a suspension only while the state is still terminal, so with the reset allowed first the resume clears the suspending flag and restores TX_READY, the outer service then finds the flag clear and skips the removal, and tx_thread_terminate returns TX_SUCCESS for a thread that is runnable again.

Interrupt masking does not close the window, because the kernel restores the prior posture before invoking the callback deliberately. On SMP TX_RESTORE also releases the global protection, so the target can be executing on another core while its callback runs — and a reset there memsets the stack a live core is running on. On the Linux, Win32 and Win64 host simulation ports the consequence is more immediate than corruption: TX_THREAD_DELETE_PORT_COMPLETION cancels and joins the host thread backing the deleted thread, so a callback-side delete of the completing thread destroys the host thread the callback is running on.

The marker

tx_thread_suspending is the marker, rather than a new control-block field. It already means "a suspension is in progress" and is already true across the callback in the two interruptable paths, so no field is added, the public structure is unchanged, and sizeof(TX_THREAD) is unchanged — which matters, because the Module Manager's object handling depends on the sizes of the control blocks.

Widening its lifetime was checked against every reader rather than assumed. There are four: two in _tx_thread_system_suspend and two in _tx_thread_system_resume. In every window this change widens, the state is TX_COMPLETED or TX_TERMINATED, and both resume readers already refuse to void a suspension for exactly those two states, so their behaviour is unchanged; and no suspension routine is called on the target in those windows, so the suspend readers never see them. No suspension-initiating service can set the marker again inside a window either: every one of them acts on a thread that is ready or suspended.

That is also why _tx_thread_system_resume needs no change. Its existing terminal-state test is sufficient once nothing can turn the terminal state into TX_SUSPENDED from inside the window.

Where the clears sit, and why

Three sites needed changing beyond the two refusals, and the safe clear point is different in each:

• The non-ready branch of _tx_thread_terminate cleared the marker before the terminated extension and the callback, which is what left them free to act on a control block the service still had mutex-release processing to do against. The clear moves to the common tail, after the last dereference of the target, and becomes the service's single clear site. In the interruptable ready branch the flag is already false there, because _tx_thread_system_suspend cleared it when it detached the thread, so the tail store writes a value the flag already holds — cheaper than testing for it, and it keeps one clear site.
• Under TX_NOT_INTERRUPTABLE neither path set the marker at all, because that configuration does not use the interruptable suspension path that sets it. Both now set it before the callback. Interrupts being disabled there does not substitute for the marker: the callback is reached by a direct call.
• In the TX_NOT_INTERRUPTABLE completion path the marker is cleared before _tx_thread_system_ni_suspend, not after it. That call returns to the scheduler for a thread that is the current thread, which a completing thread is, and does not come back; clearing afterwards would leave a normally completed thread marked for ever and therefore permanently undeletable. Nothing is lost by clearing early there, because everything from that point to the detachment runs with interrupts disabled and calls no application code.

tx_thread_system_suspend.c and tx_thread_system_resume.c are deliberately untouched. They are the only files involved that differ between the two kernels, and placing the clear in _tx_thread_system_ni_suspend would have put an unconditional store into a function ten other services call, to serve two callers.

Both kernels

All four files are byte-for-byte identical between common and common_smp before this change and after it, so common_smp was written by copying rather than by repeating the edits. The change is the same change twice, by construction.

Tests

The in-tree regression test goes to both trees and is byte-for-byte identical between them. Seven scenarios: terminating a ready non-current target with two peers ready at the same priority, with the callback attempting the delete and recreating the block if it succeeded; the same with the callback attempting the reset and then the resume; terminating a target suspended on a semaphore while owning a mutex, which is the non-ready branch; natural completion alone at its priority, including the safe post-completion reset, terminate, delete and recreate at another priority; self termination; a benign callback, whose notification count and ordering are unchanged; and the state and boundary cases, where the new refusal must not fire.

It measures rather than describes. The callback records the published state, the marker, and the status of every lifecycle service it can reach, calling the core service as well as the wrapper wherever a refusal is expected — which is what makes the TX_DISABLE_ERROR_CHECKING requirement testable from a build that has error checking on. A snapshot taken under interrupt lockout, which is the global SMP protection on an SMP port, checks that every ready list agrees with the control blocks it heads, that the priority map agrees with the lists, and that each execute pointer is a member of the list its own priority field names. Every walk is bounded, so a corrupted ring costs an assertion and not a hang.

The consequences are demonstrated from the terminator's context rather than the completing thread's, and the scenarios are ordered accordingly, because a callback-side delete of the completing thread wedges the simulation ports rather than reaching an assertion. Compiled against the unfixed sources the test fails 9 of the 24 expectations it reaches in the uniprocessor tree and 10 of 24 in the SMP tree, and the failures are the finding: the callback-side delete succeeds, the recreate succeeds, the consistency snapshot disagrees, the target is neither terminal nor detached when the service returns, and a peer has left the ready ring the recreated block hijacked.

The SMP tree gets a second test for the case that needs concurrency. The victim is excluded to core 1 and spins there without relinquishing while the controller, excluded to core 0, terminates it. The callback-side reset and delete must both be refused, and a sentinel written into the unused low end of the victim's stack must survive — a reset would have memset the whole stack before rebuilding the frame. Every wait is bounded, and if the remote precondition cannot be established the test says so and drops only the assertions that depend on it rather than reporting a pass it did not earn; measured over twenty consecutive runs it established the precondition every time.

TX_NOT_INTERRUPTABLE and TX_DISABLE_ERROR_CHECKING are not among the five build configurations either tree compiles, and each tree builds the whole library once per configuration, so neither can be reached from inside the suites. The lines this change adds under TX_NOT_INTERRUPTABLE are therefore in no configuration the trees build, and they are where the permanent-undeletability failure mode lives, so they get their own harness rather than a compile check: the four sources plus the three error wrappers are compiled directly into a test executable, once per combination, with recorders standing behind the scheduler services they call. That is what makes the marker's value at the moment of detachment directly observable. It runs 66 expectations under TX_NOT_INTERRUPTABLE, 66 under that with error checking disabled, 45 under that with notification disabled, and 63 under error checking disabled alone; against the unfixed sources those fail 25, 22, 6 and 20 respectively. The harness lives in the uniprocessor tree only, because the four sources are identical between the kernels and the shim replaces the very primitive the SMP port differs in. It is deliberately left out of the coverage instrumentation, since the same source under different feature macros has a different line set and merging those would confuse the union rather than add to it.

Its add_subdirectory line sits below add_subdirectory(samples) rather than beside regression, and the file records why: several other branches in flight add a line immediately after regression, and a second one there turns clean merges into one-line conflicts.

Results

The figures below were measured on 3 September against dev at 8c681c18. This branch is one commit ahead of dev and 33 behind, and merges cleanly; CI on this pull request re-measures against current dev, so read the checks rather than these numbers if the two disagree.

• Both suites pass in all five configurations with GCC 14: 103 of 103 in the uniprocessor tree, up from 98, and 116 of 116 in the SMP tree, up from 114.
• Merged line coverage is 100% in the uniprocessor tree (4225 of 4225) and 5172 of 5183 in the SMP tree, whose eleven uncovered lines are the same eleven that were uncovered before this change and are in tx_byte_pool_search.c and tx_thread_smp_utilities.c. All four changed files are at 100% line coverage in both trees, and SMP branch coverage rises from 2819 of 3548 to 2831 of 3556.
• Cross-compiled with arm-none-eabi-gcc at -Wall -Wextra for Cortex-M4 against common and for Cortex-A7 SMP against common_smp, all four files produce no diagnostics at all and an identical warning set to before the change, under -std=gnu99 and -std=c99 alike. -Wconversion is clean as well.

Documentation

The matching documentation is already merged, as eclipse-threadx/rtos-docs-asciidoc#57 on 9 September, covering the ThreadX User Guide's Chapter 4. A caller that has always seen TX_SUCCESS for a completed thread can now see a retryable error, so it is a documented-behaviour change. Each of tx_thread_delete and tx_thread_reset gains a note saying the refusal exists, that it is transient, and what it means in practice, and each Return Values entry names the new reason beside the state test; tx_thread_entry_exit_notify gains a note saying what the callback is running inside.

The existing sentence forbidding a callback from calling a ThreadX API with a suspension option is deliberately left as it stands. Widening it to forbid lifecycle calls outright would document a restriction the kernel does not enforce — create and resume remain available from a callback and are harmless there — and would read as deprecating a legitimate pattern. What a callback author needs is the return value to expect.

The ThreadX SMP manual needs no matching change: its Chapter 4 documents only the SMP-specific services, and these three are documented once, in the ThreadX User Guide, which serves both kernels.

MISRA

No deviation is required: explicit comparisons to TX_TRUE, existing ThreadX types, single-entry and single-exit control flow, no goto, and two added constant-time tests that change no real-time complexity.

[blanchardj-dev]

Isn't this a very heavy handed solution to a problem that is honestly unclear? On top of the description being very hard to understand it is unclear which problem this is trying to fix.

Moreover thread state change callbacks in RTOS are always very dangerous area of code and most operations that changes the kernel state within those callbacks can be dangerous. Not just thread delete and reset.

Finally the proposed change affects extremely critical and intricate portions of the kernel and is, in my opinion, the wrong solution for the problem.

I am quite nervous to see this kind of AI generated change being merged so quickly.

[fdesbiens]

Hi @blanchardj-dev.

Thank you for your feedback. I appreciate that you expressed your concerns candidly. I want to emphasise that I reviewed this extensively on my own before creating and merging the PR. I certainly do not trust AI blindly.

Going forward, if you and others are willing to commit time to perform code reviews, I will gladly reduce the cadence. My past efforts have been more or less successful in that domain, and I certainly hope we can change that.

I agree this is a meaningful change and that we should be careful. So, if you wish, I am happy to revert this to allow a proper review.

The problem, in a nutshell: An exit callback runs after the state is published as TX_COMPLETED/TX_TERMINATED but before the thread is unlinked from the ready list — and that state is exactly what tx_thread_delete accepts as authorisation to release the block:

tx_thread_delete(t);        /* succeeded, before this change */
tx_thread_create(t, ...);   /* legal — delete exists to permit exactly this */

tx_thread_create zeroes the block and can auto-start it at a new priority; _tx_thread_terminate then resumes with its stale pointer and unlinks that block from a list it is no longer on. On the Linux/Win32 simulation ports, it's more immediate: TX_THREAD_DELETE_PORT_COMPLETION joins the host thread the callback is running on. Reset is the same window from the other side.

That's the whole bug. If the description didn't convey it in five lines, that's the description's fault. I am happy to amend it.

I disagree with your "Heavy-handed" assessment. The kernel change touches 4 files: two flag tests, two stores under TX_NOT_INTERRUPTABLE, and one existing store in tx_thread_terminate moved later. No new field, sizeof(TX_THREAD) is unchanged, tx_thread_system_suspend.c/resume.c are untouched. The ~4,300 lines are tests. The moved clear in terminate is the only structural piece and the part I'd most want reviewed.

About callbacks being dangerous generally — agreed, and this doesn't make them safe. Delete and reset were scoped in because they're the two services that invalidate the control block the calling service still holds a pointer to. If your position is that this belongs in the User Guide and nowhere else, that's legitimate — better said now than after it ships.

You say it's the wrong solution — what's the right one? Docs only, a check in the txe wrappers, something at the callback site? I'll take any of those seriously, including reverting first and starting from the design question. Just let me know!

[blanchardj-dev]

I can summarize my high level concerns as follows:

1. Validity of the problem

I do not argue that the sequences of operations mentioned would have the stated result. However, there are a lot of other invalid operations that can be done with similar or even worse consequences. The pull request does not discuss as to why this particular problem should be fixed over many other similar programming errors.

2. Is the proposed solution the right one

The problem, as described, relates to an invalid call, basically t that specific location the API mentioned should just not be called. The fix, however, modifies the kernel thread state machine instead of preventing the invalid calls directly. Another solution that could be considered is just having a flag to know if we are within a state change notification callback and just return that those calls are invalid at that point.

3. Added comments

Finally, the comments added within the code are overly verbose and to be frank reads like AI babble. Out of context of this PR they are detrimental to the legibility of the code and they should either be removed or rewritten.

[fdesbiens]

Thank you for the quick reply, @blanchardj-dev.

This particular problem surfaced as I was proactively working on potential issues around ThreadX modules. It was flagged as a security vulnerability candidate, but I concluded this was a plain code fix with no advisory needed.

I will look into the alternative solutions you propose. If we conclude that this particular fix stays, I will clean up the comments.

This interaction is an example of what I wish would happen more often on this project. I need the core team to commit time to triage and reviews.

[fdesbiens]

Hi @blanchardj-dev.

Thank you — this is useful, and I concede your third point without reservation. The comments are several times the length of every other inline comment in those files, and they explain history rather than behaviour. That history belongs in the commit message, where it already is. I will cut them back to house style regardless of how the rest of this discussion ends.

On your second point: the flag you propose is a reasonable instinct, and on a uniprocessor kernel it would be sufficient. _tx_thread_preempt_disable covers the whole window there, so the callback really is the only way in. It is not sufficient on SMP. In the interruptable path of _tx_thread_terminate, TX_RESTORE is _tx_thread_smp_unprotect(), so the global protection is released once TX_TERMINATED has been published, and it stays released through the callback, the suspension, and the mutex-release processing that still dereferences thread_ptr. Disabling preemption does not hold another core. A flag that describes the calling context is false on that path and closes nothing. The same is true when TX_DISABLE_NOTIFY_CALLBACKS is defined.

That also answers your first point better than the pull request does. A thread on another core calling tx_thread_delete on a thread it has observed as TX_TERMINATED is not a programming error. It is the documented contract, and the kernel published that state itself. The real issue is that the kernel advertises a terminal state it has not finished leaving, and that is what separates this from the other invalid operations you have in mind. I should have led with it.

I should also be clearer about the module story than I was above. This surfaced while I was looking at potential module vulnerabilities, but that is provenance and nothing more. A memory-protected module cannot reach this window: _txm_module_manager_tx_thread_entry_exit_notify_dispatch registers _txm_module_manager_thread_notify_trampoline rather than the module's own pointer, and the trampoline posts a message to the module's callback request queue, so the module's code runs later on its own callback thread. This is a plain kernel fix, no advisory is warranted, and you should not read it as a module issue.

So: I will rewrite the description around the SMP race and trim the comments. If you would still prefer to review this from a reverted tree, say so and I will revert first.

[blanchardj-dev]

That also answers your first point better than the pull request does. A thread on another core calling tx_thread_delete on a thread it has observed as TX_TERMINATED is not a programming error. It is the documented contract, and the kernel published that state itself. The real issue is that the kernel advertises a terminal state it has not finished leaving, and that is what separates this from the other invalid operations you have in mind. I should have led with it.

On this point do you mean there is also a race condition even without the participation of the thread exit notification callback? And does this race condition only affects the SMP kernel or both?

On the subject of the fix itself. tx_thread_suspending is also used when suspending a thread when waiting on a synchronization object. For example in _tx_semaphore_get(). What would be the impact on the top level API of this change in that case?

[fdesbiens]

Hi @blanchardj-dev.

Yes to the first question, and it impacts SMP only. I should have separated the two cases earlier.

The sequence needs no callback. Core 0 calls tx_thread_terminate(t). The service publishes TX_TERMINATED and then issues TX_RESTORE, which on an SMP port is _tx_thread_smp_unprotect(). Core 1 is then free to enter the kernel, observes a terminated thread, and calls tx_thread_delete(t), which is legal on a terminated thread and succeeds. Core 0 carries on through _tx_thread_system_suspend and the mutex-release call at tx_thread_terminate.c:277, both of which still dereference thread_ptr. This holds with TX_DISABLE_NOTIFY_CALLBACKS defined, so no notification callback is involved at any point.

On the uniprocessor kernel, it does not happen. _tx_thread_preempt_disable is raised on entry to _tx_thread_terminate and not lowered until the tail, and the completion path raises it before the TX_RESTORE that precedes the callback. No other thread runs in that window, and _txe_thread_delete and _txe_thread_reset both reject a non-thread caller. So on the uniprocessor kernel the callback really is the only way in, and there the change hardens against misuse rather than closing a race. The race itself is SMP only, and you were right to press on the distinction.

On your second question: there is no impact on the top-level API for a thread suspended on a synchronisation object. Both new tests sit behind the existing terminal-state check and are reached only when the state is already TX_COMPLETED or TX_TERMINATED. A thread suspended in _tx_semaphore_get is in TX_SEMAPHORE_SUSP, so tx_thread_delete has already returned TX_DELETE_ERROR, and tx_thread_reset TX_NOT_DONE, from the check that was there before. The flag is never consulted on that path.

The change also adds no writer of the flag on any suspension path, so _tx_semaphore_get and its peers are untouched. The pre-existing readers are four in the uniprocessor tree: two in _tx_thread_system_suspend and two in _tx_thread_system_resume. In every window the change widens, the state is terminal, and both resume readers already decline to cancel a suspension for exactly those two states. The only difference visible at the API is for a thread already in a terminal state, and only until the transition completes.

My offer to revert for review stands. Just let me know!

[blanchardj-dev]

About the offer to revert this is something we maybe should discuss during the committers meeting along with the best practices with AI generated code and PRs.

[fdesbiens]

Sure. That was my intent as well.

That said, without a firm commitment from contributors and committers to review PRs and candidate vulnerabilities in a timely fashion, any discussion we have will be moot.

The Eclipse project handbook provides generative AI guidelines. https://www.eclipse.org/projects/handbook/#genai

I followed them strictly in everything I did.