Why is this synchronization not working?

Question

I have this code:

    #include <windows.h>
#include <tchar.h>
#include <stdio.h>
#include <stdlib.h>

#include <time.h>

#define ITERATIONS 10

typedef struct NUMERE
{
    DWORD a;
    DWORD b;
} *PNUMERE;

HANDLE ghThreadHandle[2];
HANDLE ghEvents[2];
//HANDLE hEvent;

NUMERE nr;

DWORD WINAPI GenerateNumbers(PNUMERE nr)
{
    //PNUMERE nr = ((PNUMERE)param);
    if(nr == NULL)
        return -1;

    nr->a = rand() % 100;
    nr->b = (nr->a) * 2;

    _tprintf(TEXT("Generated\n"));

    //Sleep(10);

    return 0;
}

DWORD WINAPI DisplayNumbers(PNUMERE nr)
{
    //NUMERE nr = *((PNUMERE)param);

    _tprintf(TEXT("Displayed: %d %d\n"),nr->a,nr->b);

    return 0;
}

DWORD WINAPI DoStuff(PVOID param)
{
    int index = *((int*)param);

    for(unsigned int i = 0 ; i < ITERATIONS ; i++)
    {
        if(index == 0)
        {
            WaitForSingleObject(ghEvents[1],INFINITE);
            ResetEvent(ghEvents[0]);

            if(GenerateNumbers(&nr) == -1)
                _tprintf(TEXT("GenerateNumbers error!\n"));

            SetEvent(ghEvents[0]);
            ResetEvent(ghEvents[1]);
        }
        else
        {
            WaitForSingleObject(ghEvents[0],INFINITE);
            ResetEvent(ghEvents[1]);

            DisplayNumbers(&nr);

            SetEvent(ghEvents[1]);
            ResetEvent(ghEvents[0]);
        }
    }

    return 0;
}

DWORD GenerateThreads()
{
    int temp0 = 0, temp1 = 1;
    ghThreadHandle[0] = CreateThread(NULL
        ,0
        ,(LPTHREAD_START_ROUTINE)DoStuff
        ,(LPVOID)&temp0
        ,0
        ,NULL);

    if(NULL == ghThreadHandle[0])
        return -1;

    ghThreadHandle[1] = CreateThread(NULL
        ,0
        ,(LPTHREAD_START_ROUTINE)DoStuff
        ,(LPVOID)&temp1
        ,0
        ,NULL);

    if(NULL == ghThreadHandle[1])
    {
        CloseHandle(ghThreadHandle[0]);
        return -1;
    }

    return 0;
}

int main()
{   
    srand(time(NULL));

    ghEvents[0] = CreateEvent(NULL,TRUE,TRUE,TEXT("MyEvent0"));
    ghEvents[1] = CreateEvent(NULL,TRUE,TRUE,TEXT("MyEvent1"));

    if(NULL == ghEvents[0] || NULL == ghEvents[1])
    {
        _tprintf("Error creating events\n");
        return -1;
    }

    if(GenerateThreads() == -1)
    {
        _tprintf("Error GenerateThreads\n");
        return -1;
    }

    WaitForMultipleObjects(2,ghThreadHandle,TRUE,INFINITE);

    //getchar();

    CloseHandle(ghThreadHandle[0]);
    CloseHandle(ghThreadHandle[1]);

    CloseHandle(ghEvents[0]);
    CloseHandle(ghEvents[1]);

    return 0;
}

I want the two functions (GenerateNumbers and DisplayNumbers) to be called alternatively. However, upon launch, GenerateNumbers is called twice and then it just waits. The DisplayNumbers method never gets called. Can someone explain the cause of this deadlock?

Answer 1

The GenerateThreads function passes the address of local variables to the other thread (temp0 and temp1). The function then returns immediately after starting the threads. This means that the other threads are now accessing memory that has been freed. It appears that by the time the threads read their param, the memory changed value to zero, so both threads think that they are the GenerateNumbers thread.

Adding additional debug print statements would have identified this problem sooner.

Note that you still have a race condition at the end of each block, because you signal the other thread to start before resetting the wake-up event. The other thread may wake up, do its work, then set the wake-up event, all before the first thread returns from SetEvent. The first thread then resets its event and waits on it, causing it to lose the wake-up event and consequently hang.