HeapFree() Possible Causes of Crash

Question

typedef inside header file:

typedef struct tagMYSTRUCT {
    wchar_t mystr[40] = { 0 };
    DWORD threadId = NULL;
    HANDLE threadHandle = NULL;
    HWND receiverWnd = NULL;
} MYSTRUCT, *PMYSTRUCT;

Thread creation:

MYSTRUCT ats = *(PMYSTRUCT)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(MYSTRUCT));

wcscpy(ats.mystr, L"hello");

ats.threadHandle = CreateThread(NULL, 0, MyThread, &ats, 0, &(ats.threadId));

This is thread which uses HeapFree() function. But it crashes. I believe this is bad practice but I want to know why. What is the logic behind and why HeapFree crashes program?

DWORD WINAPI MyThread(LPVOID lpParam) {

    MYSTRUCT ActiveStruct = *(PMYSTRUCT)lpParam;

    if (lpParam != NULL) {
        std::cout << "1" << std::endl;    // Gets printed.
        HeapFree(GetProcessHeap(), NULL, lpParam);
        std::cout << "2" << std::endl;    // Crashes before this line.
    }

    ...

}

You're modifying `ats` in one thread and freeing it "at the same time" in another thread, how can you wonder why it crashes? But the actual problem is that you're passing the *address of the variable ats*, not the *address that you allocated memory at*. — Jonathan Potter, Mar 11 '16 at 20:30
Bingo. You need to pass the value of `ats` to the thread, not its address. — David Schwartz, Mar 11 '16 at 22:32

Peter · Accepted Answer · 2016-03-11T21:39:12.860

You've obviously come from another language that blends the concepts of pointers and references differently than C++. Your usage is wildly inappropriate in C++. You have complicated things further by using non-standard functions (HeapAlloc() which is windows specific, not C++, etc) to manage memory.

If you are going to use HeapAlloc() (which is non-standard C++, being windows specific) or any standard function that dynamically allocates memory, the result needs to be stored in a pointer.

MYSTRUCT ats = *(PMYSTRUCT)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(MYSTRUCT));

wcscpy(ats.mystr, L"hello");

ats.threadHandle = CreateThread(NULL, 0, MyThread, &ats, 0, &(ats.threadId));

What this does is convert the pointer returned by HeapAlloc() into a pointer to MYSTRUCT, dereferences that pointer which interprets that memory location as the value of a MYSTRUCT, and copies that value into ats.

At the least this is a memory leak - the memory allocated by HeapAlloc() is lost (never used again, it's address not stored anywhere), and you are passing the address of ats to the thread function.

There is therefore NO RELATIONSHIP between the memory allocated by HeapAlloc() and the address passed to the thread function.

Even worse is the thread function itself, which I've simplified here

DWORD WINAPI MyThread(LPVOID lpParam)
{
     MYSTRUCT ActiveStruct = *(PMYSTRUCT)lpParam;

     if (lpParam != NULL)
     {
          std::cout << "1" << std::endl;    // Gets printed.
          HeapFree(GetProcessHeap(), NULL, lpParam);
          std::cout << "2" << std::endl;    // Crashes before this line.
     }
}

lpParam is going to contain the address of ats from the function that was passed by the function creating the thread.

If the function creating the thread has returned (after all, threads run in parallel) then that ats will no longer exist. If that happens, lpParam will be a dangling pointer (the address of an object that no longer exists as far as your program is concerned).

ActiveStruct is now going to be a local object which contains a copy of the object at the address passed to the function. In other words, it is a local copy of ats allocated previously by the func. If that ats has ceased to exist, and the address passed is dangling, the simple act of creating ActiveStruct causes undefined behaviour.

Even worse, lpParam is the address of (what was) ats. If ats still exists (i.e. the function which created the thread hasn't returned), it was not created on the heap, so should not be released using HeapFree(). If it no longer exists, then it shouldn't be passed to HeapFree() either. Either way, HeapFree() is being asked to release memory that was not allocated using HeapAlloc(). That will be virtually guaranteed to cause a runtime error.

At minimum, you need to change the code which creates the thread to

MYSTRUCT *ats = (PMYSTRUCT)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(MYSTRUCT));    //  note changed position of *

wcscpy(ats->mystr, L"hello");    //   note usage of ats as a pointer

DWORD threadID;        // we need these since ats is being released by the thread function
HANDLE threadHandle;   // it is not a good idea for CreateThread() to use them

threadHandle = CreateThread(NULL, 0, MyThread, ats, 0, &(threadId));       // changes since ats is now a pointer

and the thread function to

DWORD WINAPI MyThread(LPVOID lpParam)
{
     MYSTRUCT *ActiveStruct = (PMYSTRUCT)lpParam;    // this is now a pointer

     if (lpParam != NULL)
     {
          std::cout << "1" << std::endl;    // Gets printed.
          HeapFree(GetProcessHeap(), NULL, lpParam);
          std::cout << "2" << std::endl;    // Crashes before this line.
     }
}

Since you are making fundamentally wrong assumptions about the C++ memory model, I would assume other things (which you haven't shown) are wrong in your code. But this should get you started.

I don't think this is enough. The heap allocated object could be destroyed before CreateThread returns. Hence `ats.threadHandle` and `ats.threadId` would not be valid. — David Heffernan, Mar 11 '16 at 21:27
Maybe not. But the OP has shown no code which uses `ats` in the function that calls `CreateThread()`, after calling `CreateThread()`. I am not going to attempt to debug code that is not shown. That is the point of my comment right at the end. — Peter, Mar 11 '16 at 21:30

David Heffernan · Answer 2 · 2016-03-11T21:17:24.933

You are getting in something of a mess over this. You are passing the address of a stack allocated structure which you do not intend to do. I think it's clear that you intend to pass the address a heap allocated structure. When you try to deallocate that structure, calling HeapFree, you encounter a runtime error because you passed to HeapFree the address of memory not allocated by HeapAlloc.

I'll show you how it is done using new and delete rather than HeapAlloc and HeapFree. There's really no need to use HeapAlloc here. Use the standard C++ memory allocator.

MYSTRUCT *pats = new MYSTRUCT(); // zero initialise
wcscpy(pats->mystr, L"hello");
DWORD threadId;
HANDLE threadHandle = CreateThread(NULL, 0, MyThread, pats, 0, &threadId);

....

DWORD WINAPI MyThread(LPVOID lpParam) 
{
    MYSTRUCT ActiveStruct = *(PMYSTRUCT)lpParam;
    delete (PMYSTRUCT)lpParam;

    // if you want the thread ID, call GetCurrentThreadId
    // or if you want a thread handle call GetCurrentThread
}

Note that I did not attempt to store the thread handle and thread ID directly into the structure. That's because the structure could in theory be destroyed before the call to CreateThread returns. I'm using local variables instead. If your thread needs to find its ID, or obtain a handle to itself, there are API calls to do that.

score 0 · Answer 3 · answered Mar 11 '16 at 20:45

0

1) you are not checking pointers before casting and dereferencing them

2) you are actually allocating MYSTRUCT on the stack and copy zeros from a heap-allocated buffer of MYSTRUCT size

3) your heap-allocated pointer leaks after copy assignment

4) you are passing pointer to stack-allocated MYSTRUCT instance to CreateThread which becomes invalid right after MYSTRUCT goes out of scope (which may happen at any time before new thread starts, while it works, or after it exits)

5) CreateThread and c++ runtime don't play well together

answered Mar 11 '16 at 20:45

trbvm

141
7

CreateThread and C++ runtime are fine together, unless you are a time traveller from ten years ago. – David Heffernan Mar 11 '16 at 20:56
Documentation clearly states that CreateThread should not be used for thread management by applications relying on c runtime. Actually it does not make sense to use CreateThread at all since there is ::std::thread available, unless you are a time traveller from ten years ago... – trbvm Mar 11 '16 at 21:06
That documentation is long out of date. `std::thread` is from C++11 and I agree it makes more sense to use it. – David Heffernan Mar 11 '16 at 21:09

pm100 · Answer 4 · 2016-03-11T21:32:34.003

0

this is the corrected version if you insist on using HeapAlloc

PMYSTRUCT ats = (PMYSTRUCT)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(MYSTRUCT));
if(ats == NULL)
    return; // exit, throw do something
wcscpy(ats->mystr, L"hello");

CreateThread(NULL, 0, MyThread, ats, 0, &(ats.threadId));

HeapAlloc returns a pointer to the allocated memory, that is placed in a pointer. The pointer is then used to manipulate the allocated struct, finally this pointer is passed to the thread. And dont assign the result of createthread to something this is destroyed by the thread.

edited Mar 11 '16 at 21:32

answered Mar 11 '16 at 21:18

pm100

48,078
23
82
145

I don't think this is enough. The heap allocated object could be destroyed before `CreateThread` returns. Hence `ats.threadHandle` and `ats.threadId` would not be valid. – David Heffernan Mar 11 '16 at 21:19
sure, but there is no code showing that he intends to free it prematurely. Looks like he allocates it and hands it to the thread.I am just correcting the code he shows – pm100 Mar 11 '16 at 21:20
He frees it in the thread, and that could happen before CreateThread returns. – David Heffernan Mar 11 '16 at 21:23
you are right, I did not notice the assignment to ats.threadHandle – pm100 Mar 11 '16 at 21:32

HeapFree() Possible Causes of Crash

4 Answers4