Multi-threaded TFTP Server - Issue With pthreads

Question

I created a simple tftp server that only handles read requests (RRQ). Everything was working fine until I started to make a multi-threaded version of the server. In the application, I simply receive requests in the main thread and I then forward the request to a new thread that does the packet analysis. Therefore, I need to forward the socket, the received packet and the sockaddr_in struct that contains the client information to the thread. With that said, I created a struct that holds all of these and forward them to the pthread.

I get two identical error messages, one in the main and the other in the connection handler. The problems seems to be in the referencing these variables in the struct and retrieving them in the thread. It seems the problem is in the following statements: in connection_handler(): buffer = cthread->buffer; and in the main(): clientT.buffer = buffer;

Here's the code, I've written so far...

#include <stdio.h>
#include <sys/socket.h>
#include <sys/types.h>
#include <netinet/in.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <pthread.h>

#define TIMEOUT 5000
#define RETRIES 3

void *connection_handler(void *);

struct clientThread 
{
    int clientSock;
    char opcode;
    char buffer[1024];  
    struct sockaddr_in client;
};

int main()
{
    char buffer[1024];
    int udpSocket, client_socket, nBytes;
    struct sockaddr_in serverAddr, client;
    socklen_t addr_size;

    udpSocket = socket(AF_INET, SOCK_DGRAM, 0);

    serverAddr.sin_family = AF_INET;
    serverAddr.sin_port = htons(69);
    serverAddr.sin_addr.s_addr = inet_addr("127.0.0.1");
    memset(serverAddr.sin_zero, '\0', sizeof serverAddr.sin_zero); 

    bind(udpSocket, (struct sockaddr *) &serverAddr, sizeof(serverAddr));

    while(1)
    {
        memset(buffer, 0, 1024);
        nBytes = recvfrom(udpSocket,buffer,1024,0,(struct sockaddr *)&client, &addr_size);

        // Creating a thread and passing the packet received, the socket and the sockaddr_in struct...
        pthread_t client_thread;
                struct clientThread clientT;
                strcpy(clientT.buffer,buffer);
                clientT.clientSock = udpSocket;
                clientT.client = client;
        pthread_create(&client_thread, NULL, connection_handler, &clientT);
    }

    return 0;
}


void* connection_handler (void *clientThreaded)
{
        char buffer[1024], filename[200], mode[20], *bufindex, opcode;

        struct clientThread *cthread = clientThreaded;
        int udpSocket = cthread->clientSock;
        strcpy(buffer, cthread->buffer);
        //opcode = cthread->opcode;
        struct sockaddr_in client = cthread->client;

        bufindex = buffer;
        bufindex++;

        // Extracting the opcode from the packet...
        opcode = *bufindex++;

        // Extracting the filename from the packet...
        strncpy(filename, bufindex, sizeof(filename)-1);

        bufindex += strlen(filename) + 1;

        // Extracting the mode from the packet...
        strncpy(mode, bufindex, sizeof(mode)-1);

        // If we received an RRQ...
        if (opcode == 1)
        {
                puts("Received RRQ Request");
                struct timeval tv;
                tv.tv_sec = 5;
                char path[70] = "tmp/";
                char filebuf [1024];
                int count = 0, i;  // Number of data portions sent
                unsigned char packetbuf[1024];
                char recvbuf[1024];
                socklen_t recv_size;

                socklen_t optionslength = sizeof(tv);
                setsockopt(udpSocket, SOL_SOCKET, SO_RCVTIMEO, &tv, optionslength);

                FILE *fp;
                char fullpath[200];
                strcpy(fullpath, path);
                strncat(fullpath, filename, sizeof(fullpath) -1);
                fp = fopen(fullpath, "r");
                if (fp == NULL)
                    perror("");

                memset(filebuf, 0, sizeof(filebuf));


                while (1)               
                {
                        int acked = 0;
                        int ssize = fread(filebuf, 1 , 512, fp);
                        count++;
                        sprintf((char *) packetbuf, "%c%c%c%c", 0x00, 0x03, 0x00, 0x00);
                        memcpy((char *) packetbuf + 4, filebuf, ssize);
                        packetbuf[2] = (count & 0xFF00) >> 8;
                        packetbuf[3] = (count & 0x00FF);

                        int len = 4 + ssize;

                        memset(recvbuf, 0, 1024);
                        printf("\nSending Packet #%i", count);
                        sendto(udpSocket, packetbuf, len, 0, (struct sockaddr *) &client, sizeof(client));

                        for (i=0; i<RETRIES; i++)
                        {
                            int result = recvfrom(udpSocket, recvbuf, 1024, 0, (struct sockaddr *) &client, &recv_size);

                                if ((result == -1) && ((errno == EAGAIN) || (errno == EWOULDBLOCK)))
                                {
                                    sendto(udpSocket, packetbuf, len, 0, (struct sockaddr *) &client, sizeof(client));
                                    printf("\nRetransmitting Packet #%i", count);
                                }

                                else if (result == -1)
                                {
                                   // Handle Error
                                }

                                else
                                {
                                    acked++;
                                        printf("\nReceived ACK For Data Packet #%i", count);
                                        break;
                                }
                    }


                        if (acked!=1)
                        {
                            puts("\nGave Up Transmission After 3 Retries");
                                break;
                        }

                        if (ssize != 512)
                            break;
                }
    }

    return 0;
}

Thanks in advance :)

Answer 1

In the main loop, the variable clientT is local inside that loop, once the loop iterates the variable will go out of scope and any pointer to it will become invalid. Dereferencing such a pointer will lead to undefined behavior.

Instead what you should to is to dynamically allocate the structure using malloc, and pass that pointer instead. Don't forget to free the structure once you're done with it in the thread.

Answer 2

the current posted code, 7pm PDT,
causes the compiler to emit several warnings 
(all of which need to be fixed)
plus some errors.


Errors like: 'buffer = cthread->buffer;'
is copying the address of 'cthread->buffer' to the address of the array 'buffer'.
That probably is not what is wanted.  
suggest something similar to: strcpy(buffer, cthread->buffer);

 #include <time.h> is missing
 so this line: 'struct timeval tv;' is referencing an undefined struct.   

The compiler needs to be run with all warnings enabled.  
then fix the warnings and the errors.
as a minimum, for gcc, use the parameters:
-Wall -Wextra -Wshadow -pedantic
There are plenty of other error/warning messages that can be enabled
but the above list will catch ~99percent of all errors/warnings


Some googling should find info on how to fix the current errors and warnings.  

(although to me, the error/warning messages
make it very clear as to the root cause of the problem.
however, I have been programming for 40 some years)

Each compiler message indicates:
1) which line in the current translation unit (file) 
2) and what is wrong with that line.