CreateThread ends with bad output

Question

I'm making research work about multithreading. I'm using Win32Api for CreateThread. I have char array which contains 5 drive letters. I need to MessageBox these drives one by one.

Here's my code:

DWORD WINAPI Fun(LPVOID param)
{
const char* str = (const char*)param;
MessageBox(NULL, str, "hello", MB_OK | MB_ICONQUESTION);
return 0;
}

void StartWork()
{
int n, d, b = 0;
char dd;
DWORD dr = GetLogicalDrives();

HANDLE threads[26];

for (int i = 0; i < 26; i++)
{
    n = ((dr >> i) & 1);
    if (n == 1)
    {
        dd = char(65 + i);
        std::string text(1, dd);
        d = GetDriveType((text + ":\\").c_str());
        if (d == DRIVE_REMOVABLE || d == DRIVE_FIXED || d == DRIVE_REMOTE)
        {
            threads[b] = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)Evil, (LPVOID)text.c_str(), 0, NULL);
            b += 1;
        }
    }
}
WaitForMultipleObjects(b, threads, TRUE, 1000);
}

Output isn't that I want. I got just last disk letter (I have 3 disks - C, D, E and my output is 3 times msgbox "E")

You are passing to CreateThread a pointer to the internal buffer of the string text. text then gets destroyed when it goes out of scope. Secondly std::string is c++, why is this question marked as C? — CuriouslyRecurringThoughts, Jul 01 '19 at 16:11
Further, if you "need to MessageBox these drives one by one", why the need for concurrency in the first place? — WhozCraig, Jul 01 '19 at 16:32
@veter0 As pointed out by CuriouslyRecurringThoughts, you need to use the buffer properly. You can dynamically allocate , send it to thread function and then free it from there. Coming to your question, what type of drives do you really have? Make sure it is among the 3 types you have given and not DRIVE_CDROM or other types. — SolidMercury, Jul 01 '19 at 16:48
Use `std::thread` to get rid of all these casts and free yourself from manual memory management which some commenters suggested. `CreateThread()` doesn't really fit into a modern C++ program. Using `std::thread` you can just pass the `std::string` by value and be done with it. — zett42, Jul 01 '19 at 17:09
`int n, d, b`, `char dd`, `int i`. Don't you think your variable name for the `char` is a bit too, uhm, verbose? Anyway, we don't know, what your issue is, because we cannot see `Evil`. — IInspectable, Jul 01 '19 at 18:00

CuriouslyRecurringThoughts · Answer 1 · 2019-07-02T10:31:50.713

I am assuming that Evilis Fun in your example. Now, the code I am about to write is not good modern code (I am using C++11 though, see constexpr), but I hope it is enough to show you the problem. The strings must survive until the end of the program (or at least until all the threads are done):

void StartWork()
{
  int n, d, b = 0;
  char dd;
  DWORD dr = GetLogicalDrives();

  constexpr std::size_t numMaxThreads = 26;
  HANDLE threads[numMaxThreads];

  //Now the strings survive until the end of the program, #include <array>
  std::array<std::string, numMaxThreads> texts;
  for (int i = 0; i < numMaxThreads; i++)
  {
    n = ((dr >> i) & 1);
    if (n == 1)
    {
        dd = char(65 + i);
        std::string text(1, dd);
        texts[b] = std::move(text);
        d = GetDriveType((texts[b] + ":\\").c_str());
        if (d == DRIVE_REMOVABLE || d == DRIVE_FIXED || d == DRIVE_REMOTE)
        {
            threads[b] = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE)Fun, (LPVOID)texts[b].c_str(), 0, NULL);
            b += 1;
        }
    }
  }
  WaitForMultipleObjects(b, threads, TRUE, 1000);
}

Now, is this good modern code? No, I would recommend using std::thread: this will allow you a better handling of the lifetime of the strings, something like this

#include <string>
#include <vector>
#include <thread>
#include <Windows.h>

void Fun(const std::string& str)
{
    MessageBox(NULL, str.c_str(), "hello", MB_OK | MB_ICONQUESTION);
}

void StartWork()
{
    int n, d;
    char dd;
    DWORD dr = GetLogicalDrives();

    std::vector<std::thread> threads;

    for (int i = 0; i < 26; i++)
    {
        n = ((dr >> i) & 1);
        if (n == 1)
        {
            dd = char(65 + i);
            std::string text(1, dd);
            d = GetDriveType((text + ":\\").c_str());
            if (d == DRIVE_REMOVABLE || d == DRIVE_FIXED || d == DRIVE_REMOTE)
            {
                //thanks to zett42 for this simplification
                threads.emplace_back(Fun, text);
            }
        }
    }
    //We could be using for_each also
    for (auto& thread : threads) { thread.join(); }
}

Please notice that:

With std::thread you avoid the headache of memory management, you pass ownership of the memory to the thread, then it is the thread's responsibility to cleanup
You can ditch the use of void pointers and the need to manually cast stuff around, increasing type safety.

Edit: user zett42 suggested a simpler implementation and I updated the answer.

Simpler: `threads.emplace_back(Fun, text);`. Also, the `WINAPI` could be removed from the thread function signature. — zett42, Jul 01 '19 at 18:56
@zett42 you're right, thank you for pointing it out, answer updated. — CuriouslyRecurringThoughts, Jul 01 '19 at 19:12

CreateThread ends with bad output

1 Answers1