TerminateThread() with CloseHandle() on thread which uses only stack plain variables (without alloc) leaks memory?

0
votes

I've created simple test which creates two thread; first one (WorkerThreadFun) executes infinite loop and second one (WorkerGuardThreadFun) terminates it with small timeout.

Thread to be terminated does not seem to do expicit allocation (at least inside WorkerThreadFun) and uses only stack variables of plain C type so I hope the stack will be deallocated by TerminateThread() with CloseHandle().

By some reason this test leaks memory on my Win7.

Where is the unbalanced heap allocation?

#include <windows.h>
#include <synchapi.h>
#include <assert.h>
#include <stdio.h>

#define STK_SIZE 4097

typedef enum
{
    GRD_IDLE,
    GRD_READY,
    GRD_TASKSTARTING,
    GRD_TASKWAITING
} GuardThreadState;

typedef struct
{
    HANDLE mHworkerThread;
    HANDLE mHworkerGroupThread;

    volatile int mIsWorkerStarted;

    GuardThreadState mGuardThreadState;

    CRITICAL_SECTION mLock;
    CONDITION_VARIABLE mThreadReadyCond;
    CONDITION_VARIABLE mStartTaskCond;
    CONDITION_VARIABLE mTaskFinishedCond;
} WorkerThreadHolder;

/*
typedef VOID(WINAPI *PRtlFreeUserThreadStack)(HANDLE hProcess, HANDLE hThread);
static PRtlFreeUserThreadStack RtlFreeUserThreadStack = NULL;
*/

DWORD WINAPI WorkerThreadFun(_In_ LPVOID p);
DWORD WINAPI WorkerGuardThreadFun(_In_ LPVOID p);

void Start(WorkerThreadHolder *workerThreadHolderPtr);
void ExecuteTask(WorkerThreadHolder *workerThreadHolderPtr);

/*----------------------------------------------------------------------------*/

DWORD WINAPI WorkerThreadFun(_In_ LPVOID p)
{
    /* use stack variables only in this thread in hope the stack will be deallocated by TerminateThread() */
    WorkerThreadHolder *workerThreadHolderPtr = (WorkerThreadHolder *)p;
    volatile int i;

    workerThreadHolderPtr->mIsWorkerStarted = 1;
    /*WakeAllConditionVariable(&workerThreadHolderPtr->mThreadReadyCond);*/

    /* do nothing for infinite long time */
    for(i = 0;; ++i)
        i = i;

    /*WakeAllConditionVariable(&workerThreadHolderPtr->mTaskFinishedCond);*/

    return 0;
}

DWORD WINAPI WorkerGuardThreadFun(_In_ LPVOID p)
{
    const DWORD taskExecutionTimeoutInMillisec = 1;
    WorkerThreadHolder *workerThreadHolderPtr = (WorkerThreadHolder *)p;

    EnterCriticalSection(&workerThreadHolderPtr->mLock);

    workerThreadHolderPtr->mGuardThreadState = GRD_READY;

    WakeAllConditionVariable(&workerThreadHolderPtr->mThreadReadyCond);

    for (;;)
    {
        for (;;)
        {
            SleepConditionVariableCS(
                &workerThreadHolderPtr->mStartTaskCond,
                &workerThreadHolderPtr->mLock,
                INFINITE);

            if (workerThreadHolderPtr->mGuardThreadState == GRD_TASKSTARTING)
                break;
        }

        workerThreadHolderPtr->mGuardThreadState = GRD_TASKWAITING;

        {
            BOOL isTaskFinishedOk = FALSE;

            for (;;)
            {
                isTaskFinishedOk =
                    SleepConditionVariableCS(
                        &workerThreadHolderPtr->mTaskFinishedCond,
                        &workerThreadHolderPtr->mLock,
                        taskExecutionTimeoutInMillisec);

                if (!isTaskFinishedOk)
                    break;
            }

            if (isTaskFinishedOk)
            {
                /* never happens in this test */
            }
            else
            {
                BOOL isClosed;
                TerminateThread(workerThreadHolderPtr->mHworkerThread, 0);

                /*if (RtlFreeUserThreadStack != NULL)
                    RtlFreeUserThreadStack(GetCurrentProcess(), workerThreadHolderPtr->mHworkerThread);*/

                isClosed = CloseHandle(workerThreadHolderPtr->mHworkerThread);

                workerThreadHolderPtr->mIsWorkerStarted = 0;

                workerThreadHolderPtr->mHworkerThread =
                    CreateThread(
                        NULL,
                        STK_SIZE,
                        WorkerThreadFun,
                        (PVOID)workerThreadHolderPtr,
                        STACK_SIZE_PARAM_IS_A_RESERVATION,
                        NULL);
            }
        }

        workerThreadHolderPtr->mGuardThreadState = GRD_READY;

        WakeAllConditionVariable(&workerThreadHolderPtr->mThreadReadyCond);
    }

    return 0;
}

void Start(WorkerThreadHolder *workerThreadHolderPtr)
{
    workerThreadHolderPtr->mGuardThreadState = GRD_IDLE;

    workerThreadHolderPtr->mIsWorkerStarted = 0;

    InitializeConditionVariable(&workerThreadHolderPtr->mThreadReadyCond);
    InitializeConditionVariable(&workerThreadHolderPtr->mStartTaskCond);
    InitializeConditionVariable(&workerThreadHolderPtr->mTaskFinishedCond);
    InitializeCriticalSection(&workerThreadHolderPtr->mLock);

    workerThreadHolderPtr->mHworkerThread =
        CreateThread(
            NULL,
            STK_SIZE,
            WorkerThreadFun,
            (LPVOID)workerThreadHolderPtr,
            STACK_SIZE_PARAM_IS_A_RESERVATION,
            NULL);

    workerThreadHolderPtr->mHworkerGroupThread =
        CreateThread(
            NULL,
            0,
            WorkerGuardThreadFun,
            (LPVOID)workerThreadHolderPtr,
            0,
            NULL);
}

void ExecuteTask(WorkerThreadHolder *workerThreadHolderPtr)
{
    assert(workerThreadHolderPtr->mHworkerThread != NULL);
    assert(workerThreadHolderPtr->mHworkerGroupThread != NULL);

    EnterCriticalSection(&workerThreadHolderPtr->mLock);

    for (;;)
    {
        if (workerThreadHolderPtr->mGuardThreadState == GRD_READY /* && workerThreadHolderPtr->mIsWorkerStarted != 0 */)
            break;

        SleepConditionVariableCS(
            &workerThreadHolderPtr->mThreadReadyCond,
            &workerThreadHolderPtr->mLock,
            INFINITE);
    }

    /* just poll */
    for (;;)
    {
        if (workerThreadHolderPtr->mIsWorkerStarted != 0)
            break;
    }

    workerThreadHolderPtr->mGuardThreadState = GRD_TASKSTARTING;

    WakeAllConditionVariable(&workerThreadHolderPtr->mStartTaskCond);

    LeaveCriticalSection(&workerThreadHolderPtr->mLock);
}

/*----------------------------------------------------------------------------*/

int main(int argc, char *argv[])
{
    int i;
    WorkerThreadHolder workerThreadHolder;

    /*
    HMODULE NTLibrary = GetModuleHandleW(L"ntdll.dll");
    RtlFreeUserThreadStack = (PRtlFreeUserThreadStack)GetProcAddress(NTLibrary, "RtlFreeUserThreadStack");
    */

    Start(&workerThreadHolder);

    for(i = 0;; ++i)
    {
        ExecuteTask(&workerThreadHolder);
        printf("%d Execution started...\n", i);
        /*fflush(stdout);*/
    }

    return 0;
}

Tested with Visual Studio 2015, vc command line: /GS- /analyze- /W3 /Zc:wchar_t /ZI /Gm /Od /Fd"Debug\vc140.pdb" /Zc:inline /fp:precise /D "WIN32" /D "_DEBUG" /D "_CONSOLE" /D "_UNICODE" /D "UNICODE" /errorReport:prompt /WX- /Zc:forScope /Gd /Oy /MDd /Fa"Debug\" /nologo /Fo"Debug\" /Fp"Debug\ConsoleApplication1.pch"

linker: /OUT:"C:\Users\cherney\documents\visual studio 2015\Projects\ConsoleApplication1\Debug\ConsoleApplication1.exe" /MANIFEST /NXCOMPAT /PDB:"C:\Users\cherney\documents\visual studio 2015\Projects\ConsoleApplication1\Debug\ConsoleApplication1.pdb" /DYNAMICBASE "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "comdlg32.lib" "advapi32.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "odbc32.lib" "odbccp32.lib" /DEBUG /MACHINE:X86 /INCREMENTAL /PGD:"C:\Users\cherney\documents\visual studio 2015\Projects\ConsoleApplication1\Debug\ConsoleApplication1.pgd" /SUBSYSTEM:CONSOLE /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /ManifestFile:"Debug\ConsoleApplication1.exe.intermediate.manifest" /ERRORREPORT:PROMPT /NOLOGO /TLBID:1

2
c++cwinapi
Any DLL could allocate memory in DLL_THREAD_ATTACH with the intention of freeing it in DLL_THREAD_DETACH. But terminating a thread does not send DLL_THREAD_DETACH. (It can't, since the thread that needs to send it no longer exists.) – Raymond Chen
@Raymond Chen In this example WorkerThreadFun does not seem to load DLL nor use it – N.Cherney
That's not what Raymond implied. Try to understand when DLL_THREAD_ATTACH is fired and who receives it. Have you considered not calling TerminateThread as the rules tell you not to. – David Heffernan
@David Heffernan all DLLs which are loaded at the moment the thread fires, if I decipher your half-word right? – N.Cherney
You cannot internally coordinate with an object that has already been destroyed. If code could execute in the context of a destroyed thread, then you would have the same problems all over again. The original designers felt strongly that there should not even be a TerminateThread function because there is no way to use it properly, but people complained that it was missing, so the designers reluctantly added it, and now you have people complaining that the thing they demanded cannot be used properly. Just don't use TerminateThread. There is no valid use case for it. – Raymond Chen

2 Answers

4
votes

'TerminateThread' is 'dangerous' and in fact some de-allocations may not occur (see https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-terminatethread). Best to redesign the code to cleanly exit threads without the use of 'TerminateThread'.

0
votes

TerminateThread is not supposed to be used at all, except for debugging or some very fatal failures.

So it does not bother to deallocate memory, as these deallocations are hard to implement, and anyway memory may leak due to user's allocation.

So there would be some memory leaked even if you create suspended thread and never let it work before termination!

The biggest part of this leaked memory is the stack itself, may be not very big piece of committed memory, but whole megabyte of virtual address space (if we assume default stack).