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")

Answer 1

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:

1. 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
2. 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.