0

I am trying to set up a Client, Router, Server in C++ with sockets and UDP using GoBackN windowing. I'm not sure why I'm getting a buffer overrun error in Client::write_log(), at the fprintf() line.

Main run() function

void Client::run()
{
    srand ( time(NULL) );

    //socket data types
    SOCKET client_socket;   // Client socket
    SOCKADDR_IN sa_out;      // fill with server info, IP, port

    char buffer[RAWBUF_SIZE]; // Buffer

    WSADATA wsadata;                                    // WSA connection
    char router[11];                                    // Host data
    char cusername[128], filename[128], direction[3];   // Other header data
    DWORD dwusername = sizeof(cusername);               // Retains the size of the username
    char trace_data[128];

    FILE* logfile = fopen("client.log", "w");

    try 
    {
        if (WSAStartup(0x0202,&wsadata)!=0)
        {  
            throw "Error in starting WSAStartup";
        } 
        else 
        {

            /* Display the wsadata structure */
            cout<< endl
                << "wsadata.wVersion "       << wsadata.wVersion       << endl
                << "wsadata.wHighVersion "   << wsadata.wHighVersion   << endl
                << "wsadata.szDescription "  << wsadata.szDescription  << endl
                << "wsadata.szSystemStatus " << wsadata.szSystemStatus << endl
                << "wsadata.iMaxSockets "    << wsadata.iMaxSockets    << endl
                << "wsadata.iMaxUdpDg "      << wsadata.iMaxUdpDg      << endl << endl;
        }  

        client_socket = open_port(PEER_PORT2);

        prompt("Enter the router hostname: ",router);
        sa_out = prepare_peer_connection(router, ROUTER_PORT2);

        prompt("Enter a filename: ",filename);                  // Retrieve a filename from the client
        prompt("Direction of transfer [get|put]: ", direction);  // Retrieve a transfer direction

        // Make sure the direction is one of get or put
        if(!strcmp(direction,GET) || !strcmp(direction,PUT))
        { 
            // Retrieve the local user name
            GetUserName(cusername, &dwusername);

            int selected = rand() % 256;
            int received, verify;

            int client_num = 0; // Client packet number
            int server_num = 0; // Server packet number

            int progress = 0;
            int rcv;

            cout << "Starting packet ID negotiation..." << endl;

            while(1)
            {

                // Send a random number to the server
                if(progress < 1)
                {
                    memset(buffer, 0, sizeof(buffer));
                    sprintf(buffer,"RAND %d",selected);
                    cout << "Sending " << buffer << endl;

                    if((rcv = send_safe(client_socket, sa_out, buffer, RAWBUF_SIZE, 200)) == 201)
                    {
                        progress = 1;
                    }
                    else if(rcv != 200)
                    {
                        continue;
                    }

                    // Finally wait for a response from the server with the number
                    if(recv_safe(client_socket, sa_out, buffer, RAWBUF_SIZE, 100) == 100)
                    {
                        cout << "Received " << buffer << endl;
                        sscanf(buffer,"RAND %d %d",&verify,&received);
                    }
                    else 
                        continue;

                    progress = 1;
                }

                // Send acknowledgement to the server along with our random number
                memset(buffer, 0, sizeof(buffer));
                sprintf(buffer, "RAND %d", received);
                cout << "Sending " << buffer << endl;

                if(send_safe(client_socket, sa_out, buffer, RAWBUF_SIZE, 201) != 201)
                {
                    progress = 0;
                    continue;
                }

                break;
            }

            client_num = selected % WINDOW_SIZE + 1;
            server_num = received % WINDOW_SIZE + 1;

            cout << "Negotiated server start " << server_num << " and client start " << client_num << endl;

            sprintf(trace_data, "Negotiated srv %d and cli %d", server_num, client_num);
            write_log(logfile, cusername, trace_data);

            // Send client headers
            sprintf(buffer, HEADER, cusername, direction, filename); 
            while((rcv = send_safe(client_socket,sa_out,buffer,RAWBUF_SIZE,777)) != 777)
            {
                if(rcv == 101) 
                    break;
            }

            // Perform a get request
            if(!strcmp(direction,GET))
            {
                get(client_socket, sa_out, cusername, filename, client_num, server_num, logfile);

            }
            else if(!strcmp(direction,PUT))
            {
                put(client_socket, sa_out, cusername, filename, client_num, server_num, logfile);
            }
        }
        else
        {
            throw "The method you requested does not exist, use get or put";
        }

    }
    catch (const char *str) 
    {
        cerr << str << WSAGetLastError() << endl;
    }

    //close the client socket and clean up
    fclose(logfile);
    closesocket(client_socket);

    WSACleanup();      
}

PUT() CODE

void Client::put(SOCKET s, SOCKADDR_IN sa, char * username, char* filename, int client_num, int server_num, FILE* logfile)
{
    char window[FRAME_SIZE * WINDOW_SIZE];  // data retention window
    char buffer[FRAME_SIZE];                // send buffer
    int filesize;
    int size = 0, sent = 0;                 // Trace variables
    char tracebuf[128];

    FILE* send_file;
    fopen_s(&send_file, filename, "rb");

    if(send_file != NULL)
    {    
        // open the file

        // Determines the file size
        fseek(send_file, 0L, SEEK_END);
        filesize = ftell(send_file);
        fseek(send_file, 0L, SEEK_SET);

        //write filesize to logfile
        sprintf(tracebuf, "Filesize %d", filesize);
        write_log(logfile, username, tracebuf);

        //add file size to buffer
        strncpy_s(buffer, "SIZ", 3);
        memcpy(buffer + (3 * sizeof(char)), &filesize, sizeof(int));

        //if filesize was sent to server successfully
        if(send_safe(s,sa,buffer,FRAME_SIZE,101) == 101)
        {
            cout << "Sent filesize, starting transfer..." << endl;

            //reset buffer
            memset(buffer, 0, sizeof(buffer));

            int count = 0;
            int offset = 0;
            int frames_outstanding = 0; //for frames not ACKED
            int next = 0;
            bool resend = false;
            int packet_id;
            int pid_max = WINDOW_SIZE + 1;

            // Start sending the file
            while (1)
            {
                // If the acks mismatch with the current send offset and there are frames not ACKED, this has to be a resend
                if(next != offset && frames_outstanding > 0) 
                    resend = true;

                // Send as many frames as available for the given window size
                while((!feof(send_file) && frames_outstanding < WINDOW_SIZE) || resend)
                {
                    if(next == offset) 
                        resend = false;

                    if(!resend)
                    {
                        if(feof(send_file)) 
                            break;
                        fread(buffer,1,FRAME_SIZE,send_file);                       // Read the next block of data
                        memcpy(window + (offset * FRAME_SIZE), buffer, FRAME_SIZE); // Store the data in the local window
                        //memcpy(window, buffer, FRAME_SIZE); // Store the data in the local window
                        send_packet(s, sa, buffer, FRAME_SIZE, offset);             // Send the packet to peer
                        offset = (offset + 1) % pid_max;                        // Update the offset
                        frames_outstanding++;
                    }
                    else
                    {
                        // Resend by copying the data from the window
                        memcpy(buffer, window + (next * FRAME_SIZE), FRAME_SIZE);
                        send_packet(s, sa, buffer, FRAME_SIZE, next);

                        //log

                        sprintf(tracebuf, "Resending packet %d", next);
                        write_log(logfile, username, tracebuf);

                        next = (next + 1) % pid_max;
                    }
                }

                // Receive ACKs before continuing sending
                //while the # of unacked frames are greater than 0
                while(frames_outstanding > 0)
                {
                    //if packet id < 0
                    if((packet_id = recv_packet(s,sa,buffer,FRAME_SIZE,next)) < 0)
                    {
                        if(count < filesize) 
                            resend = true;

                        break;
                    }

                    // Receive acknowledgment from the client
                    if(!strncmp(buffer,"NAK", 3))
                    {
                        if(packet_id >= 0) 
                            next = packet_id;    // Set the next packet to the packet id
                        break;
                    }
                    else if(!strncmp(buffer,"ALL", 3))
                    {
                        frames_outstanding = 0;
                        break;
                    }

                    //log   
                    sprintf(tracebuf, "Sent %d bytes", count);
                    write_log(logfile, username, tracebuf);

                    memset(buffer, 0, sizeof(buffer));      // Zero the buffer

                    next = (next + 1) % pid_max;            // Update the next frame tracker
                    frames_outstanding--;                  // Another frame has been acked
                }

                if(feof(send_file) && frames_outstanding == 0) 
                    break; // Break when done reading the file and all frames are acked
            }

            cout << "File transfer completed" << endl;
            fclose(send_file);
        }
        else
        {           
            fclose(send_file);
            return put(s,sa,username,filename, client_num, server_num, logfile);
        }
    }
}

WRITE_LOG() CODE

void Client::write_log(FILE* logfile, char* username, char* message)
{   
    fprintf(logfile, "%s > %s\n", username, message);

    memset(message, 0, sizeof(message));
}
stevetronix
  • 1,231
  • 2
  • 16
  • 32
  • 1
    Can you please provide an [`SSCCE`](http://www.sscce.org/). There is so much extra code to sift through, but I would guess one of `username` or `message` is not null-terminated. – clcto Apr 03 '14 at 21:09
  • Yeah, something tells me the act of creating an SSCCE will likely solve your problem for you. But do it and then update the question with something simpler to fiddle with! – aardvarkk Apr 03 '14 at 21:11
  • Except for the occasional cout and the throw, looks like all 'C' to me and not C++. – PaulMcKenzie Apr 03 '14 at 21:38
  • 1
    You don't need to use memset to clear 'buffer' before sprintf(buffer,...). The sprintf call will write to the buffer and null terminate it. The memset to clear it to zeros is superfluous. – Jay Apr 03 '14 at 21:39
  • 1
    Since you're using WinSock, I'm assuming you're using Windows. Well, filenames can be 256 characters, but your char array that holds filenames only goes to 127. Rather than relying on char arrays, why not use std::string? At least doing that will eliminate one probable cause for a buffer overrun. – PaulMcKenzie Apr 03 '14 at 21:43

1 Answers1

2

Not at all sure what you actually intdended to do here:

memset(message, 0, sizeof(message));

but it's likely not doing what you intended. sizeof(message) is either 4 or, more likely, 8 on a modern system. So you are writing 8 zeros to message. Which may not be 8 bytes long in the first place.

Possibly you want:

memset(message, 0, strlen(message));

or:

message[0] = 0; 

(Which will do set the string to "empty", but not fill the entire thing with zeros).

Mats Petersson
  • 126,704
  • 14
  • 140
  • 227