[C++/win32] Winsock2-Multithreading error

[falcon4ever]

Op dit moment ben ik bezig een eenvoudige applicatie te ontwikkelen welke gebruik maakt van winsock2. De bedoeling is dat de server verschillende acties uitvoert (zoals text of bin bestanden opsturen) afhankelijk van de client invoer. De server moet meerdere clients tegelijk kunnen behandelen.

Ik heb hiervoor 2 projecten in ms visual studio gemaakt.
mytcp.exe -> het server programmatje
mytcp2.exe -> het client programmatje

Op zich werken ze samen ok (bericht van de server komt goed aan bij de client),
alleen krijg ik een foutmelding waarvan ik de reden niet begrijp:

---

Server (mytcp.exe):

C++:

//#include "main.h"
#include <winsock2.h>
#include <stdio.h>
#include <conio.h>
#include <iostream>

using namespace std;
#pragma comment(lib, "ws2_32.lib")

SOCKET server;
unsigned __stdcall ServerThread(void* pArguments);
unsigned __stdcall ClientThread(LPVOID iValue);

int main()
{
    DWORD main;

    printf("Press ESCAPE to terminate program\r\n");
    CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)ServerThread,NULL,NULL,&main);   // Seperate thread
    while(_getch()!=27);
    
    closesocket(server);
    WSACleanup();

    return 0;
}

unsigned __stdcall ServerThread(void* pArguments)
{
    printf("Starting up TCP server\r\n");
    WSADATA wsaData; //WSADATA is a struct that is filled up by the call to WSAStartup
    sockaddr_in local; //The sockaddr_in specifies the address of the socket for TCP/IP sockets. 
    int wsaret=WSAStartup(0x101,&wsaData); //WSAStartup initializes the program for calling WinSock.
    if(wsaret!=0) return 0;//WSAStartup returns zero on success. If it fails we exit.
    
    //Now we populate the sockaddr_in structure
    local.sin_family=AF_INET; //Address family
    local.sin_addr.s_addr=INADDR_ANY; //Wild card IP address
    local.sin_port=htons((u_short)20029); //port to use

    //the socket function creates our SOCKET
    server=socket(AF_INET,SOCK_STREAM,0);

    //If the socket() function fails we exit
    if(server==INVALID_SOCKET) return 0;

    //bind links the socket we just created with the sockaddr_in 
    //structure. Basically it connects the socket with 
    //the local address and a specified port.
    //If it returns non-zero quit, as this indicates error
    if(bind(server,(sockaddr*)&local,sizeof(local))!=0) return 0;

    //listen instructs the socket to listen for incoming 
    //connections from clients. The second arg is the backlog
    if(listen(server,10)!=0) return 0;

    //we will need variables to hold the client socket. thus we declare them here.
    SOCKET client;
    sockaddr_in from;
    int fromlen=sizeof(from);

    while(true)//we are looping endlessly
    {
        DWORD dwClientThread;
        client=accept(server, (struct sockaddr*)&from, &fromlen);
        char lszThreadParam[4];
        sprintf(lszThreadParam, "%i", client);
        CreateThread(NULL,NULL,(LPTHREAD_START_ROUTINE)ClientThread,&lszThreadParam,NULL,&dwClientThread);  // Seperate thread
    }
    return 0;
}

unsigned __stdcall ClientThread(LPVOID iValue)
{
        char client_id[4];
        strcpy(client_id,(char *)iValue);               
        unsigned int client = atoi (client_id);
        cout << "connecting: " << client <<"\r\n";

            // do smth here     
            char temp[512]; 
            sprintf(temp,"Hello, your client id is: %d\r\n", client);    
            send(client,temp,(int)strlen(temp),0);
            // Just sending a sample string
        
        cout << "disconnecting: " << client <<"\r\n";
        closesocket(client);
    return 0;
}

---

Client (mytcp2.exe):

C++:

// #include "main.h"
#include <winsock2.h>
#include <stdio.h>
#include <conio.h>
#include <iostream>

using namespace std;
#pragma comment(lib, "ws2_32.lib")

int main()
{
    WSAData wsaData;
    int error = WSAStartup(0x101, &wsaData);

    if (error == SOCKET_ERROR)
    {
        printf("WinSock Failed!\n");
        return 0;
    }
    printf("WinSock Started\n");

    SOCKET mySocket;
    mySocket = socket(AF_INET, SOCK_STREAM, 0);
    if (mySocket == SOCKET_ERROR)
    {
        printf("Socket Failed!\n");
        return 0;
    }
    printf("Socket Started\n");

    char server_name[40] = "localhost";
    struct hostent *host_entry;
    host_entry = gethostbyname(server_name);
    if (host_entry == NULL)
    {
        printf("Couldn't Find Host!\n");
        return 0;
    }

    struct sockaddr_in server;
    server.sin_family = AF_INET;
    server.sin_port = htons((unsigned short)20029);
    server.sin_addr.s_addr = *(unsigned long*) host_entry->h_addr;
    error = connect(mySocket, (sockaddr*)&server, sizeof(server));
    if (error == SOCKET_ERROR)
    {
        printf("Connection Failed!\n");
        return 0;
    }
    printf("Server Connected To...\n");

    char buff[512];
    int y, msgSize=0;

        // Read buffer
        while(y=recv(mySocket,buff,512,0)) msgSize = y;
        // Print buffer
        if(msgSize) for (int i=0; i<msgSize; i++) printf("%c", buff[i]);
        if(msgSize) printf("Total characters received: %d\n",msgSize);
        // Reset (only for while loop)
        // if(msgSize) for (int i=0; i<512; i++) buff[i] = 0;
        // msgSize = 0;

    closesocket(mySocket);
    printf("Socket Closed\n");

    WSACleanup();
    printf("WinSock Closed\n");
    printf("Press any key to continue...\n");
    return 0;
}

Iemand een idee wat het probleem kan zijn?

[ Voor 6% gewijzigd door falcon4ever op 04-12-2005 15:32 . Reden: ff c++ tags bij code gezet. ]

[ShadowLord]

C++:

        client=accept(server, (struct sockaddr*)&from, &fromlen);
        char lszThreadParam[4];
        sprintf(lszThreadParam, "%i", client);

Daar zit je fout.

accept geeft een socket nummer terug, wat een signed integer is. Als je deze print kan die maximaal 11 karakters lang worden (het minteken en 10 cijfers). Jij allocate een scring van 4 karakters, waar dus maar 3 cyfers in passes (laatste karakter is voro terminating null). De sprintf() die je aanropet kan niet zijn of de sting de je hebt gealloceerd groot genoeg is en schrijft fijn door. Hierdoor raakt je stack corrupt en crashed je programma.

Wat ik zou doen is iets van:

C++:

SOCKET *client;

[...]
        client = new SOCKET;
        client = accept(...);
        [...]
        CreateThread(NULL, NULL, ClientThread, &client, NULL, &dwClientThread);    // Seperate thread

En dan in je thread:

C++:

SOCKET client;
client = *iValue;
delete iValue;
[...]

Overigens zitten er meer schoonheidsfoutjes e.d. in je code (je moet the thread functies declareren a;s zodanig en NIET bij het aanroepen van CreateThread als zodanig casten). Ook is de zin van het doorgeven van een numerieke waarde als string ook onbegrijpelijk voor me. Daarom raad ik je aan om eerst wat meer te leren van de losse onderwerpen (multithreaden in particular) voordat je hierana begint. Multithreaden is namelijk ENORM lastig als je niet heel gestructureerd code en niet weet waar je mee bezig bent.

[falcon4ever]

Shadowlord,
bedankt voor je snelle en duidelijke antwoord.

Na het instellen van lszThreadParam[4]; op 11 werkt het zonder errors...

---

Maar goed, even om het te verhelderen. Deze code is verre van netjes. Dit komt doordat ik gebruik heb gemaakt van de volgende tutorial:
http://www.codeproject.com/internet/winsockintro03.asp
Helaas is deze tutorial in MFC gemaakt, terwijl ik liever gewoon win32 heb. Vandaar dat ik een en ander heb moeten omvormen om het werkend te krijgen (deels met dirty oplossingen).

Zoals je al aangaf is die CreateThread niet een schoonheid:
ik heb dit moeten omzetten:
AfxBeginThread(ClientThread,(LPVOID)client);
naar een CreateThread(...).
Hiervoor heb ik wat code uit een andere persoonlijke multithread app gepakt (vandaar de deels vreemde namen)
Daarnaast wist ik niet hoe ik die "client" doorgepaast kon krijgen heb ik het via een omweg met char gedaan. Uiteraard niet de beste oplossing.

---

Het aanpassen zodat het een schonere code word aan de hand van jouw tips lukt helaas niet helemaal.

In de main thread moest ik alleen 1 ding aanpassen:
client = accept(...); veranderd worden in *client = accept(...);

in de ClientThread compiled hij niet door:
client = *iValue;

(d:\LINUX\mytcp\main.cpp(79): error C2440: '=' : cannot convert from 'LPVOID' to 'SOCKET')

[farlane]

Ik zou je willen aanraden om je user informatie die je in de threadfunctie nodig hebt in een structuur te gooien en een pointer naar die structuur aan je CreateThread mee te geven.

Verder doe je behoorlijk wat dingen met sprintf en strcpy die naar statische buffers schrijven. Ik zou je willen aanraden om deze allemaal te vervangen door strncpy en snprintf zodat je een maximale lengte kan meegeven. Als je het goed doet kun je dan niet meer over je buffergrenzen schrijven. ( Nog beter is om std::string te gebruiken. ( alhoewel snprintf voor mij een onmisbare functie is )

[ShadowLord]

falcon4ever schreef op zondag 04 december 2005 @ 16:35:
Het aanpassen zodat het een schonere code word aan de hand van jouw tips lukt helaas niet helemaal.

In de main thread moest ik alleen 1 ding aanpassen:
client = accept(...); veranderd worden in *client = accept(...);

in de ClientThread compiled hij niet door:
client = *iValue;

(d:\LINUX\mytcp\main.cpp(79): error C2440: '=' : cannot convert from 'LPVOID' to 'SOCKET')

Ik heb de code zo ff uit de losse hand geschreven. Dan vergeet ik nog wel eens wat castings enzo

De 1e heb je perfect opgelost met de *. De 2e is ook vrij simpel... De compiler heeft geen idee wat voro datatype er bedoeld wordt met de pointer van iValue, want het is een void-pointer. Een cast bied hier de oplossing:
client = (SOCKET)*iValue;
Of was het nou: client = *(SOCKET *)iValue;
Weet ik niet meer zeker (ik code de laatste tijd niet zoveel C++ meer jammergenoeg)...

De tips van farlane zijn ook erg goed. Als je meer data mee wil sturen naar je thread is een struct -de- oplossing. Ook snprintf kan idd handig zijn tegen buffer overruns (eigenlijk alle 'n' string functies waar je een max lengte op kan geven). Let bij dit soort funties altijd wel eve op of ze netjes je strings afsluiten met een terminating null. Anders kun je nog steeds errors krijgen als je de string probeert te lezen.