Pointer is deallocated shortly after declaration in multithread function

Question

I'm currently having a very weird problem in some code of mine. A variable that seems to be fine after its declaration is corrupted right after and it results in an access violation (basically the pointer are still pointing at the same place but the memory seems to be unallocated). I'm pretty sure the problem is related to the multithreading but I have no idea what it is since I am quite new to multithreading.

Here is the code :

#include "Firewall.h"
#include <Ws2tcpip.h>

Firewall::Firewall(void)
{
}


Firewall::~Firewall(void)
{
}

void Firewall::parseFile(string filePath)
{
    XMLNode xMainNode=XMLNode::openFileHelper(filePath.c_str(),"firewall");

    // Filtrage
    XMLNode nodeFiltrage = xMainNode.getChildNode("filtrage");
    XMLNode currentNode;

    for(int i=0; i < nodeFiltrage.nChildNode();i++)
    {
        currentNode = nodeFiltrage.getChildNode(i);

        string nom = currentNode.getName();

        if(nom == "permettre")
            mapFiltrage_.insert(pair<int,bool>(atoi(currentNode.getAttribute().lpszValue), true));

        else if(nom == "bloquer")
            mapFiltrage_.insert(pair<int,bool>(atoi(currentNode.getAttribute().lpszValue), false));
    }

    // Redirection

    XMLNode nodeRedirection = xMainNode.getChildNode("redirection");
    XMLNode currentSubNode;

    for(int i = 0; i < nodeRedirection.nChildNode(); i++)
    {
        currentNode = nodeRedirection.getChildNode(i);
        currentSubNode = currentNode.getChildNode("source");

        SourceDestination source((string)currentSubNode.getAttribute("adresse"), atoi(currentSubNode.getAttribute("port")));

        currentSubNode = currentNode.getChildNode("destination");
        SourceDestination destination((string)currentSubNode.getAttribute("adresse"), atoi(currentSubNode.getAttribute("port")));

        mapRedirection_.insert(pair<SourceDestination, SourceDestination>(source,destination)); 

        pair<SourceDestination, SourceDestination> test;
    }


}

void Firewall::initialiser()
{
    std::map<int, bool>::iterator iterFiltrage = mapFiltrage_.begin();
    HANDLE handleThread;

    std::string tempFiltrage = "localhost";
    thread_arg arg;

    // Parcours et lancement des connexions de filtrage
    while(iterFiltrage != mapFiltrage_.end())
    {
        arg.port = (*iterFiltrage).first;
        arg.host = tempFiltrage;
        arg.objRef = this;

        handleThread = CreateThread(NULL, 0, listenThread, &arg, 0, NULL);
        listeThread_.push_back(handleThread);

        iterFiltrage++;
    }

    // Parcours et lancement des connexions de redirection
    std::map<SourceDestination, SourceDestination>::iterator iterRedirection = mapRedirection_.begin();

    while(iterRedirection != mapRedirection_.end())
    {
        // Éviter la duplication inutile des sockets
        if(mapFiltrage_.find((*iterRedirection).first.Port()) == mapFiltrage_.end())
        {
            arg.host =  (*iterRedirection).first.Host();
            arg.port = (*iterRedirection).first.Port();
            arg.objRef = this;

            handleThread = CreateThread(NULL, 0, listenThread, &arg, 0, NULL);
            listeThread_.push_back(handleThread);
        }

        iterRedirection++;
    }
}


DWORD WINAPI Firewall::listenThread(LPVOID lpParam)
{
    thread_arg* temp = (thread_arg*)lpParam;
    Firewall* firewallRef = temp->objRef;

    return firewallRef->runThread(lpParam);
}

DWORD Firewall::runThread( LPVOID lpParam )
{
    thread_arg* infosSocket = (thread_arg*)lpParam;

    // Créer le socket et l'attacher à la source
    SOCKET sock = socket(AF_INET, SOCK_STREAM, 0);

    if(sock == INVALID_SOCKET)
    {
        cout << "Erreur de creation de socket" << endl;
        return EXIT_FAILURE;
    }

    //Recuperation de l'adresse locale
    hostent *thisHost;
    const char* test = infosSocket->host.c_str();
    thisHost=gethostbyname(test);
    char* ip;
    ip=inet_ntoa(*(struct in_addr*) *thisHost->h_addr_list);

    SOCKADDR_IN sin;
    sin.sin_addr.s_addr = inet_addr(ip);
    sin.sin_family = AF_INET;
    sin.sin_port = htons(infosSocket->port);



    if(bind(sock, (SOCKADDR*)&sin, sizeof(sin)) == SOCKET_ERROR)
    {
        cout << "Erreur de binding" << endl;
        return EXIT_FAILURE;
    }

    // Contexte du client
    SOCKADDR_IN csin;
    SOCKET csock;
    socklen_t crecsize = sizeof(csin);

    listeSocket_.push_back(sock);
    listeSocket_.push_back(csock);

    // Écouter sur le port
    if(listen(sock, 5) == SOCKET_ERROR)
    {
        cout << "Erreur de listen" << endl;
        return EXIT_FAILURE;
    }

    //csock = accept(sock, (SOCKADDR*)&csin, &crecsize);

    return EXIT_SUCCESS;
}

void Firewall::quitter()
{
    // Fermer les sockets
    vector<SOCKET>::iterator iter1 = listeSocket_.begin();

    while(iter1 != listeSocket_.end())
    {
        closesocket((*iter1));
        iter1++;
    }

    // Fermer les threads

    vector<HANDLE>::iterator iter2 = listeThread_.begin();

    while(iter2 != listeThread_.end())
    {
        TerminateThread((*iter2), EXIT_SUCCESS);
        CloseHandle((*iter2));
    }
}

Thanks a lot.

Answer 1

Your problem is in this code:

thread_arg arg;

loop(...)
{
    arg = ...;
    handleThread = CreateThread(..., &arg, ...);
}

Every thread started here receives the address of the same thread_arg instance. Then, for starting the next thread, you again modify that instance under the feet of the previously started thread. As a remedy, create a structure that holds the necessary arguments (host, port, this) and the HANDLE to the thread. Store this structure in a std::list and then pass the address of the according element to CreateThread().

There is another problem in your code, you should check returnvalues. It's much nicer to ask for help on some code if you know that all obvious errors have been detected. For that, it's easiest to use exceptions. After CreateThread(), which should possibly be beginthread() instead, add these lines:

if(handleThread == NULL)
    throw std::runtime_error("CreateThread() failed");

In a second step, create a dedicated exception class, derived from runtime_error, which holds the win32 error code (see GetLastError()) and includes the textual error description in the exception message (see FormatString()). This might sound like a lot of code for nothing, but you only write this once and you can reuse it in many places.

Lastly, your quitter() has two problems. The first is an infinite loop. Assuming you don't need the handles after closing them, try this instead:

for(; listeThread_.empty(); listeTread_.pop_back())
{
    TerminateThread(listeThread_.back(), EXIT_SUCCESS);
    CloseHandle(listeThread_.back());
}

You can write this as while-loop, too, but I personally prefer a for-loop if the number of iterations is essentially fixed. Of course, you still need to check the returnvalues of TerminateThread() and CloseHandle(). The second issue is that TerminateThread() is a bad idea, because you might be terminating the thread in the middle of something that remains half-done. Search the web for "terminatethread harmful". At this point here, all you can do is to wait for it to end using WaitForSingleObject().