Fixed a kernel stack leak when deleting user-mode module threads

[prashit-vora]

Fixes #400.

User-mode module threads allocate a separate kernel stack from the module object pool. The thread-delete dispatcher freed the TX_THREAD object but left that kernel stack allocated.

This change releases the kernel stack after _txe_thread_delete() succeeds and before releasing the thread object.

Testing:

• Built the Cortex-M3 GNU module and module-manager images.
• Ran the reproduction in QEMU using mps2-an385.
• Without the fix, tx_thread_create() returned TX_NO_MEMORY after 18 create/delete cycles.
• With the fix, all 64 cycles completed.

Temporary QEMU reproduction

#define TXM_MODULE

#include "txm_module.h"

#define LEAK_TEST_ITERATION_COUNT  64
#define LEAK_TEST_STACK_SIZE       512

#define LEAK_TEST_PASS             0x600D600DUL
#define LEAK_TEST_ALLOCATE_FAILED  0xA1000000UL
#define LEAK_TEST_CREATE_FAILED    0xA2000000UL
#define LEAK_TEST_TERMINATE_FAILED 0xA3000000UL
#define LEAK_TEST_DELETE_FAILED    0xA4000000UL

TX_THREAD     *leak_test_thread_ptr;
ULONG          leak_test_stack[LEAK_TEST_STACK_SIZE / sizeof(ULONG)];
volatile ULONG leak_test_iterations;
volatile ULONG leak_test_result;

static void leak_test_thread_entry(ULONG input)
{
    (void) input;
}

void demo_module_start(ULONG id)
{
UINT iteration;
UINT status;

    (void) id;
    leak_test_iterations = 0;
    leak_test_result = 0;

    for (iteration = 0; iteration < LEAK_TEST_ITERATION_COUNT; iteration++)
    {
        status = txm_module_object_allocate((VOID **) &leak_test_thread_ptr,
                                            sizeof(TX_THREAD));
        if (status != TX_SUCCESS)
        {
            leak_test_result = LEAK_TEST_ALLOCATE_FAILED | status;
            break;
        }

        status = tx_thread_create(leak_test_thread_ptr, "leak test thread",
                                  leak_test_thread_entry, 0,
                                  leak_test_stack, sizeof(leak_test_stack),
                                  16, 16, TX_NO_TIME_SLICE, TX_DONT_START);
        if (status != TX_SUCCESS)
        {
            leak_test_result = LEAK_TEST_CREATE_FAILED | status;
            break;
        }

        status = tx_thread_terminate(leak_test_thread_ptr);
        if (status != TX_SUCCESS)
        {
            leak_test_result = LEAK_TEST_TERMINATE_FAILED | status;
            break;
        }

        status = tx_thread_delete(leak_test_thread_ptr);
        if (status != TX_SUCCESS)
        {
            leak_test_result = LEAK_TEST_DELETE_FAILED | status;
            break;
        }

        leak_test_iterations = iteration + 1;
    }

    if (leak_test_iterations == LEAK_TEST_ITERATION_COUNT)
    {
        leak_test_result = LEAK_TEST_PASS;
    }

}

QEMU results

Parent revision without the fix:
leak_test_iterations = 18
leak_test_result = 0xA2000010
                       ^ create failed with TX_NO_MEMORY

Revision with the fix:
leak_test_iterations = 64
leak_test_result = 0x600D600D
                       ^ pass

NOTE: This was not tested on physical hardware.

[prashit-vora]

Hi @fdesbiens, Could you approve the workflow runs for this PR?

[prashit-vora]

@fdesbiens could you review it ? i think its ready to merge.

[fdesbiens]

Thanks for the detailed report and the QEMU reproduction, @prashit-vora. I confirmed the leak independently: _txm_module_manager_thread_create() allocates the syscall kernel stack from the module object pool at common_modules/module_manager/src/txm_module_manager_thread_create.c:318 whenever TXM_MODULE_USER_MODE is set, and nothing released it on the tx_thread_delete() path. The memory was only reclaimed when the module was stopped, because _txm_module_manager_stop() drains the whole allocated-object list. A module that cycles threads at run time therefore leaks exactly as you describe.

Your fix is in the right place and the ordering is correct. The kernel stack is released only once _txe_thread_delete() has succeeded, so a failed delete never frees the stack of a live thread. Reading tx_thread_module_kernel_stack_start after the delete is also safe, since _tx_thread_delete() only clears tx_thread_id and does not scrub the control block. The field is unconditionally present in TX_THREAD_EXTENSION_2 for every port under ports_module, so no port is left behind. I built the module manager for Cortex-M3/GNU with -Wall -Wextra and the change compiles clean.

I pushed one small follow-up commit to your branch rather than asking you for another round trip. In the original version, a failure of the kernel stack deallocation was assigned to return_value, which then skipped the thread object release. Since the thread has already been deleted by that point, that path would have turned a successful delete into a permanently leaked TX_THREAD object, trading one leak for another. The kernel stack status is now tracked separately, the thread object is always released once the delete succeeds, and the kernel stack failure is still reported when it does not mask a thread object failure.

Merging once CI is green. Thanks again for the clean investigation and the reproduction case.

[prashit-vora]

Since the thread has already been deleted by that point, that path would have turned a successful delete into a permanently leaked TX_THREAD object, trading one leak for another. The kernel stack status is now tracked separately, the thread object is always released once the delete succeeds, and the kernel stack failure is still reported when it does not mask a thread object failure.

Thanks for explaining the follow-up change. Could you please clarify why the thread-object deallocation error is given priority if both deallocations fail? I’d like to better understand the reasoning behind that choice.

[fdesbiens]

Good question, and it was a deliberate choice rather than an arbitrary one.

The mechanical constraint is that the dispatcher has a single ALIGN_TYPE to return, which becomes the single UINT that tx_thread_delete() hands back to the module. When both deallocations fail, one status has to win.

The main reason for picking the thread object is actionability. The thread control block is the object the caller passed in, so a failure to release it is something the module can actually observe and reason about: it still holds thread_ptr, and the memory backing it is the memory it supplied. The syscall kernel stack, by contrast, is allocated implicitly by the module manager inside _txm_module_manager_thread_create(), its address is never exposed to module code, and the module has no handle on it and no remedy for it. Reporting a failure the caller can do nothing about, in preference to one it can, seemed like the wrong trade.

The second reason is backward compatibility of the error contract. Before this PR, the only status a module could ever receive from this path after a successful delete was the thread object deallocation status. Keeping that one in front means existing module error handling sees exactly what it saw before, and the kernel stack status is purely additive: it surfaces only in the new case where it is the sole failure. If we had let it take priority, we would have changed the value returned in a pre-existing failure mode, which is a subtler kind of break than the leak we were fixing.

In practice the distinction is mostly theoretical, because both calls go through _txm_module_manager_object_deallocate(), which resolves the owning module from _tx_thread_current_ptr -> tx_thread_module_instance_ptr and returns TX_NOT_AVAILABLE when the module has no object pool, or TX_PTR_ERROR when the pointer does not belong to it. If both deallocations fail they will nearly always fail for the same reason and return the same status, so the priority rarely changes the observed value at all.

Worth adding that neither failure is unbounded in any case: _txm_module_manager_stop() walks the module's allocated object list and releases everything, so anything stranded here is reclaimed when the module stops.